ENH Remove classIsOldLink method

docs/en/09_migrating/02_gorriecoe-migration.md

@@ -277,8 +277,8 @@ For databases that support transactions, the full data migration is performed wi
     - via the browser: `https://www.example.com/dev/build?flush=1`
     - via a terminal: `sake dev/build flush=1`
 1. Run the task
-    - via the browser: `https://www.example.com/dev/tasks/linkfield-tov4-migration-task`
-    - via a terminal: `sake dev/tasks/linkfield-tov4-migration-task`
+    - via the browser: `https://www.example.com/dev/tasks/gorriecoe-to-linkfield-migration-task`
+    - via a terminal: `sake dev/tasks/gorriecoe-to-linkfield-migration-task`
 
 The task performs the following steps:
 

src/Tasks/GorriecoeMigrationTask.php

@@ -97,68 +97,4 @@ class GorriecoeMigrationTask extends BuildTask
             ],
         ],
     ];
-
-    /**
-     * List any has_many relations that should be migrated.
-     *
-     * Keys are the FQCN for the class where the has_many is declared.
-     * Values are the name of the old has_one.
-     *
-     * Example:
-     * <code>
-     * // SiteConfig had two has_many relationships,
-     * // one to Link.MyHasOne and another to Link.DifferentHasOne.
-     * SiteConfig::class => [
-     *   'LinksListOne' => 'MyHasOne',
-     *   'LinksListTwo' => 'DifferentHasOne',
-     * ]
-     * </code>
-     */
-    private static array $has_many_links_data = [];
-
-    /**
-     * List any many_many relations that should be migrated.
-     *
-     * Keys are the FQCN for the class where the many_many is declared. See example below for values.
-     *
-     * Example:
-     * <code>
-     * // SiteConfig had three many_many relationships:
-     * // The table name for "LinksListOne" will be guessed. It wasn't a many_many through and had no extra fields
-     * // The table name for "LinksListTwo" will be guessed. It wasn't a many_many through but did have some extra fields
-     * // The table name for "LinksListThree" is explicitly provided. It was a many_many through with some extra fields
-     * SiteConfig::class => [
-     *   'LinksListOne' => null,
-     *   'LinksListTwo' => [
-     *     'extraFields' => [
-     *       'MySort' => 'Sort',
-     *     ],
-     *   ],
-     *   'LinksListThree' => [
-     *     'table' => 'App_MyThroughClassTable',
-     *     'extraFields' => [
-     *       'MySort' => 'Sort',
-     *     ],
-     *     'through' => [
-     *         'from' => 'FromHasOneName',
-     *         'to' => 'ToHasOneName',
-     *     ],
-     *   ],
-     * ]
-     * </code>
-     */
-    private static array $many_many_links_data = [];
-
-    /**
-     * The table name for the base gorriecoe link model.
-     */
-    private string $oldTableName;
-
-    /**
-     * The old link model class name
-     */
-    private function classIsOldLink(string $class): bool
-    {
-        return $class === 'gorriecoe\Link\Models\Link';
-    }
 }

src/Tasks/LinkFieldMigrationTask.php

@@ -278,9 +278,4 @@ private function migrateHasManyRelations(): void
         }
         $this->extend('afterMigrateHasManyRelations');
     }
-
-    private function classIsOldLink(string $class): bool
-    {
-        return is_a($class, Link::class, true);
-    }
 }

src/Tasks/LinkableMigrationTask.php

@@ -88,28 +88,4 @@ class LinkableMigrationTask extends BuildTask
             ],
         ],
     ];
-
-    /**
-     * List any has_many relations that should be migrated.
-     */
-    private static array $has_many_links_data = [];
-
-
-    /**
-     * List any many_many relations that should be migrated.
-     */
-    private static array $many_many_links_data = [];
-
-    /**
-     * The table name for the base link model.
-     */
-    private string $oldTableName;
-
-    /**
-     * The old link model class name
-     */
-    private function classIsOldLink(string $class): bool
-    {
-        return $class === 'Sheadawson\Linkable\Models\Link';
-    }
 }

src/Tasks/MigrationTaskTrait.php

@@ -126,7 +126,7 @@ private function setOwnerForHasOneLinks(): void
                 }
 
                 // Skip if the has_one isn't for Link, or points at a belongs_to or has_many on Link
-                if (!$this->classIsOldLink($hasOneClass)) {
+                if (!is_a($hasOneClass, Link::class, true)) {
                     continue;
                 }
                 if ($this->hasReciprocalRelation([$hasOneClass], $hasOneName, $modelClass)) {
@@ -435,9 +435,4 @@ abstract public function performMigration(): void;
      * Check if we actually need to migrate anything, and if not give clear output as to why not.
      */
     abstract private function getNeedsMigration(): bool;
-
-    /**
-     * Returns true if the class represents an old link to be migrated
-     */
-    abstract private function classIsOldLink(string $class): bool;
 }

src/Tasks/ModuleMigrationTaskTrait.php

@@ -18,6 +18,62 @@
  */
 trait ModuleMigrationTaskTrait
 {
+    /**
+     * List any has_many relations that should be migrated.
+     *
+     * Keys are the FQCN for the class where the has_many is declared.
+     * Values are the name of the old has_one.
+     *
+     * Example:
+     * <code>
+     * // SiteConfig had two has_many relationships,
+     * // one to Link.MyHasOne and another to Link.DifferentHasOne.
+     * SiteConfig::class => [
+     *   'LinksListOne' => 'MyHasOne',
+     *   'LinksListTwo' => 'DifferentHasOne',
+     * ]
+     * </code>
+     */
+    private static array $has_many_links_data = [];
+
+    /**
+     * List any many_many relations that should be migrated.
+     *
+     * Keys are the FQCN for the class where the many_many is declared. See example below for values.
+     *
+     * Example:
+     * <code>
+     * // SiteConfig had three many_many relationships:
+     * // The table name for "LinksListOne" will be guessed. It wasn't a many_many through and had no extra fields
+     * // The table name for "LinksListTwo" will be guessed. It wasn't a many_many through but did have some extra fields
+     * // The table name for "LinksListThree" is explicitly provided. It was a many_many through with some extra fields
+     * SiteConfig::class => [
+     *   'LinksListOne' => null,
+     *   'LinksListTwo' => [
+     *     'extraFields' => [
+     *       'MySort' => 'Sort',
+     *     ],
+     *   ],
+     *   'LinksListThree' => [
+     *     'table' => 'App_MyThroughClassTable',
+     *     'extraFields' => [
+     *       'MySort' => 'Sort',
+     *     ],
+     *     'through' => [
+     *         'from' => 'FromHasOneName',
+     *         'to' => 'ToHasOneName',
+     *     ],
+     *   ],
+     * ]
+     * </code>
+     */
+    private static array $many_many_links_data = [];
+
+    /**
+     * The table name for the base gorriecoe link model.
+     */
+    private string $oldTableName;
+
     /**
      * Perform the actual data migration and publish links as appropriate
      */

tests/php/Tasks/LinkableMigrationTask/CustomLinkableLink.php

@@ -0,0 +1,15 @@
+<?php declare(strict_types=1);
+
+namespace SilverStripe\LinkField\Tests\Tasks\LinkableMigrationTaskTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\LinkField\Models\Link;
+
+class CustomLinkableLink extends Link implements TestOnly
+{
+    private static string $table_name = 'Linkable_Test_Custom_Link';
+
+    private static array $has_one = [
+        'ForHasMany' => HasManyLinkableLinkOwner::class,
+    ];
+}

tests/php/Tasks/LinkableMigrationTaskTest.php

@@ -81,20 +81,6 @@ public function onBeforeLoadFixtures(): void
         parent::onBeforeLoadFixtures();
     }
 
-    protected function tearDown(): void
-    {
-        parent::tearDown();
-    }
-
-    public function testGetNeedsMigration()
-    {
-        $this->startCapturingOutput();
-        $result = $this->callPrivateMethod('getNeedsMigration');
-        $output = $this->stopCapturingOutput();
-        $this->assertSame($expected, $result);
-        $this->assertSame($expected ? '' : "Nothing to migrate - old link table doesn't exist.\n", $output);
-    }
-
     public function testInsertBaseRows(): void
     {
         // Remove existing links which can cause ID conflicts.
@@ -172,6 +158,100 @@ public function testInsertTypeSpecificRows(): void
         $this->assertEmpty($output);
     }
 
+
+    public function provideSetOwnerForHasOneLinks(): array
+    {
+        return [
+            [
+                'ownerClass' => HasOneLinkableLinkOwner::class,
+                'fixtureRelationsHaveLink' => [
+                    'owner-1' => [
+                        'Link' => true,
+                    ],
+                    'owner-2' => [
+                        'Link' => true,
+                    ],
+                    'owner-3' => [
+                        'Link' => true,
+                    ],
+                ],
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider provideSetOwnerForHasOneLinks
+     */
+    public function testSetOwnerForHasOneLinks(string $ownerClass, array $fixtureRelationsHaveLink): void
+    {
+        $this->startCapturingOutput();
+        $this->callPrivateMethod('insertBaseRows');
+        $this->callPrivateMethod('setOwnerForHasOneLinks');
+        $output = $this->stopCapturingOutput();
+
+        $relationsByID = [];
+        foreach ($fixtureRelationsHaveLink as $fixture => $data) {
+            $data['fixture'] = $fixture;
+            $relationsByID[$this->idFromFixture($ownerClass, $fixture)] = $data;
+        }
+
+        foreach (DataObject::get($ownerClass) as $owner) {
+            if (array_key_exists($owner->ID, $relationsByID)) {
+                $data = $relationsByID[$owner->ID];
+                $ownerFixture = $data['fixture'];
+                $record = $this->objFromFixture($ownerClass, $ownerFixture);
+                foreach ($data as $relation => $hasLink) {
+                    if ($relation === 'fixture') {
+                        continue;
+                    }
+                    /** @var Link $link */
+                    $link = $record->$relation();
+                    if ($hasLink === null) {
+                        // Relation should either not be for link, or should not be in the DB.
+                        if (is_a($link->ClassName, Link::class, true)) {
+                            $this->assertFalse($link->isInDB(), "Relation {$relation} should not have a link saved in it");
+                        }
+                        continue;
+                    } elseif ($hasLink === false) {
+                        // The special case is where the Owner relation was already set to a different record.
+                        $isSpecialCase = $ownerClass === HasOneLinkableLinkOwner::class && $ownerFixture === 'owns-has-one-again';
+                        // Relation should be for link, but should not have its Owner set.
+                        $this->assertTrue($link->isInDB(), "Relation {$relation} should have a link saved in it");
+                        // Can't check OwnerClass here - see https://github.com/silverstripe/silverstripe-framework/issues/11165
+                        $this->assertSame(
+                            [
+                                $isSpecialCase ? $this->idFromFixture(HasOneLinkableLinkOwner::class, 'owns-has-one') : 0,
+                                $isSpecialCase ? 'Link' : null
+                            ],
+                            [
+                                $link->OwnerID,
+                                $link->OwnerRelation,
+                            ],
+                            "Relation {$relation} should not have an Owner set"
+                        );
+                        continue;
+                    }
+                    $this->assertTrue($link->isInDB(), "Relation {$relation} should have a link saved in it");
+                    $this->assertSame(
+                        [
+                            $owner->ID,
+                            $owner->ClassName,
+                            $relation,
+                        ],
+                        [
+                            $link->OwnerID,
+                            $link->OwnerClass,
+                            $link->OwnerRelation,
+                        ],
+                        "Relation {$relation} should have an Owner set"
+                    );
+                }
+            }
+        }
+
+        $this->assertSame("Setting owners for has_one relations.\n", $output);
+    }
+
     private function startCapturingOutput(): void
     {
         flush();

tests/php/Tasks/LinkableMigrationTaskTest.yml

@@ -71,3 +71,14 @@ LinkableMigrationTaskTest_OldLinkTable:
     URL: 'http://www.silverstripe.org'
     OpenInNewWindow: true
     MySort: 12
+
+SilverStripe\LinkField\Tests\Tasks\LinkableMigrationTaskTest\HasOneLinkableLinkOwner:
+  owner-1:
+    Title: 'HasOne Link Owner 1'
+    Link: =>LinkableMigrationTaskTest_OldLinkTable.custom-link-10
+  owner-2:
+    Title: 'HasOne Link Owner 2'
+    Link: =>LinkableMigrationTaskTest_OldLinkTable.custom-link-11
+  owner-3:
+    Title: 'HasOne Link Owner 3'
+    Link: =>LinkableMigrationTaskTest_OldLinkTable.custom-link-12