Problem/Motivation
We added migration for Drupal 6 node translation to Drupal 8 content translation in #2225775: Migrate Drupal 6 core node translation to Drupal 8. The situation with Drupal 7 is very similar in a way because Drupal 7 core also uses node copies for translations, but it is different because Drupal 7 also has configurable fields in core and some fields that were built-in in Drupal 6 are configurable in Drupal 8.
Also, Drupal 7 has per field translation support which facilitates the Entity Translation contributed module but not used in core for node translation, so that should be dealt with carefully when doing the migration.
Proposed resolution
- Migrate field storage config to default to translatable as is with Drupal 8 default fields.
- Migrate field instance config to set translatable based on whether the parent content type was translatable (based on the language_content_type_{bundle} variable from D7.
- Make the node migration sources include only the default translation (same as with the D6 migration).
- Add auxiliary migrations to bring in the translations (same as with the D6 migration).
- Add new nodes for testing both in the migrate dump and in the kernel tests.
Remaining tasks
Commit :)
User interface changes
None.
API changes
The Drupal\node\Plugin\migrate\D7NodeDeriver plugin gets a new boolean $translations argument for its constructor to derive translation sources as well (same change as with D6NodeDeriver earlier).
The Drupal\node\Plugin\migrate\source\d7\D7Node migrate source plugin also takes a module handler in its constructor, so it can migrate source language information on translations when content translation module is enabled on Drupal 8 (same as with the D6 node source plugin earlier)
MigrateFieldInstanceTest::assertEntity() gets a new required argument at the end to assert for translatability.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#104 | 2669964-104.patch | 43 KB | Gábor Hojtsy |
#98 | 2669964-98.patch | 43.05 KB | Gábor Hojtsy |
#98 | interdiff.txt | 606 bytes | Gábor Hojtsy |
#96 | interdiff.txt | 838 bytes | quietone |
#95 | 2669964-95.patch | 43.06 KB | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedComment #3
quietone CreditAttribution: quietone as a volunteer commentedComment #5
mikeryanThis is dependent (at the very least) on the entity destination support for adding translations which is included in #2225775: Migrate Drupal 6 core node translation to Drupal 8.
Comment #6
mikeryanComment #7
Gábor HojtsySince the title said i18n module, I assume this is about Drupal 7 core translation. The technology for that should be the same as #2225775: Migrate Drupal 6 core node translation to Drupal 8. I don't think we need to solve the entity_translation issue here, but we need one opened for that too. As per #2225775-188: Migrate Drupal 6 core node translation to Drupal 8 we don't have such an issue yet.
Comment #8
Gábor HojtsyComment #9
quietone CreditAttribution: quietone as a volunteer commentedA first step is to add translations and perhaps another language to the test fixture. But an error is given when trying to enable multilingual support on the article, page or test content type. The error is in 'An illegal choice has been detected error' and that is on the Default parent item. Also, when editing content the 'Menu settings' option is not displayed even though the admin permissions are correct.
So, it does appear that something is amiss with the menus. If that is indeed the case, that should be addressed in a new issue.
Comment #10
Gábor Hojtsy@quietone: did you experiment with that test? it sounds like from your report. It would be nice if you could post it so we see those fails you are talking about :) Thanks!
Comment #11
quietone CreditAttribution: quietone as a volunteer commented@Gábor Hojtsy, sure.
I tried various combinations of adding a new content type and new menus and changing the 'Default parent item'. All attempts result in the error.
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedFound the case. There is a D7 variable, 'menu_override_parent_selector' which is set to true. Setting it to false allows the content types to be modified via the UI. Note that MigrateMenuSettingsTest will need to be changed as well.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedAdded one translation for node/3 and the resulting patch is 1.3M.
Sigh.
Comment #14
Gábor Hojtsy@quietone: would it make sense to post that in an issue and get committed so we can focus here on the logic patch itself?
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedAs suggested a new issue has been created, #2785241: Add a translated node to d7_dump.
Comment #16
Gábor HojtsyMarking postponed then.
Comment #17
Gábor HojtsyWell, I guess reading that issue its true that only the testing of this is postponed on that, so we don't need to postpone this in fact.
Comment #18
Gábor Hojtsy#2785241: Add a translated node to d7_dump landed. I think this needs a yaml file similar to the D6 yaml file and a test similar to the D6 test, since the data structures are the same for translations.
Comment #20
quietone CreditAttribution: quietone as a volunteer commentedOk, let's make some progress here. I've pretty much copy/pasted the D6 node translation code to D7. Got most of the new/modified tests working except D7/MigrateNodeTest where the Icelandic translation is saved to the English node. I'm going to park the issue for a while and in the meantime, let's see what else testbot finds.
Comment #22
Gábor HojtsyTwo minor things fixed: D6 to D7 in the d7 code and a missing newline. Fixing these directly to avoid making noise about them :) No other changes.
Comment #24
Gábor Hojtsy@quietone: several fails in MigrateUpgrade7Test too.
Comment #25
quietone CreditAttribution: quietone as a volunteer commentedThis adds d7_node_translation to the migrate_drupal_ui test.
Comment #27
quietone CreditAttribution: quietone as a volunteer commentedOops, typed in the wrong screen.
Comment #29
quietone CreditAttribution: quietone as a volunteer commentedJust a list of observations and questions:
I think I'll bring up a fresh D7, create some translations, and get a better understanding of the field tables. And double check the fixture, esp for the translatable column.
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedAdded the changes to EntityConfigBase.php and EntityFieldStorageConfig.php from #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields.
Comment #32
quietone CreditAttribution: quietone as a volunteer commentedAnd finally, update the entity counts.
Comment #33
penyaskitoI didn't know about the details, but installed a D7 site and played around it. I couldn't find any usage of that one in Content translation module in core, and if you set up entity translation it sets that up to true if there is a field marked as translatable with entity translation.
So I'm guessing this was added in Drupal core for allowing contrib modules as entity translation to allow this option.
Git annotate on the field.install schema method doesn't provide useful information for when and why this column was added.
Comment #34
penyaskitoI would say that, as content translation creates different nodes for each translation, if a content type is set as "content translation translatable" in D7, we should make all field instances (and their storages) translatable. And by all I mean all, not the D8 assumed defaults. E.g. we would need to create copies of file in images fields, as they may be different between different translations. I think we cannot do better than this.
Comment #35
penyaskitoFrom the translations point of view, the source code makes sense to me. Someone with more exposure to the APIs should check it though.
I haven't tested it yet.
And probably needs to wait for #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields to be committed, but don't want to mark this postponed yet.
About #33 and #34, I'm sure we should do that, but may not be the right issue. Fields, field instances and their settings should be migrated first, as stated:
Are we handling that in a different issue?
Comment #36
Gábor Hojtsy@penyaskito: why would this be postponed on #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields? This is a pure core-core data migration while that migrates a contrib module's translation data to core. Why would that be a pre-requisite?
Comment #37
quietone CreditAttribution: quietone as a volunteer commented@Gábor Hojtsy, because the changes to the destination plugins, EntityConfigBase.php and EntityFieldStorageConfig.php in #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields were added to this in #30.
And note that the points raised in #34 and #35 still need to be addressed.
Comment #38
Gábor HojtsyAll, right why do we need config translation coupled in here for our content translation then? These are not dependent on each other in Drupal 8 and Drupal 7 did not even have ways to translate anything related to nodes that I remember. (It could still be that my memory is short mind you :) What makes this require config translation support?
Comment #39
quietone CreditAttribution: quietone as a volunteer commentedIn D7 the body is a field and thus in D8 uses config for storage and field info.
Comment #40
Gábor HojtsyOk, I am afraid that is still not enough information for me to tell why we need this. In which case would the node body field be saved as a translation of the body field? Where would its langcode come from to be identified as a translation? What I missing from the patch?
Comment #41
quietone CreditAttribution: quietone as a volunteer commentedYes. And further for all content type the storage for all fields must be created with translatable true. Right now the D7 translatable value (from field_config) is used for both the storage and the instance. Presumably the field destination plugin could do this work.
@Gábor Hojtsy, the translation information is in node__body. Does that answer your question?
>select * from node__body;
Comment #42
Gábor HojtsyBut making the field instances or field storages translatable (resides in their respective original YAML files) does not require translating the configuration YAML files themselves, in fact the translations are not supposed to override anything about the translatability settings of the field storage or field instance configuration themselves. I think what you are referring to is that the field instance/storage config should configure the field to be translatable, not that the field instance/storage ITSELF should be translatable, which is not a requirement at all in Drupal 8 for content translation. You can enable content translation and never have config translation and still do all the content translation you want (much more than in Drupal 7). Therefore my lack of understanding why we need support for translation of the config itself in this patch. Drupal 8 does not require the translatability of the config for said config to configure a field as translatable, these are in two different subsystems.
Comment #43
phenaproximaComment #44
quietone CreditAttribution: quietone as a volunteer commentedThis needs to be rerolled due #2807917: Convert Node's Migrate source tests to new base class. I chose to start again from the patch in #27, thus removing the changes to EntityConfigBase.php and EntityFieldStorageConfig.php. The updated files are the d7 tests, NodeTest.php and NodeTranslationTest.php.
Comment #47
quietone CreditAttribution: quietone as a volunteer commentedNow, force the field storage to have translatable true and update the entity count in the migrate_drupal_ui MigrateUpgrade7Test. This works locally on the MigrateNode test, but lets see what the testbot finds.
Comment #49
Gábor HojtsyWe discussed this on the migrate meeting. Indeed, the field translatability settings should be entirely ignored for node types that use core node translation in Drupal 7, because Drupal 7's core node translation just stores entirely separate copies of the node, so all fields may be translated. It is fine to force all the fields migrated to be set to be translatable.
The catch is in Drupal 7 core translatability vs. entity translation (contrib module) translatability may be different per content type. In core, http://cgit.drupalcode.org/drupal/tree/modules/translation/translation.m... decides which ones are core supported. So this variable should be inspected and this migration should only be run for node types where this would return TRUE :)
If this would return TRUE then the migration in this patch should apply to nodes of that type and the field translatability settings in Drupal 7's field settings should be ignored entirely and migrated to translatable in Drupal 8 instead. If this would return FALSE for a node, then that node should not be attempted to be migrated with this migration, it should not have translations using the core method this patch supports.
Comment #50
quietone CreditAttribution: quietone as a volunteer commented@Gábor Hojtsy, thanks, that really helped
So, this patch modifies the field and field instance migrations. The field migration no longer migrates 'translatable' so that the default D8 value of true is used. This alone is sufficient for the MigrateNodeTest to pass. But translatable for the field instances should be set correctly. The field instance migration will migrate the translatable value from the field_config table to the field instance, unless it is a node. In that case translatable is forced to true in fieldInstance::prepareRow. Tests updated as well.
I think this would result in data loss. language_content_type_ appears to be just a flag indicating if this node type is translatable or not. The translatability can be disabled after creating translated nodes. To be sure, I just tried this using the d7 dump database. Setting multilingual support to 'disabled', changes language_content_type to "0" and, as expected, the node and the icelandic translation are still there.
Comment #52
quietone CreditAttribution: quietone as a volunteer commentedLooks like I wasn't working from HEAD. :-( Just needed to update d7_node.yml and d7_node_translation.yml
Comment #53
Gábor HojtsyYay, great progress, some mostly minor / docs findings:
Move the top comment into the condition IMHO and document the else clause too, so its clear what are the two paths someone may get translatable.
Why is this the provider if d7_node was not provided by migrate_drupal? (Both are in node module)
Empty line lost between these.
This still implements the interface, we can/should leave as inheritdoc IMHO.
Can we assert some data (title, body to cover a base and a configurable field) about the translation too?
Comment #54
quietone CreditAttribution: quietone as a volunteer commented1. Fixed. (I wish I could find an example in the coding standards for this).
2. Is this related to #2560795: Source plugins have a hidden dependency on migrate_drupal. And since that is fixed it can be removed from d7_node_translation.yml and d6_node_translation.yml?
3. Fixed
4. Removed.
5. Tests added, but not for a field. Is that getting out of scope? The D6 core node translation didn't do any checking on fields.
And still to do:
I added the test from the D6 version which gets the source language of the translation and checks that it is what is expected. For D7 the source language returned is 'und' instead of the expected 'en'.
Comment #55
Gábor Hojtsy@quietone:
re #54/2 I don't know, it is odd though that d7_node does not need it, why would this one need it?
re #54/5 While the D6 migration may not have tested fields, D7 fields are more complicated (as we have seen here :D), so testing the title and body IMHO would be nice to cover a sample base field and a sample configurable field
re #54/+1 does that have something to do with #2746293: Migrate content_translation_source when migrating node translations?
Comment #56
quietone CreditAttribution: quietone as a volunteer commented@Gábor Hojtsy, yes I can confirm that copy/pasting code from #2746293: Migrate content_translation_source when migrating node translations fixes this. And still to add from that patch is a test that content_translation_source for a source other than English is correct. That will involve modifying the dump, which is not something I look forward to, so I'll take a long break first.
Comment #57
quietone CreditAttribution: quietone as a volunteer commentedForgot to change the status.
Comment #58
quietone CreditAttribution: quietone as a volunteer commentedThis adds a new node to the dump with language Icelandic, then an English translation of that node and tests the source language of that translation.
Comment #60
quietone CreditAttribution: quietone as a volunteer commentedRemove changes to unnecessary changes to tracker_node table in the dump.
Comment #61
Gábor HojtsyYay. So I thin #54/5 is the only remaining piece? Or am I missing something?
Comment #62
quietone CreditAttribution: quietone as a volunteer commentedI don't know if something is missing. Assertions exist on the body and title of the translated node. But I'm not really sure what exactly is meant by a configurable field and 'non configurable field. If another field needs to be added, what type? That node type has title, body, tags, image and link fields.
Comment #63
mikeryantranslatble?
Comment #64
Gábor HojtsyOh, how did I miss all the assertions on the Icelandic translation? Those were the ones I thought were missing :)
So this looks complete then.
Comment #65
Gábor HojtsyFixing the typo Mike found. And with that I think this is now RTBC :)
Comment #66
heddn+1 on RTBC.
Comment #68
Gábor HojtsyRandom unrelated fail.
Comment #70
Gábor HojtsyDoes not apply anymore unfortunately.
Comment #71
quietone CreditAttribution: quietone as a volunteer commentedRerolled.
Comment #72
Gábor HojtsyYay, thanks! Still looks good.
Comment #73
Gábor HojtsyFully updated issue summary to help reviewers.
Comment #74
Gábor HojtsyAlso removing two db dump snippets that I found to be extra in the review I did for updating the issue summary. Data on node visits and two new locale strings. Neither has anything to with the node translation migration. The update is cosmetic so keeping at RTBC.
Comment #75
Gábor HojtsyComment #76
alexpottFrom \Drupal\field\Plugin\migrate\source\d7\FieldInstance::query() - I don't see why we need to add the fc.data field twice... that's just confusing - no? If we do then a comment is necessary. I think that we could just do:
$data = unserialize($row->getSourceProperty('field_data'));
If we remove data_fc I think this needs changing to
'field_data'
.Comment #77
quietone CreditAttribution: quietone as a volunteer commentedThis patch addresses issues in #76.
Not sure why the interdiff includes this error, "interdiff impossible; taking evasive action".
Comment #78
phenaproximaI found nothing but nitpicks with this patch. Looks great!
What does 2 signify? There should be a comment about that. Better yet, let's use an interface constant if available.
This should be assertSame().
The vid field is not in the process pipeline...
I'd prefer to use migration_tags instead of hard-coding the migration ID.
Nit: Should be [].
Should be ModuleHandlerInterface.
This should be ModuleHandlerInterface.
Nit: There should be an extra blank line here.
I'm just now noticing this, but $modules should have an @inheritdoc comment.
Let's assert against $node->label() instead.
Use $translation->label()
Let's kill these comments in arrays where there is no mixing of node and node revision fields.
Comment #79
masipila CreditAttribution: masipila commentedHi,
Comment #7 clearly states that this issue will NOT contain migration path for D7 contrib entity_translation and its field translations. I perfectly understand the rationale for this since field level translations is a complete different story than the core translations.
According to #7, there was no issue for migrating entity_translation field translations to D8 six months ago. I've been trying to search one but haven't found yet. Could you guys who are familiar with the existing i18n migrate issues point me to the right issue if one exists already?
Cheers,
Markus
Comment #80
Gábor Hojtsy@masipila: I think #2073467: Migrate Drupal 7 Entity Translation settings to Drupal 8 is the issue for that. I think it may be feasible to move that issue to core, since other contrib things that moved to core have their migration path in core, but I'm not the expert in placement of migrations :)
Comment #81
masipila CreditAttribution: masipila commentedThanks!
Comment #82
quietone CreditAttribution: quietone as a volunteer commentedFixed nits in #78. However, not sure the modified comment is sufficient for #1. And not sure what is meant by using migration tags in #4. Note this same code is used in the D6NodeDeriver.
Comment #83
jofitz CreditAttribution: jofitz at ComputerMinds commentedExpanded on the comment addressing #78.1 - hopefully that helps explain what the "2" means. Unfortunately there is not a relevant interface constant.
Comment #84
phenaproximaI don't think we need to mention "Drupal 7 Multilingual Support option 'Enabled, with translation'" here -- it's confusing. Let's just mention that 2 is a Drupal 7 constant that signifies that translation is enabled for this bundle.
It also doesn't look like #78.3 was fixed, according to the interdiff...
Comment #85
Gábor Hojtsy@phenaproxima I don't necessarily agree that it is superfluous to add that given there are different translation options (if entity translation is also enabled). I think there were still outstanding things in #82, so moving to needs review for that.
Comment #86
Gábor Hojtsy83 did not apply anymore to D7NodeDeriver, so here is a reroll.
Comment #87
Gábor HojtsyAs per the migrate meeting, @phenaproxima suggested we have a list of the potential values for that constant instead. Here it goes.
Comment #88
Gábor Hojtsy@phenaproxima: re #78.3, I think that was resolved in #82, just look at the patch itself. The interdiff appears to be rolling back the file entirely but the patch has the file as a copy/rename of another file and has limited changes there. I guess git decided to optimize that diff :D
So I think #78.4 and the response to it in #82 are the only thing still to be done?
Comment #91
Gábor HojtsyDuh, I rolled against 8.3, of course it did not apply to 8.3.x. Should this be against 8.2 still primarily though?
Comment #92
quietone CreditAttribution: quietone as a volunteer commentedI've added Gábor Hojtsy changes in #87 to the patch in #83 to test against 8.2.x.
Yes, #78.4 still needs to be done.
Comment #93
quietone CreditAttribution: quietone as a volunteer commentedHow is this for #78.4?
Comment #95
quietone CreditAttribution: quietone as a volunteer commentedFix typo.
Ignore this interdiff, the correct one is in the next comment.
Comment #96
quietone CreditAttribution: quietone as a volunteer commentedThis is the correct interdiff for the typo fix.
Comment #97
phenaproximaI looked it over and nothing problematic leaps out at me. All makes sense, and seems pretty nicely tested. Except this one tiny thing:
Nit: There's an extra blank line here. Can be fixed on commit.
Comment #98
Gábor HojtsyThat's too easy ;)
Comment #100
catchWith the tnid migrated to nid, what happens with node references/entity references that point to the old nid? Does this need a follow-up? Or should it be addressed here (assuming it's not).
I also opened another follow-up here #2850085: Redirects for translation set migration path in Drupal 6 and 7 - doesn't need fixing with this issue but don't think I've seen it covered elsewhere.
Comment #101
Gábor Hojtsy@catch: Drupal 6 had the same problem, we have #2746527: [META] Handle data related to Drupal 6 and 7 node translations with different IDs as a META for that, given that path aliases, comments posted on nodes, node ratings, nodequeues, whatnot would all need to react on this migration (additionally to entity references that you mentioned). We can make that meta apply to both Drupal 6 and 7 or open another one for 7.
Path aliases in particular have been resolved in Drupal 6 with #2827644: Fix path alias migration of translated nodes [D6] and would be very similar for Drupal 7. I think we don't need to open every possible followup for the changed IDs because we don't know the extent of the problem and it applies to contrib migrations as well. I can expand #2746527: [META] Handle data related to Drupal 6 and 7 node translations with different IDs as a meta for both Drupal 6 and 7.
Comment #102
Gábor HojtsyUpdated #2746527: [META] Handle data related to Drupal 6 and 7 node translations with different IDs to deal with both Drupal 6 and 7 and @catch moved it to critical.
Comment #103
catchNeeds a re-roll for 8.4.x:
error: patch failed: core/modules/node/src/Plugin/migrate/D7NodeDeriver.php:65
error: core/modules/node/src/Plugin/migrate/D7NodeDeriver.php: patch does not apply
Comment #104
Gábor HojtsyStraight reroll against 8.4.x. It was just a comment moved and updated in 8.4.x that made this not apply.
Comment #107
catchCommitted/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!
Comment #108
xjmLet's mention this one in the release notes/changelog since it's not shipping in any patch release and it is a big deal.
Comment #110
Gábor HojtsyAnyone interested in the next chapter at #2073467: Migrate Drupal 7 Entity Translation settings to Drupal 8? :)