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.
Comment | File | Size | Author |
---|---|---|---|
#17 | interdiff-13-17.txt | 1.92 KB | maxocub |
#17 | 2827644-17.patch | 5.35 KB | maxocub |
| |||
#13 | interdiff-9-13.txt | 622 bytes | maxocub |
#13 | 2827644-13.patch | 5.37 KB | maxocub |
| |||
#9 | interdiff-7-9.txt | 4.54 KB | maxocub |
Comments
Comment #2
maxocub CreditAttribution: maxocub commentedHere'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.
Comment #3
maxocub CreditAttribution: maxocub commentedI added support for D7, even if #2669964: Migrate Drupal 7 core node translations to Drupal 8 is not in yet.
Comment #4
maxocub CreditAttribution: maxocub commentedBoth #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.
Comment #7
maxocub CreditAttribution: maxocub commentedLet's see if that fixes the existing tests.
Comment #8
maxocub CreditAttribution: maxocub commentedBack to work on new tests.
Comment #9
maxocub CreditAttribution: maxocub commentedHere'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?
Comment #11
maxocub CreditAttribution: maxocub commentedComment #12
maxocub CreditAttribution: maxocub commentedAs 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.
Comment #13
maxocub CreditAttribution: maxocub commentedI removed the D7 part since the node translation migration is not ready. I'll open a new issue for D7.
Comment #14
penyaskitoI 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!
Comment #15
phenaproximaSelf-assigning for review.
Comment #16
phenaproximaTwo nits, but I think it looks really good.
Nit: 'url' should be 'URL'.
Can we use [] instead of array()?
Comment #17
maxocub CreditAttribution: maxocub commentedThanks 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.
Comment #18
phenaproximaI 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.
Comment #22
catchCommitted/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.
Comment #24
maxocub CreditAttribution: maxocub as a volunteer and commented