Problem/Motivation
User profile fields in Drupal 6 are translated using the i18nprofile submodule of the i18n suite. We need to migrate these translations.
Proposed resolution
1. Establish a system to migrate configuration translations since this is the first issue that needs it. Drupal\migrate\Plugin\migrate\destination\EntityConfigBase is changed to take the language manager and the config factory in its constructor. Change import() and updateEntity() to support translations. Expand rollback() to support for rolling back config translations.
2. User profile field translations are stored within i18n_strings and locales_target tables connected by the lid key on Drupal 6. These profile values are fields in the user entity on Drupal 8. Migrate the title and description of the fields to Drupal 8.
3. Add tests.
Remaining tasks
Migrate field options translations (if field options themselves are migrated). This is to be handled in #2845975: Migrate Drupal 6 user profile field value option translations.
User interface changes
None.
API changes
Constructor of Drupal\migrate\Plugin\migrate\destination\EntityConfigBase gets two new arguments. As per https://www.drupal.org/core/d8-bc-policy the constructor of a plugin is considered internal and thus does not explicitly count as an API change.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#68 | 2225717-68.patch | 27.11 KB | jofitz |
#68 | interdiff_63-68.txt | 937 bytes | jofitz |
#63 | interdiff.txt | 2.78 KB | quietone |
#63 | 2225717-63.patch | 27.1 KB | quietone |
#57 | interdiff.txt | 1.97 KB | quietone |
Comments
Comment #1
phenaproximaComment #2
phenaproximaBlocked by #2594263: Add translation data to Migrate Drupal's test fixtures.
Comment #3
svendecabooterUnblocked
Comment #4
svendecabooterComment #5
quietone CreditAttribution: quietone commentedSo, how does this work in D6? I've enabled the modules, installed languages and user profiles don't translate. Not any better on D8, there I get WSOD. Nor can I find instructions, so I guess this should be straight forward.
Comment #6
penyaskitoAre D6 profiles fieldable? We may need to migrate those fields and the fields provided by the module first to the user entity. Then we can migrate the data from there using the user as destination.
Does this make sense?
Comment #7
quietone CreditAttribution: quietone commentedYes and no. I'm not even thinking about that yet because my starting point is to create D6 profile data with translations. I've tried without success. How does one make a profile data that is translated? What are the steps?
Comment #8
Gábor HojtsyComment #9
Gábor HojtsyWhen enabling the profile translation in i18n, it says "You must enable the Profile, String translation, Locale modules to install Profile translation.". I went ahed with those. Added a new "Home page" text profile field at
admin/user/profile
(in the Home page category for simplicity). Now I get that category as a tab at user/1/edit/Homepage and I get the category title and the field title atadmin/build/translate/search
when filtering to profile fields:(So its not just the fields but also the categories that would need to be supported).
Note that its not the actual field values that are translated, the field title, explanation, options (in case of select options) and the category labels are. Not the values. Similar to field translation in i18n where it translates the field settings, not the field values. Hope this helps.
Comment #10
Gábor HojtsyComment #11
quietone CreditAttribution: quietone commentedAh, now it makes sense.
And I somehow missed (several times) the profile option at the bottom of admin/build/translate/search.
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedA migration template and source plugin for D6.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedRemoved some copy/paste errors and added source plugin test.
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedComment #15
quietone CreditAttribution: quietone as a volunteer commentedAnother iteration. Added static map to the template, adjusted the destination plugins to suit and added tests for all the user profile fields. Note this requires the data from #2670170: Add i18n string & variable data to d6_dump.
While the changes to the destination plugins work, I'm not sure if this is the correct/best way to accomplish this migration. And the other i18n_string migrations haven't been written yet, so these changes may not work for those.
Also, should this migration be in the config_translation module or somewhere else?
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedRunning non translation migrations with the above patch fail. Tracked this down to the fact that container->get(language_manager) returns a non configurable language interface when config_translation is not installed. So, in this patch EntityConfigBase checks the instance of the language manager service, so that, if it is configurable and the row has a langcode property, then the row is imported as translation data.
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedAdd the testing data #2670170: Add i18n string & variable data to d6_dump so the testbot can check this.
Comment #18
quietone CreditAttribution: quietone as a volunteer commentedSince that passed tests, marking this as postponed on #2670170: Add i18n string & variable data to d6_dump
Comment #19
quietone CreditAttribution: quietone as a volunteer commentedReroll.
Comment #20
Kristen PolThanks for the patch. Here are some minor nitpicks:
Should this be
MigrateI18NUserProfileFieldInstanceTest
? (capitalize "I18N") ori18nMigrateUserProfileFieldInstanceTest
?It's a bit odd that some of the values are actually in French while others are like "fr - some English text". Is there some reason for this? It would be good to use one or the other. My preference is to use the latter (English text) since I don't understand French. :)
Add comment?
Add comment?
Add comment?
Add comment?
Add comment?
Add comment?
Add comment?
Add comment?
Needs better comment using a full sentence.
Comment #21
quietone CreditAttribution: quietone as a volunteer commentedI thought I responded to this weeks ago, seems I didn't.
1) The Naming conventions states
So it seems the name should be MigrateI18nUserProfileFieldInstanceTest and I18nProfileFieldTest.php.
2) The reason there is a mix of French and 'fr - english text' is because I had a French speaking friend visiting while I was making these changes. It seems appropriate to have the correct French when available. In this case, there is no need to understand the language as we are simply migrating text.
3 - 10) Not sure what you are asking.
11) Fixed.
Comment #22
Gábor HojtsyThanks for all your work on these issues, reviewed this one:
Needs to be redone to not use migration templates given the move away from them.
Also the other ones have d6 in their name, should this not? Hm, based on the code this does seem like both Drupal 6 and 7? Why not have both tags on it then?
... Or given this is called d6_i18n..., should it not try to deal with Drupal 7? :)
Should filter on i18n object type too no? Otherwise if some other object had the same name but a different object type, it would be included here.
The original config may also have a langcode theoretically. So it would be better to check if this langcode is different from the original langcode of the entity.
Comment #23
Gábor Hojtsy@quietone: are you still working on this?
Comment #24
quietone CreditAttribution: quietone as a volunteer commented1. Currently, all the migrations in core are in ./core/modules/*/migration_templates. I'm not sure this is the place to make a precedent. Maybe that needs an issue?
For now, this should be tagged for Drupal 6 only, because that is the priority. To add D7 the test data needs to be added to the dumps. There is already an issue for that, #2670846: Add i18n data to d7_dump.
2. Fixed.
3. True. But since this also has a join on locales.target.lid = i18nstrings.lid, where lid is a primary key, this shouldn't matter.
4. Added a test for the default language.
Comment #25
Gábor HojtsyThis is only true if we make assumptions about the source data. In Drupal 6, the source data is indeed assumed to be in the site default language and all others are translations. Since this is a destination plugin, I am not sure Drupal 6 assumptions should be baked in here.
Configuration in Drupal 8 may be created in whatever language and then translated to whatever other language. So what should be checked is the active config matches the language of this data or not. If it does not, then this is a translation (regardless of site default language).
The id should also start with d6, not?
Comment #26
quietone CreditAttribution: quietone as a volunteer commented1. Changed to getCurrentLanguage. Is that what you meant?
2. Yes, I missed that.
That's all for tonight.
Comment #27
Gábor HojtsyNo, we need to check against
And compare the langcode in the migrated data to that langcode there in the config.
Comment #29
quietone CreditAttribution: quietone as a volunteer commentedReworked EntityConfigBase.
Comment #31
quietone CreditAttribution: quietone as a volunteer commentedThere was trouble with my test setup so I couldn't run migrate tests before. The check on the language manager in EntityConfigBase is restored. And now that I fixed my setup, the tests run locally.
Comment #32
quietone CreditAttribution: quietone as a volunteer commentedRerolled to use the new test base class for source plugins.
Comment #33
phenaproximaComment #34
phenaproximaShould be
plugin: 'entity:field_config'
, since colons must be quoted in YAML.Really? Core profile didn't provide any i18n support, to my recollection. Shouldn't this maybe be one of the i18n suite of modules?
Nit: For readability, I'd prefer [] syntax instead of array() here.
Should the full translation really be a source identifier? That seems kind of weird to me.
Every instance of assertIdentical() in this function should be assertSame(). Can it also be broken up into "paragraphs" (maybe every time the language_manager service is called) for readability?
Use
$this->container->get()
, not\Drupal::service()
.Please move $databaseContents and $expectedResults into providerSource() to stay consistent with the other tests.
This and all other parameters should use the fully qualified class name.
All of this definitely needs test coverage.
There doesn't appear to be any reason why the config factory service cannot be injected into the constructor and used here.
Should be using the FQCN.
Comment #35
quietone CreditAttribution: quietone as a volunteer commentedThanks for the review.
1. Except that we don't escape any of the other destination plugin strings.
2. Yes, that is wrong. Changed to 'i18n' but this also depends on locales_target.
3. Fixed.
4. Ouch, fixed.
5. Fixed.
6. Fixed, with a twist. The language manager is now a variable. I think it is easier to read the test.
7. Fixed.
8. Fixed.
9. Still needs tests. Not sure where to start on it.
10. Fixed.
11. Fixed.
Comment #36
quietone CreditAttribution: quietone as a volunteer commentedJust noticed this which can be added later with the test.
Should be s/match/does not match/
Comment #37
Gábor HojtsyI asked about these changes on #2810347-32: [policy, no patch] Mark migrate.module as beta stability to make sure we are allowed to do this in some form, and which form :)
Not sure what is this doing :) We take $config but then don't use it and attempt to load a langcode top level config file (which does not exist). I proposed this logic in #27:
That would in fact take the langcode value of the config from the active config storage :)
I think the tricky part is that config may not exist also. So if the langcode of that does not match the langcode of the row, that does not yet mean we need to save a translation, only if the original config existed.
How do we ensure to only process config translations for config that exists already? (That would be the only scenario where this logic would work, otherwise I don't know if we could tell in this class if we are dealing with a translation or original config really).
Comment #38
quietone CreditAttribution: quietone as a volunteer commentedDid some testing and found that rollback does not work as expected for the i18n migrations. The rollback will delete the config entity not the translation of the config entity.
Comment #39
quietone CreditAttribution: quietone as a volunteer commentedThis i18n migration has dependencies on the user_profile_field and user_profile_field_instance so the config *should* exist.
If it does not a FieldException is thrown.
Comment #40
Gábor Hojtsy@quietone:
Well, it goes in the field config entity destination, so if that is rolled back, then the original ones would be? Should it use its own ids or a different destination to make rollback possible?
Superb, thanks! That also means that the config plugin in itself does not make sure this happens but it offloads that requirement to the migration itself. I guess that may be the case for other things as well, so not arguing the plugin should do something about it then :)
Comment #41
heddnAdding a tag. I did a quick review of the latest patch and it does seem to effect BC; and in a way that is fairly intrusive.
Comment #42
mikeryanBC break would definitely need a change record - that might help demonstrate how intrusive this is. Who in the contrib/custom space would be affected by this?
Comment #43
quietone CreditAttribution: quietone as a volunteer commentedSimilar to EntityContentBase, adding a language code to the destination is mostly working. The mostly is because the rollback count is incorrect. The rollback is deleting the translated field instance and not just the translated string from the field. But there is no information on the destination to indicate which string was translated. I guess that will have to be added.
Comment #44
Gábor HojtsyBy "which string was translated" do you mean which field had a translation saved? I don't think we need to know the string specifically, its enough to know that a config translation was created. Or do you want to manage rollback on a per string basis, so specific strings are removed from the config translation rather than that translation of that field removed?
Comment #45
quietone CreditAttribution: quietone as a volunteer commentedThese string translation migrations do one string per migration. For example. the user profile fields have a label and description field, and those are migrated one at a time, resulting in 2 rows in the map table. The rollback now rolls back one translation at a time, either the label or the description, but not both. I have a working version, though it is very specific to this migration. I want to explore other migrations that use the destination
entity:field_config
to learn more.Or, change the source plugin to get all translation strings at once.
Comment #46
Gábor HojtsyI think logically the migration looks at the source data as a unit, but I looked at the destination data as a unit :) Approaching from the source data makes more sense because there is no guesswork as to which items have translations, we don't need to check each of them one by one. Since a label may not have a corresponding description and a description does not necessarily have a corresponding label (although less likely), starting from the source data and coupling them up may be as hard...
So I think the per string approach may be fine and consistent with the rest of the system.
Comment #47
mikeryanSince this blocks beta stability for the Migrate API, setting Migrate critical.
Comment #48
quietone CreditAttribution: quietone as a volunteer commentedAdding rollback, with a test.
Comment #50
quietone CreditAttribution: quietone as a volunteer commentedRenamed the test file and implode the destination_identifier in EntityConfgBase::rollback.
Comment #51
mikeryanLooks like we've got our tests - still needs a change record, though.
I haven't given a full review, but just want to clarify what BC break(s) we have here - if I'm not mistaken, the only use case that would be affected would be extending EntityConfigBase or a class extending it and overriding __construct() and create(), correct? Nothing in core does this, and it's hard to conceive of a real-world use case, so as a practical matter I think the actual impact will likely be nil.
Comment #52
Gábor Hojtsy@mikeryan: my understanding was that rollback is still a problem with this approach(?) and that is the biggest outstanding problem.
Re whether we should consider this a BC break, I pointed out in #2225717-37: Add config translation support to migrations and implement for Drupal 6 user profile fields and asked in #2810347-32: [policy, no patch] Mark migrate.module as beta stability because it is not entirely clear-cut. I am not working on extending or providing services on top of migrate, so I don't know how severe/practically applicable this is. If we believe it is not, then we can remove the tag here and re-RTBC #2810347: [policy, no patch] Mark migrate.module as beta stability I guess.
Comment #53
mikeryanComment #54
mikeryanJust some minor points, then RTBC as far as I'm concerned.
Since the BC impact is minimal, removing migrate-critical tag.
Looks like we can drop this.
Condition seems wrapped extra snugly.
s/Conform/confirm/
s/Original/original/
Comment #55
Gábor HojtsyRe #52 we discussed on the migrate meeting that the rollback problem that was discussed earlier is now resolved in the patch. Will review again with multilingual features in mind.
Comment #56
quietone CreditAttribution: quietone as a volunteer commentedTested this by migrating the d6_dump to a D8 site that was installed in Spanish, a language not used in the d6_dump. Then I checked the translations of the user account settings. The english text has been stored as the original Spanish text instead of the English text. Hopefully, something simple like requiring the default_language migration.
Comment #57
quietone CreditAttribution: quietone as a volunteer commentedFixes for items from #54.
Comment #58
mikeryanPatch itself is RTBC - should still get a change record here, though.
Comment #59
Gábor HojtsyI promised to review this but did not get around to it earlier, sorry. Found two hopefully easy things to fix.
Should this just be simpler as
That would make this much readable. Also a little note as to what is this being done would be useful.
The logic is opposite of the two lines of comments? It is a translation if the two languages DONT match not if they match.
Comment #60
Gábor HojtsyComment #61
Gábor HojtsyUpdated issue summary significantly. Also added a change notice draft at https://www.drupal.org/node/2834360
Comment #62
mikeryanChange notice looks good to me. We should be back at RTBC with Gabor's comments applied.
Comment #63
quietone CreditAttribution: quietone as a volunteer commentedGábor Hojtsy, thanks for the review.
1. Oh yea, I meant to tidy that up. And changed a few related lines to use short array syntax.
2. Comment fixed.
Also, minor code standard fixes.
Comment #65
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-test passed, setting back to NR.
Comment #66
Gábor HojtsyI don't see the fix for #59/2 in the interdiff. Otherwise changes look good.
Comment #67
jofitz CreditAttribution: jofitz at ComputerMinds commentedI'll take a look...
Comment #68
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrect the logic in a comment.
Comment #69
Gábor HojtsyThanks, that resolves the concerns I had. Since otherwise Mike marked it RTBC and approved the change notice I wrote, back to RTBC.
Comment #71
quietone CreditAttribution: quietone as a volunteer commentedNo, it passed. Back to RTBC
Comment #72
Gábor HojtsyThis blocks all multilingual configuration migrations given its support code added for config translation migrations.
Comment #73
Gábor HojtsyUpdate title to be more honest about what is going on here :) Also opened the followup mentioned in the summary at #2845975: Migrate Drupal 6 user profile field value option translations and linked it in from there.
Comment #75
DamienMcKennaIs this just blocked by the number of hours in a day? :)
Comment #76
DamienMcKennaI cross-linked the issues that this one was blocking, so we'd be able to find them more easily.
Comment #77
Gábor Hojtsy@DamienMcKenna: yes, unfortunately Drupal is not quite advanced yet to expand the hours in a day.
Comment #78
xjm@DamienMcKenna, it is not helpful to post comments on RTBC issues asking why they have not been committed. All RTBC issues will receive a response from committers as soon as they have the availability.
Edit: also, such comments are actually disruptive to getting something committed, since they make an RTBC newer than it is. It's unfortunate, but avoiding unnecessary comments on RTBCs will actually help committers get to them sooner.
Comment #79
xjm@catch, @alexpott, @cilefen, and I discussed this issue just now and agreed that it would be great to get this into 8.3.x still, and that we would be okay committing it to 8.3.x between the beta and RC for sure (unlike most things) since Migrate API itself is also beta.
Tagging rc deadline for now; we debated whether it would be okay during a patch release for a beta experimental module and saw cases for both. We don't really have a precedent yet.
But this is still definitely committable to 8.3.x until February 28.
Thanks everyone for your interest and contributions on this issue!
Comment #80
catchI'd forgotten that we migrate 6/7 profile fields to the user entity. This doesn't seem right to me so I've opened #2851289: Revisit if/how to migrate profile module data to 8.x to discuss it again.
Doesn't affect this issue as such, I realise it unblocks other configuration translation migrations.
Comment #81
xjm(Screwed up the tag in a kind of important way; fixing.)
Comment #82
alexpottGiven we have the followup to discuss @catch's generic concern re profile fields I think we can proceed with this issue. The changes to migrating config entities where translations are involved all make sense and it is great to see good coverage of the rollback scenario. Nice work everyone.
Committed and pushed 6fc8b98 to 8.4.x and e9f78c5 to 8.3.x. Thanks!
Coding standards fixed on commit.
Comment #85
DamienMcKenna@xjm, @gabor, others: I am sincerely sorry for my impatience in #75 :-(
Comment #86
Gábor HojtsyYay, thanks everyone, especially @quietone! This unblocks so many other issues also :)
Comment #88
idebr CreditAttribution: idebr at iO commentedThe implementation requires the source plugin to list a single property at a time using a hardcoded
property
andtranslation
.This is confusing from DX point of view, since the data structure for content entity translations does save multiple properties at time. I created #3175477: Migration destination for configuration entity translations is opinionated towards i18n and can only migrate a single property at a time to allow saving multiple properties in a single migration row.