Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
It looks like there is a missing condition on i18n_string.objectid
in the d7_term_localized_translation
source plugin.
As a result the first tid gets migrated correctly but all of the following tid (taxonomy terms) get the values from the first one.
Shouldn't we add: ->condition('i18n.objectid', $object_id);
in core/modules/taxonomy/src/Plugin/migrate/source/d7/TermLocalizedTranslation.php:73
Proposed resolution
The taxonomy term translation source plugin should use I18nQueryTrait properly. It is included but not actually used, as if it was intended to be used but was forgotten.
Remaining tasks
Patch
Review
Commit
Comment | File | Size | Author |
---|---|---|---|
#33 | 3143676-33.patch | 13.54 KB | ayushmishra206 |
#33 | interdiff_30-33.txt | 2.15 KB | ayushmishra206 |
#30 | interdiff_27-30.txt | 562 bytes | ridhimaabrol24 |
#30 | 3143676-30.patch | 14.24 KB | ridhimaabrol24 |
#27 | 3143676-27.patch | 13.96 KB | MeenakshiG |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commented@herved, are you saying that the migration of the translation of the name and description for term 1 is correct and that the migration for the name and description for term 2 uses the name and description for term 1?
I do not believe that is true and it is tested in d7\MigrateTermLocalizedTranslationTest::testTranslatedLocalizedTaxonomyTerms where two terms with unique names and descriptions and translations are migrated.
Oh and the condition ( ->condition('i18n.objectid', $object_id);) you suggest be added is actually done in I18nQueryTrait, in order to get the translation for the property not already on the row. That is where we get the translations for name and description and put them on the row.
If I have missed your point please copy your source data and the results you are getting and what you expect to get.
Comment #3
herved CreditAttribution: herved commentedExactly, and this only happens when 2 tids have translations for a same language.
The following query doesn't filter on the tid and returns multiple/all rows and gets the first one (1st tid):
Here are 2 patches, 1 with updated and failing tests, and 1 with the fix.
Also
I18nQueryTrait
doesn't seem to be in use there, shouldn't we remove it? Or use it (instead of fixing the query in d7_term_localized_translation) ?Comment #4
herved CreditAttribution: herved commentedAnd here would be the fix using
I18nQueryTrait
Comment #6
quietone CreditAttribution: quietone as a volunteer commented@herved, thanks, I now see the problem. I don't know why this didn't use i18nQueryTrait properly, that was a mistake.
This will also need a migration test. That is, some new entries in the drupal7 fixture and the corresponding assertions in MigrateTermLocalizedTranslationTest.
Comment #7
quietone CreditAttribution: quietone as a volunteer commentedStarting from the patch in #4 I expanded the source plugin test as well as the Kernel migration test.
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedComment #11
quietone CreditAttribution: quietone as a volunteer commentedThat should have been testing with 9.1.x not 8.8.x. Changed the fixture to avoid the word 'Bajoran' and updated the tests.
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedTwo corrections to the entity counts in Upgrade7Test
I checked the D6 source plugin and it does not use this fix, although ideally it could be modified to use I18nQueryTrait.
edit: s/use/need/
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedAnother change to entity count
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedAnd again.
Comment #18
shaktikworking on it.
Comment #19
quietone CreditAttribution: quietone as a volunteer commented@shaktik, hope you see this. I've already made a patch and just running tests locally.
Comment #20
shaktikHi @quietone,
forgot to add interdiff added interdiff_16-19, just solved test case issue.
Comment #21
quietone CreditAttribution: quietone as a volunteer commented@shaktik, In the future, please check if someone is actively working on a patch before assigning it to your self. I think that uploading 5 patches in the last 24 hours, which I have done here, when you assigned it to yourself qualifies as actively working on a patch. There are plenty of migrations issues tagged, Needs Work, that haven't had any updates in weeks and if you wish to start on one of those I am happy to assist.
The patch in #20 will fix the failing test but this should have an assertion added that the term in the newly added localized vocabulary is added.
Comment #22
shaktikHi @quietone,
This issue as Unassigned state that's why I have assigned my self and worked on it.
Thanks,
Shakti.
Comment #23
quietone CreditAttribution: quietone as a volunteer commented@shaktik, I see, but the fact that an issue is not assigned to someone does not mean that it isn't actively being worked on. One needs to look at the recent comments and see what has been happening to decide if it is appropriate to assign it to yourself. The guidelines for Assigning ownership do discourage one from assigning an issue to oneself.
Comment #24
firewaller CreditAttribution: firewaller commentedWhat I was experiencing was the description would use a localized variable from another entity type (i.e. menu instead of term). I managed to resolve this issue in 8.9.x with the attached patch.
Comment #25
mikelutzRe-uploading and re-testing the patch from #21 as that is the correct one with tests.
Comment #26
mikelutzComment #27
MeenakshiG CreditAttribution: MeenakshiG at OpenSense Labs for Drupal India Association commentedRerolled the patch
Comment #29
mikelutzLooks like a related test failure that needs addressing.
Comment #30
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedFixing failed tests.
Comment #31
quietone CreditAttribution: quietone as a volunteer commentedThanks for the rerolls. Please remember to update the tags when the reroll is complete.
Indentation error
This isn't necessary, let's remove the use of the constant.
Here too.
Comment #32
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #33
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedMade the changes suggested in #31. Please Review
Comment #34
quietone CreditAttribution: quietone as a volunteer commentedI reviewed the patch in #33 and it fixes all the points in #31. I think this is RTBC but I can't change the status to that since I have made several patches here.
+1 for RTBC
Comment #35
mikelutzI agree.
Comment #37
catchThis is very nice cleanup.
Committed 876073f and pushed to 9.1.x. Thanks!