D6->D8: Migrating links without leading slash leads to fatal error

Let's get some sprinters (novice welcome) to test this on both D6 and D7 - do you see this problem with migrating link fields?

Yes, this problem still persists. Confirmed the steps to reproduce:

Used Drupal versions:

Drupal 6.38

migrated to

Drupal 8.2.x

Below is the error message when attempting to view the offending node:

----------------------------------------------------------------------------------------------------

The website encountered an unexpected error. Please try again later.

InvalidArgumentException: The URI 'test' is invalid. You must use a valid URI scheme. in Drupal\Core\Url::fromUri() (line 276 of core/lib/Drupal/Core/Url.php).

I think I even saw it happening with the frontpage (or maybe 403, or 404 link) on the system settings screen. Going to the frontpage throws an error then, but I'll make sure I can confirm.

In the process of testing if this is an issue in D7 with Link 7.x-1.4 - the link field does not allow for an invalid URL, instead resenting an error message:

"The value test provided for Link is not a valid URL."

This may not be an issue with a Link field in D7 migrations

@ancapaaron and @bsnodgrass - thanks for the manual testing.

Next step here is updating the automated tests to catch this - notably, the link fields in both Drupal 6 and Drupal 7 fixtures are all full HTTP links, we need to test with local links.

Adding tags for Drupal source and destination version. migrate-d7-d8 added based on mikeryan's comment (#8) about changing the test data in both the D6 and D7 fixtures.

Changed the d6 dump and added a test for an internal link.

I don't think the slash matters. Importing '/node/10' also fails with the same message.

Here's an attempted fix. There's a bit of almost-duplicated code from LinkWidget, but I'm not sure if there's any good way to abstract it out—we want to auto-add a slash, and they don't.

Taking off novice. There isn't a clear enough path forward for a novice to pick this up. Are we sure that internal: is necessary? Doesn't it depend on the type of link field? There are three types, store only internal. Store external or store mixed. Are all of these treated the same way?

@heddn: I believe internal: is always necessary for D8 link fields, unless the link data is external. We check if it's external before adding "internal:".

For migrating the content, we don't particularly care what the D6 link field settings are—we just handle any type.

Over in #2345833: Convert assetEqual to assertIdentical in migrate_drupal it was decided to use assertIdentical instead of assertEquals.

Oh, thanks for the note. I just assumed I would use the one not marked @deprecated, but I'll go with the flow :)

assertIdentical is deprecated. Use assertSame.

testbot hiccup, working again

@vasi, sorry about that.

Added a unit test. I'm not sure what all the test cases are, but they can be added to the data provider by the other contributors.

Generally looks good, thanks! Some feedback...

1. +++ b/core/modules/link/src/Plugin/migrate/process/d6/CckLink.php
   @@ -37,6 +37,35 @@ public static function create(ContainerInterface $container, array $configuratio
   +   * Turn a Drupal 6 or 7 URI into a Drupal 8-compatible format.
   ...
   +   *   The 'url' value from Drupal 6 or 7.
   

   This is a D6-specific plugin.

2. +++ b/core/modules/link/src/Plugin/migrate/process/d6/CckLink.php
   @@ -37,6 +37,35 @@ public static function create(ContainerInterface $container, array $configuratio
   +    $uri = preg_replace('#^/?#', '/', $uri);
   

   A rather subtle means of prepending - would it be clearer and/or more efficient to do something like

   $uri = '/' . ltrim('/', $uri);
   

   ?

3. +++ b/core/modules/link/tests/src/Unit/Plugin/migrate/process/d6/CckLinkTest.php
   @@ -0,0 +1,52 @@
   +      'No leading forward slash' => [
   +        'node/10',
   

   Let's also test with the leading slash, just to make sure we don't accidentally double it up.

4. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
   @@ -3420,9 +3420,9 @@
   -  'field_test_link_url' => 'http://www.example.com/buy-one-upon-a-time',
   +  'field_test_link_url' => 'node/10',
   

   Am I correct in assuming the fixture has no leading-slash cases? As previously, let's add one.

Is it worth looking at the patch in #2674090: Unable to migrate D7 link fields and seeing what might be shared between the two?

Both of these issues look like they are practically solving the same problem. Can we use the same process plugin for both migrations?

Also, not sure we need to add every test case to the fixture with the unit test.

I think it would be good to use the same process plugin. Once we do that, we could also use the same CckField too!

The only critical difference between the process plugins now is that the D7 one tries to turn links into 'entity:' URIs:

+++ b/core/modules/link/src/Plugin/migrate/process/d7/CckLink.php
@@ -0,0 +1,73 @@
+      $system_path = \Drupal::service('path.alias_manager')->getPathByAlias($path_alias);
+      if ($system_path != $path_alias && substr($system_path, 0, 6) == '/node/') {
+        $uri = 'entity:' . substr($system_path, 1);
+      }

I'm not sure if this is something we want. There's no way to tell whether the creator of the D6 content intended to link to "the node currently at this path, even when it changes path" or "whatever is at this path, even if it's later something different".

I'd recommend that for now we go with this patch as-is, assuming it's well-reviewed. Then in #2674090: Unable to migrate D7 link fields we can handle merging the process and CckField plugins.

In addition the the things we already looked at, there's the existing internal_uri process plugin, and also one being added by #2669978: Migrate D7 Menu Links. We probably want to find a way to coalesce these.

+++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
@@ -3422,7 +3422,7 @@
+  'field_test_text_single_checkbox2_value' => 'Off',

This change is a mistake and has been removed. I must have ticked the checkbox before creating the initial patch.

Am I correct in assuming the fixture has no leading-slash cases? As previously, let's add one.

Yes, that is correct. Drupal6 didn't allow me to enter a leading slash to a link. Therefor, I have added a leading slash test in MigrateNodeTest.php. This works for internal links. With an external link, it prepends 'internal:' resulting in 'internal:/https://www.drupal.org/node/2127611'.

I agree with vasi on getting this in and then dealing with the other link related issues.

Failures are unrelated to this patch, and pass locally. Retesting.

Tests passing now, so NR.

+++ b/core/modules/link/src/Plugin/migrate/process/d6/CckLink.php
@@ -37,6 +37,32 @@ public static function create(ContainerInterface $container, array $configuratio
+   * Turn a Drupal 6 or 7 URI into a Drupal 8-compatible format.

This is a 6-only plugin.

Otherwise looks fine - the comment can be fixed on commit.

Test failure looks irrelevant, requeued.

+++ b/core/modules/node/tests/src/Kernel/Migrate/d6/MigrateNodeTest.php
@@ -92,12 +99,27 @@ public function testNode() {
+    // 2. Add an leading slash to an internal link.

Fixed on commit - 'a leading'. And also the Drupal 7 mention per #36

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!