Steps to reproduce:
- Set up a D6 content type with a link field
- Enter a link URL like "test", something without a protocol or leading slash
- Run a D6 to D8 migration. It runs smoothly.
- Try to visit the migrated node. You'll get an error.

In either 8.0.5 or 8.1 head the result is a fatal error.

This is also an issue for Search API, since it will kill the entire search.

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

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

floydm created an issue. See original summary.

floydm’s picture

My workaround, for the moment, is to subclass the CckLink process plugin and override the transform method, changing:

    // Massage the values into the correct form for the link.
    $route['uri'] = $value['url'];

to:

    // Massage the values into the correct form for the link.
    $url = $value['url'];

    if (filter_var($url, FILTER_VALIDATE_URL)){
      $route['uri'] = $url;
    } elseif (substr($url, 1) == '/') {
      $route['uri'] = 'internal:' . $url;
    } else {
      $route['uri'] = 'internal:/' . $url;
    }

And then in my migration yml switching all link fields to use my process plugin instead of cck_link. If I do that and rerun my migration, the migrated nodes with the links load fine.

That's in a migration to 8.0.5. Customizing a d6 migration to 8.1 has me baffled.

catch’s picture

Priority: Normal » Major
Issue tags: +Migrate critical
mikeryan’s picture

Issue tags: +Novice

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

ancapaaron’s picture

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).

swentel’s picture

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.

bsnodgrass’s picture

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

mikeryan’s picture

Issue tags: +Needs tests

@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.

quietone’s picture

Issue tags: +migrate-d6-d8, +migrate-d7-d8

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.

quietone’s picture

quietone’s picture

Status: Active » Needs review
FileSize
2.2 KB

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

Status: Needs review » Needs work

The last submitted patch, 11: 2698083-d6-fail-11.patch, failed testing.

heddn’s picture

Issue summary: View changes
vasi’s picture

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

vasi’s picture

Status: Needs work » Needs review
FileSize
3.89 KB
2.89 KB

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.

heddn’s picture

Issue tags: -Novice

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?

vasi’s picture

@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.

quietone’s picture

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

vasi’s picture

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

Mac_Weber’s picture

The last submitted patch, 20: test_only-2698083-20.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 20: d6_d8_migrating_links-2698083-20.patch, failed testing.

Mac_Weber’s picture

Status: Needs work » Needs review

testbot hiccup, working again

quietone’s picture

@vasi, sorry about that.

Sam152’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 25: d6_d8_migrating_links-2698083-25.patch, failed testing.

mikeryan’s picture

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.

mikeryan’s picture

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

Sam152’s picture

Status: Needs work » Needs review
FileSize
5.9 KB
1.89 KB

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.

vasi’s picture

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.

vasi’s picture

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.

quietone’s picture

+++ 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.

Status: Needs review » Needs work

The last submitted patch, 32: d6_d8_migrating_links-2698083-32.patch, failed testing.

quietone’s picture

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

quietone’s picture

Status: Needs work » Needs review

Tests passing now, so NR.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: d6_d8_migrating_links-2698083-32.patch, failed testing.

mikeryan’s picture

Status: Needs work » Reviewed & tested by the community

Test failure looks irrelevant, requeued.

  • catch committed d8d92b5 on 8.2.x
    Issue #2698083 by quietone, Sam152, Mac_Weber, vasi: D6->D8: Migrating...

  • catch committed 698ff30 on 8.1.x
    Issue #2698083 by quietone, Sam152, Mac_Weber, vasi: D6->D8: Migrating...
catch’s picture

Status: Reviewed & tested by the community » Fixed
+++ 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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.