Problem/Motivation
Revisited this and discovered that the destination property content_translation_update_definitions
is no longer used anywhere in core. I used grep -ir content_translation_update_definitions core
and found it declared in 3 migration yml files but not anywhere else.
Further exploration shows that content_translation_update_definitions
was added in #2225775: Migrate Drupal 6 core node translation to Drupal 8 and later removed in #2599228: Programmatically created translatable content type returns SQL error on content creation.
I don't think any of the three migrations were committed at the time that the property was removed. I suspect these migrations started as a copy paste of the migrations added in #2225775: Migrate Drupal 6 core node translation to Drupal 8 and the use of the property,or lack thereof, was never questioned or tested.
Original IS
d6_language_content_settings.yml, d7_language_content_settings.yml, and d7_node_translation.yml all have a property content_translation_update_definitions on the destination, which set to node. I'm not sure what the purpose of the setting is but it implies that it should be changed for other entities. It seems like for other entities one would copy the migration and change that destination property. Would it not be more flexible to have this as a property in the pipeline?
destination:
content_translation_update_definitions:
- node
And note that the source uses a constant value of node for the entity type. While that is a similar problem, it is a much more common pattern and the entity type property is in the pipeline and therefor easier to modify.
Proposed resolution
Remove usage of content_translation_update_definitions in destination plugins.
Comment | File | Size | Author |
---|---|---|---|
#7 | 2930050-7.patch | 1.86 KB | quietone |
Comments
Comment #6
quietone CreditAttribution: quietone at Acro Commerce commentedComment #7
quietone CreditAttribution: quietone as a volunteer commentedShould be not failures.
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedComment #9
heddnBased on the IS, we don't use these values any more. Let's get rid of them. Ideally should be backported to 8.8.
Comment #10
alisonI'll work on a backport -- just so I know, is there a fancy/automatic process for doing this, or is it just a straightforward/manual thing, "make the same changes against that other version of the code"?
Comment #11
alison...actually, I think I'm misunderstanding something -- the patch in #7 applies fine on 8.8.x, so does that mean there's no need for backporting, it's just something the committer will need to do when they commit? (Or was it just that we needed someone to make sure the patch at #7 applies fine on 8.8.x? If so -- done.)
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedThis doesn't need backporting, it is removing what is effectively dead code.
Comment #13
alexpottCommitted and pushed 44416f1c03 to 9.0.x and 4669dbe1f8 to 8.9.x. Thanks!
Going to see about getting this into 8.8.x.
Comment #16
alexpottDiscussed with @catch and we agreed to backport.
Comment #18
xjmWe don't have a process for using tags like "needs backport to 8.8.x"; it's just an extra committer +1 or sometimes a release management decision during the alpha. Just for future reference. :)
This does indeed appear to be dead code that can be removed non-disruptively so the backport makes sense.