Problem/Motivation

Sub-issue for #2746527: [META] Handle data related to Drupal 6 and 7 node translations with different IDs.

When migrating path aliases of translated nodes, some aliases will point to non-existent nodes since source and translations are stored in the same node.

If in D6 we have node/1 as the source in English and node/2 as the French translation, in D8 we will only have node/1. If we had an alias for node/2, it won't work anymore.

Proposed resolution

When migrating path aliases, use the migration process plugin to get the translation source nid and correct the translation path aliases.

Remaining tasks

Write a patch. Write a test. Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

maxocub created an issue. See original summary.

maxocub’s picture

Title: Fix path alias migration of translated nodes [D6] » [PP-2] Fix path alias migration of translated nodes [D6 & D7]
Status: Active » Postponed
Issue tags: +drupal7, +migrate-d7-d8
Related issues: +#2827656: Once a process plugin returns multiple values, all following plugins are expected to handle multiple values, +#2767643: Scalar to array migration returns NULL
FileSize
2.23 KB

Here's a work in progress.

This issue is postponed on #2827656: Once a process plugin returns multiple values, all following plugins are expected to handle multiple values and #2767643: Scalar to array migration returns NULL as the process pipeline using explode, extract and migration doesn't work and the migration plugin returns NULL.

maxocub’s picture

Title: [PP-2] Fix path alias migration of translated nodes [D6 & D7] » Fix path alias migration of translated nodes [D6 & D7]
Issue tags: +Needs tests
FileSize
2.83 KB
1.93 KB

I added support for D7, even if #2669964: Migrate Drupal 7 core node translations to Drupal 8 is not in yet.

maxocub’s picture

Status: Postponed » Needs review

Both #2827656: Once a process plugin returns multiple values, all following plugins are expected to handle multiple values and #2767643: Scalar to array migration returns NULL are now fixed, so this is no longer postponed.

It still needs tests, but I'll put it on needs review to see if it breaks existing tests.

Status: Needs review » Needs work

The last submitted patch, 3: 2827644-3.patch, failed testing.

The last submitted patch, 2: 2827644-2.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
1.12 KB

Let's see if that fixes the existing tests.

maxocub’s picture

Status: Needs review » Needs work

Back to work on new tests.

maxocub’s picture

Status: Needs work » Needs review
FileSize
5.98 KB
3.23 KB
4.54 KB

Here's a test for Drupal 6.

Since #2669964: Migrate Drupal 7 core node translations to Drupal 8 is not in yet, I can't write a test for Drupal 7. Should I reduce the scope to Drupal 6 only and open another issue for Drupal 7?

Status: Needs review » Needs work

The last submitted patch, 9: 2827644-9-test-only.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
maxocub’s picture

Status: Needs review » Needs work

As discussed in the weekly migrate meeting, we'll focus on Drupal 6 here since Drupal 7 node translation is not fixed yet. So I'll remove the D7 part and open an issue.

maxocub’s picture

Title: Fix path alias migration of translated nodes [D6 & D7] » Fix path alias migration of translated nodes [D6]
Assigned: maxocub » Unassigned
Status: Needs work » Needs review
Issue tags: -drupal7, -migrate-d7-d8, -Needs tests
FileSize
5.37 KB
622 bytes

I removed the D7 part since the node translation migration is not ready. I'll open a new issue for D7.

penyaskito’s picture

I didn't test it manually, but code looks good and added test makes sense. I'm wondering how that plays with #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations once it is fixed, but I'm sure @maxocub has a good answer as he is working on both :-)

I won't RTBC until someone (or a future me) tests it, but looks great, thanks!

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

phenaproxima’s picture

Two nits, but I think it looks really good.

  1. +++ b/core/modules/path/tests/src/Kernel/Migrate/d6/MigrateUrlAliasTest.php
    @@ -93,4 +105,33 @@ public function testUrlAlias() {
    +   * Test the url alias migration with translated nodes.
    

    Nit: 'url' should be 'URL'.

  2. +++ b/core/modules/path/tests/src/Kernel/Migrate/d6/MigrateUrlAliasTest.php
    @@ -93,4 +105,33 @@ public function testUrlAlias() {
    +    $path = $alias_storage->load(array('alias' => '/the-real-mccoy'));
    

    Can we use [] instead of array()?

maxocub’s picture

Thanks for the reviews!

@phenaproxima: I addressed the nits.

@penyaskito: I don't think we have to worry about #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations here since we are replicating what's in Drupal 6 to a fresh install of Drupal 8. Thus the URL aliases should be set as translatable since it's not something we can change in D6. The only thing about #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations that could be problematic with the path alias upgrade is for untranslatable node types which should have their aliases langcode set as 'und'. I think this is out of scope and to early to deal with since #2689459: If you don't want to translate your URL alias, the original URL alias won't work with your translations is not yet in.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I think we're good to go.

I had asked @maxocub on IRC to add a few assertions that prove the aliases are loaded when calling things like $entity->toUrl() and whatnot. He tried, but ran into a wall of problems relating to the alias whitelist. Unless anyone has suggestions for how to hack around that, I think due diligence has been rendered.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2827644-17.patch, failed testing.

  • catch committed 7f81be3 on 8.3.x
    Issue #2827644 by maxocub, phenaproxima: Fix path alias migration of...

  • catch committed e0d1869 on 8.2.x
    Issue #2827644 by maxocub, phenaproxima: Fix path alias migration of...
catch’s picture

Status: Needs work » Fixed

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

Think the CI error was a retest one-off but will keep an eye out.

Status: Fixed » Closed (fixed)

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

maxocub’s picture