Problem/Motivation

Working on #2746541: Migrate D6 and D7 node revision translations to D8 I was getting the following error, see https://www.drupal.org/pift-ci-job/1411173

1) Drupal\Tests\taxonomy\Kernel\Migrate\d6\MigrateTaxonomyVocabularyTranslationTest::testTaxonomyVocabularyTranslation
Undefined index: id

The undefined index is because it is trying to write to $definition['d6_term_node_translation:1'] which does not exist. It would exist if content_translation was enabled, but it is not in the test. The d6_term_node_translation plugin is added to the list to alter if config_translation is enabled but that migration is in content_translation so it is not in $definition.

Also, the plugin alter assumes that the all the ids it writes to exist in the $definition array. I reckon that should that be changed.

Proposed resolution

Add d6_term_node_translation if content_translation is enabled. Or should it be both config_translation and content_translation?
Add a check that the definition exists before writing to it

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#22 interdiff.3082719.12-22.txt627 bytesmikelutz
#22 3082719-22.drupal.reroll_for_8_7.patch11.14 KBmikelutz
#12 interdiff-8-12.txt588 bytesquietone
#12 3082719-12.patch11.13 KBquietone
#8 3082719-8.patch11.13 KBquietone
#8 3082719-8-fail.patch9.89 KBquietone
#5 3082719-5.patch1.24 KBquietone
#5 3082719-5-fail.patch1.38 KBquietone
#4 3082719-4.patch764 bytesquietone
#4 interdiff-2-4.txt2.18 KBquietone
#2 3082719-2.patch2.06 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review
FileSize
3082719-2.patch2.06 KB

Not sure if config_translation should be checked or not. And looking at the history, I was the one to add it in #2859297: Migrate taxonomy term references for D6 Node translations. Seems to me it should only check for content_translation. Is that correct?

The attached patch checks that the definition exists before writing to it and it adds d6_term_node_translation if both config_translation and content_translation are enabled.

Ghost of Drupal Past’s picture

Ghost of Drupal Past
he/him
CreditAttribution: Ghost of Drupal Past commented

Maybe we could use foreach (array_intersect($plugin_ids, array_keys($definitions)) as $plugin_id) {? I am not sure which one is better, just offering an idea, feel free to ignore.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
interdiff-2-4.txt2.18 KB
3082719-4.patch764 bytes

Yes, I like that change. Thx.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
3082719-5-fail.patch1.38 KB
3082719-5.patch1.24 KB

Now let's change the test to determine if translation is available from testing if config_translation is enabled to testing if content_translation is enabled. The test should fail because content_translation is not enabled in the test and thus the d6_term_node_translation migration is not discovered.

edit: make the first sentence clearer

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

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Can we add a test that tests this scenario and ensures the migration definition is not altered if it doesn't exist?

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3082719-8-fail.patch9.89 KB
3082719-8.patch11.13 KB

Adding a test.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: +Migrate critical

Adding migrate critical tag because this is needed in #2746541: Migrate D6 and D7 node revision translations to D8 an already large patch with a different scope.

The last submitted patch, 8: 3082719-8-fail.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

NickDickinsonWilde’s picture

NickDickinsonWilde
He/Him/His
Primary language English
Location Victoria, BC (T'So-uke lands)
CreditAttribution: NickDickinsonWilde at Taoti Creative commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.
One very minor standards issue which can be fixed on commit IMO:

+      'primary key' => ['vid',],

should be ...['vid']...

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Reviewed & tested by the community » Needs review
FileSize
3082719-12.patch11.13 KB
interdiff-8-12.txt588 bytes

Sorry, I missed that. Here is the fix.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and commented
Status: Needs review » Reviewed & tested by the community

+1 to RTBC, pending tests.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and commented
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: +blocker

I think this is actually a blocker to #2746541: Migrate D6 and D7 node revision translations to D8

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Title: migrate_drupal_migration_plugins_alter should only alter definitions that exist » migrate_drupal_migration_plugins_alter() should only alter definitions that exist

  • Gábor Hojtsy committed 04febf6 on 8.8.x 
    Issue #3082719 by quietone, mikelutz, NickWilde, Charlie ChX Negyesi:...

  • Gábor Hojtsy committed b578483 on 8.7.x 
    Issue #3082719 by quietone, mikelutz, NickWilde, Charlie ChX Negyesi:...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks all, looks good and straightforward.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Fixed » Needs work

I think this broke 8.7.x:
https://www.drupal.org/pift-ci-job/1429277

(All environments are failing on 8.7.x but not 8.8.x.)

Reverted against 8.7.x and retesting against that branch.

  • xjm committed 8c5cf18 on 8.7.x 
    Revert "Issue #3082719 by quietone, mikelutz, NickWilde, Charlie ChX...
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at Four Kitchens commented
Status: Needs work » Needs review
FileSize
3082719-22.drupal.reroll_for_8_7.patch11.14 KB
interdiff.3082719.12-22.txt627 bytes

Here's the reroll for 8.7. Prior to #2926068: Deprecate system_rebuild_module_data() and remove usages in core the module handler in core incorrectly depended on the system module.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

We're fine again.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Do we need to do this in 8.7.x? - #2746541: Migrate D6 and D7 node revision translations to D8 will only land in 8.8.x

I think this issue should be marked fixed and set to 8.8.x.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.7.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Agreed with #24, moving back to 8.8.x and fixed.

Status: Fixed » Closed (fixed)

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