Follow-up to #2225775: Migrate Drupal 6 core node translation to Drupal 8
Problem/Motivation
Drupal 6 with the i18n contributed module supports "synchronized fields", that will be kept the same amongst an entire translation set.
Proposed resolution
Turn "synchronized fields" into untranslated fields in the D8 content translation settings.
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff-34-40.txt | 26.42 KB | quietone |
#40 | 2754493-40.patch | 13.34 KB | quietone |
Comments
Comment #2
mikeryanComment #3
Gábor HojtsyUpdated issue summary noting that this is a Drupal 6 contrib feature, not a core feature.
Comment #5
maxocub CreditAttribution: maxocub commentedComment #6
maxocub CreditAttribution: maxocub commentedComment #7
mikeryanComment #12
quietone CreditAttribution: quietone at Acro Commerce commentedThis enables i18nsync in the fixture and has a source plugin and test to get the i18nsync% variables from the variable table. Still to do is a process and destination.
But I've not used synchronized fields and am not sure what is supposed to happen. The IS states to "Turn "synchronized fields" into untranslated fields in the D8 content translation settings" which seems straight forward but what if the synchronized field has translations then why would the content translation setting be set to untranslated?
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedMeant to add the patch. I don't know why but adding the synchronized fields added a lot of data to the fixture.
Comment #14
masipila CreditAttribution: masipila as a volunteer commentedRe-parenting so that all multilingual migrations can be found from the same meta. Moved original parent to related issues.
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedWoohoo! At Drupal South @thomas.feichter showed and explained to me how synchronized fields work!
Comment #16
heddnTriaging the issue queue.
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedStarted using the Drupal 6 database created at Drupal South. It has once synchronized field, an email field, on the Company content type. I then added a text field to the same Content type and set it to synchronized. Then I looked into synchronized fields in Drupal 8 and came up with nothing. I see no setting nor did I find any docs or blogs etc.
So it seems that the value of a D6 synchronized field needs to be migrated to all the translations of the node. But what is the migration doing now, this issue was opened a long time ago and much has changed. To start I looked at the data for Company fields in the D6 db.
Ah, the field values are already duplicated for each field. This is likely to work correctly now without a new migration.
Just tested that by running a full migration using the UI and navigated to the Company nodes and the value for both synchronized fields is shown correctly on both nodes, the English and the French. Nice!
What next?
First, this needs to be tested by someone else to confirm the restuls. And if this is working for CCK fields then close this issue. I've uploaded the database used for testing this.
Second, do we know if this is working for other, non CCK, synchronized fields? In D6, on the Company content type, the following can be marked synchronized.
Comment #18
mikelutzI'm looking into this a little more, but I believe this issue is more about the field settings not coming over properly than the content. The field should be marked as untranslatable, but the content does appear in the database correctly.
Comment #19
mikelutzI believe this would be the test that needs to pass (though it should probably be moved into content_translation, now that I look at it)
Comment #21
mikelutzgah..
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedJust some cleanup on the patch. Setting to NR to make sure new errors are not introduced.
This will need a migration to change the translation setting of the synchronized fields.
Comment #24
quietone CreditAttribution: quietone as a volunteer commentedThe D6 fixture has changed so this most likely needs a reroll.
Comment #25
vacho CreditAttribution: vacho at Skilld commentedPatch rerolled
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedThe field instance migration is changed to include the translatable field and the source plugin determines the value in prepareRow(), it is set true unless this is a synchronized field.
The test file that mikelutz created has been removed and the assertion moved to MigrateNodeTest because MigrateNodeTest already does lots of assertions on the migrated fields. Tests have been added to MigrateFieldInstance as well.
Comment #28
quietone CreditAttribution: quietone as a volunteer commentedI've got some fixes locally for this, just need some time to get it ready.
Comment #29
quietone CreditAttribution: quietone as a volunteer commentedHmm, that should be NW
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedThis add i18nsync to the list of noUpgradePaths since it provides functionality, the source_module has not changed. Also the node type the sync field is on has been changed from company to employee because using company conflicted with the test MigrateLanguageContentSettingsTest.
Comment #32
quietone CreditAttribution: quietone as a volunteer commentedForgot the entity counts
Comment #34
quietone CreditAttribution: quietone as a volunteer commentedAdjust the the other entity counters for the new field.
Comment #35
quietone CreditAttribution: quietone as a volunteer commentedLast thing is to check the changes to the fixture. Can anymore be removed?
Comment #36
quietone CreditAttribution: quietone as a volunteer commentedDouble checked the changes to the fixture and I'm just not sure if all the changes to locales_source are needed. I know they are not needed for the tests but maybe they are needed when working via a Drupal 6 UI.
Ready for review.
Comment #37
quietone CreditAttribution: quietone as a volunteer commentedComment #38
mikelutzComment #39
mikelutzThe tiny test I submitted in #21 was completely rewritten, so I think I can still review this. The approach is correct, as are the tests that check it. I think the locales_source changes should be removed from the fixture. It will shrink the patch, and they are an easy enough hunk to remove. They aren't necessary for working in the UI, they just get inserted while working in the UI anytime t() encounters a new string. I think with them removed this is ready to go.
Comment #40
quietone CreditAttribution: quietone as a volunteer commentedThanks for explaining about locales_source.
Rather tired but I've gone ahead and removed that changes to locales_source. Let's hope my fingers are working well.
Comment #41
mikelutzLooks good to go, and a more manageable size. Thanks!
Comment #43
mikelutzRandom test fail..
Comment #44
Gábor HojtsyLooks good to me! Thanks all!