Hi,

the d6_url_alias migration fails when the src path in the alias from D6 contains no slashes.

The failure message is this one:

Processed 527 items (526 created, 0 updated, 1 failed, 0 ignored) - done with 'ao2_it_d6_url_alias' [status]
$ drush migrate-messages ao2_it_d6_url_alias
source_ids_hash level message
93b99e18ed0ac290d6853182b67e4127f473d77e0fe07d1f7d2fbe5c69fec4f7 1 Array index missing, extraction failed.

This comes from core/modules/migrate/src/Plugin/migrate/process/Extract.php.

The cause seems to be an alias which does not contain any slashes in the src path:

SELECT * FROM `url_alias` WHERE `src` NOT LIKE '%/%'

pid	src	dst		language
----------------------------------------
294	contact	contatto	it

I think the above is a valid alias but it's a case not covered by d6_url_alias in its process part:

...
  node_translation:
    -
      plugin: explode
      source: src
      delimiter: /
    -
      plugin: extract
      index:
        - 1
    -
      plugin: migration_lookup
      migration:
        - ao2_it_d6_node_translation_blog
        - ao2_it_d6_node_translation_news
        - ao2_it_d6_node_translation_page
...

AFAIU here the migration extracts the second element (index: 1) of the path to pass in node_translation, knowing that core/modules/path/src/Plugin/migrate/destination/UrlAlias.php will use this data only in case the path actually represents a node, having the guarantee that the extracted element is the nid.

This "blind" approach generally works (for instance it causes no problem with paths like taxonomy/term/xxx because node_translation will be ignored in the destination plugin for these paths), but it breaks if the src path does not contain any slashes.

Thanks,
Antonio

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ao2 created an issue. See original summary.

ao2’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Active » Needs review
FileSize
1.46 KB

The issue is also in 8.5.x and the attached patch should fix it.
Now I can import all my url aliases.

Consider applying the patch with git am if you like to preserve the detailed commit message.

Ciao,
Antonio

Status: Needs review » Needs work
ao2’s picture

Status: Needs work » Needs review
quietone’s picture

Let's start with a fail test. Added a url alias without a slash to the test fixture and updated the test.

Status: Needs review » Needs work

The last submitted patch, 5: 2927227-5-fail.patch, failed testing. View results

ao2’s picture

Thanks quietone,

The second assertion in the patch from #5 seems wrong, the alias is set with 'language' => '' but then the assertion checks for $this->assertSame('en', $path['langcode']); I think the latter could be $this->assertSame('und', $path['langcode']);.

Anyways, I ran some local tests with:

../../vendor/bin/phpunit modules/path/tests/src/Kernel/Migrate/d6/MigrateUrlAliasTest.php

With the current vanilla core all the tests pass:

OK (2 tests, 31 assertions)

With your new test from #5 there is indeed a failure:

There was 1 failure:

1) Drupal\Tests\path\Kernel\Migrate\d6\MigrateUrlAliasTest::testUrlAliasWithTranslatedNodes
Failed asserting that null is identical to '/admin'.

.../web/core/modules/path/tests/src/Kernel/Migrate/d6/MigrateUrlAliasTest.php:138

FAILURES!
Tests: 2, Assertions: 31, Failures: 1.

After applying the patch from #2, and fixing the second assert as per my comment above, the tests pass:

OK (2 tests, 33 assertions)

So the proposed solution should be OK.

BTW, the failure is not about translated nodes, so maybe the new assertions should not be in testUrlAliasWithTranslatedNodes()? And do we really need the one about $path['language']?

Ciao,
Antonio

ao2’s picture

Ping.

jofitz’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
1.68 KB

Combine @quietone's test-only patch from #5 (with the langcode correction suggested by @ao2 in #7) with @ao2's fix patch from #2.

The last submitted patch, 9: 2927227-9-test_only.patch, failed testing. View results

quietone’s picture

Status: Needs review » Needs work

BTW, the failure is not about translated nodes, so maybe the new assertions should not be in testUrlAliasWithTranslatedNodes()? And do we really need the one about $path['language']?

Right, this should be changed and it looks like the last thing to do. I don't have time now.

jofitz’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
1.66 KB

Moved the test into testUrlAlias().

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Olarin’s picture

For anyone still using Drupal 8.4 here's a patch that applies against that (when the migrations directory was still the migration_templates directory). Meanwhile I can confirm that the fix works for me.

Olarin’s picture

And just for the sake of argument let's demonstrate that it applies against 8.6.x, too.

quietone’s picture

Issue tags: +migrate-d6-d8

Just adding tags

heddn’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
    @@ -46583,6 +46583,12 @@
    +  'dst' => 'noslash',
    

    This is a super nit. In general, I like the name noslash in the test case. However, its very confusing. No one will ever see the value without the slash in the fixture. All they will see is this reference in the test case. And it obviously has a slash in it here. Which made me review the code several times. And the only reason it made sense was because I could see the diff with the fixture changes. So in this case, picking another name would less confusing. Perhaps source-noslash or even example would work just as well. Or a comment in the test and keep it noslash?

  2. +++ b/core/modules/path/migrations/d6_url_alias.yml
    @@ -27,6 +27,7 @@ process:
    +      default: 'INVALID_NID'
    

    What is this all about? Can we add a comment explaining?

jofitz’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
1.77 KB

Address @heddn's review in #17.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for pandering to my request. RTBC.

alexpott’s picture

Shouldn't we fix the D7 one too?

alexpott’s picture

As far as I can see core/modules/path/migrations/d7_url_alias.yml has the same problem.

alexpott’s picture

Issue tags: +Needs followup

@quietone wants a follow-up. Needs to be created before a commit.

quietone’s picture

Yea, I'm all into small incremental steps right now, just like the small steps I'm taking to clean out the spare room.

Followup made #2942388: Ensure d7_url_alias migration works for paths without a slashes

alexpott’s picture

Adding issue credit.

  • alexpott committed 17a47b8 on 8.6.x
    Issue #2927227 by Jo Fitzgerald, Olarin, ao2, quietone, heddn:...

  • alexpott committed bf47414 on 8.5.x
    Issue #2927227 by Jo Fitzgerald, Olarin, ao2, quietone, heddn:...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs followup

Committed 17a47b8 and pushed to 8.6.x. Thanks!
Committed bf47414 and pushed to 8.5.x. Thanks!

Backported to 8.5.x since this is a bugfix.

Status: Fixed » Closed (fixed)

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