Problem/Motivation
In #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields we set up a system to migrate config translations. We also migrated translations for user profile fields and their descriptions. We did not migrate field options (eg. select boxes or radio buttons or checkboxes) translations.
While working on the patch and creating the test fixture, in particular, it was discovered that only the select fields can have value translations. Therefor, this is limited to select fields.
Proposed resolution
Migrate user profile field options translations.
Remaining tasks
- Add data to test fixture . See generating database fixtures for d8 migrate tests
- Write the migration test
- Write a source plugin and source plugin test
- Write the migration
.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#56 | interdiff-52-56.txt | 1.64 KB | quietone |
#56 | 2845975.patch | 16.27 KB | quietone |
#52 | interdiff-50-52.txt | 675 bytes | quietone |
#52 | 2845975-52.patch | 16.4 KB | quietone |
#50 | interdiff-48-50.txt | 597 bytes | quietone |
Comments
Comment #2
Gábor HojtsyComment #4
quietone CreditAttribution: quietone as a volunteer commented#2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields is in, no loner postponed.
Comment #5
mikeryanComment #6
rakesh.gectcrStill not clear on, what will be the first approach here. Working on it. Any guidance will help
Comment #7
Gábor HojtsyI think this would use the regular config translation migrations, see migrations for vocabularies, user profile fields themselves, etc. The most recent one is #2225781: Migrate D6 i18n taxonomy vocabularies.
Comment #8
quietone CreditAttribution: quietone as a volunteer commented@rakesh.gectcr, thanks for taking this on.
AFAIKT, the d6 test fixture does not include a user profile select field. So the first steps are to add the data to the fixture and add the test. There is a handbook page for generating database fixtures for d8 migrate tests. As usual, please improve that page, it is pretty basic. For the test, the direction provided in #7 looks good. This will need a new source plugin, so that and it's test would be next. Then the migration itself.
I worked on the test fixture a bit. I added a select field to the user profile. But I can't find where/how to translate it. The new field doesn't even appear on the Translate interface and I'm not sure how to translate the select values. Can someone explain these steps?
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedHere is a start. This doesn't include the changes to the fixture because I don't have time to clean it up now. So, there is no point in running the tests until that is done.
This will also need to be postponed on #2961114: Migrate D6 i18n CCK field option translations because I'm sure it needs the destination changes but I am setting to NW until I add the changes to the test fixture.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedMeant to unassign rakesh.gectcr, it has been over a year.
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedAdd in the changed to the test fixture.
Now postponing until #2961114: Migrate D6 i18n CCK field option translations is committed.
Comment #15
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 #16
quietone CreditAttribution: quietone as a volunteer commentedNo longer postponed. Testing patch in #14.
Comment #18
quietone CreditAttribution: quietone as a volunteer commentedNot surprised that it needs a reroll.
Comment #19
aleevasRe-rolled https://www.drupal.org/project/drupal/issues/2845975#comment-12753681
Comment #21
quietone CreditAttribution: quietone as a volunteer commented@aleevas, wrong patch?
Comment #22
jofitz CreditAttribution: jofitz at jofitz commentedRe-rolled.
Comment #24
jofitz CreditAttribution: jofitz at jofitz commentedFixed coding standards errors.
Leaving as Needs Work to fix test failures.
Comment #25
quietone CreditAttribution: quietone as a volunteer commentedThis took a while. I think there were errors in patch #14 which wasn't helping at all.
Now, lets see if that fixes everything.
Comment #27
quietone CreditAttribution: quietone as a volunteer commentedAdded a conditional to the source plugin, d6_profile_field_translation, because the test fixture introduces a profile field that has does not have a translation for the title but does for this options. This meant the query was returning a row where language was NULL.
Took out a lot of changes in the test fixture which are not needed. At least I hope not.
And fixed the translation values, it should not have a value of 30.
Comment #29
quietone CreditAttribution: quietone as a volunteer commentedFix formatting in yml file and update the entity counts.
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedThis is now ready for review. Thanks!
Comment #31
maxocub CreditAttribution: maxocub commentedAs always, great work with those tricky i18n issues.
Unused constant.
According to https://www.drupal.org/docs/develop/standards/api-documentation-and-comm..., {@inheritdoc} should be the only line in the docblock.
Do we need this empty default clause?
This can be removed.
Comment #32
quietone CreditAttribution: quietone as a volunteer commentedFirst a reroll.
Comment #33
quietone CreditAttribution: quietone as a volunteer commentedAnd fixes for all the items in #31.
Comment #35
quietone CreditAttribution: quietone as a volunteer commentedMessed up the test fixture in the reroll. This should fix it.
Comment #38
quietone CreditAttribution: quietone as a volunteer commentedI really dislike rerolling the test fixtures.
Maybe this will fix it.
Comment #39
quietone CreditAttribution: quietone as a volunteer commentedFinally, test are passing again! Everything in #31 has been addressed so this is ready for review again,
Comment #40
maxocub CreditAttribution: maxocub commentedThe patch looks quite ready, but I still have two questions and one nit :
Because there's no "bypass: true", the values in the "static_map" process plugin are the only field types that can be passed to the "d6_profile_field_option_translation" process plugin. And in the switch statement of this last process plugin, we have cases for "list_string", "list_integer" & "list_float", but no default case. This makes me think that either there is some some missing values in the "static_map" (list_integer & list_float) and some extra ones too (anything that's not list_*) or either we need a default case in the switch statement.
Since this patch is dealing with option fields, I think the possible values in the static map should only be list_* fields, but maybe I'm missing something...
Are those lines removed for a reason or by accident? I can't figure out why they would need (or not) to get removed.
Nit: This line can be removed.
Comment #41
quietone CreditAttribution: quietone as a volunteer commentedJust fixing the nit now (time to listen to the DrupalSouth session!).
Comment #42
quietone CreditAttribution: quietone as a volunteer commented40.1. That static_map is copied (like most of that migration) from user_profile_field.yml. Having said that, I think (maybe) the source plugin should extend from ProfileField.
40.2. To fix this the following needs to be restored to the fixture, which will be easier when #3016011: Reroll all migrate dump files is committed.
- 'lid' => '1672',
- 'translation' => 'fr - Type',
- 'language' => 'fr',
- 'plid' => '0',
- 'plural' => '0',
- 'i18n_status' => '0',
-))
Comment #43
quietone CreditAttribution: quietone as a volunteer commented40.1 Removed unused items from static map as suggested. Also, changed the source plugin to extend from ProfileField.
40.2 Restored the accidentally removed block from the test fixture.
Comment #45
quietone CreditAttribution: quietone as a volunteer commentedThe NW is because I named the interdiff a patch (again). This is really ready for review, all the points raised in #40 have been addressed.
Comment #46
heddnComment #47
heddnTriaging the issue queue.
Comment #48
quietone CreditAttribution: quietone as a volunteer commentedThis is the reroll.
Comment #49
vacho CreditAttribution: vacho at Skilld commentedActual patch can apply.
Comment #50
quietone CreditAttribution: quietone as a volunteer commentedThis needed a reroll.
Comment #52
quietone CreditAttribution: quietone as a volunteer commentedArg, a stupid typo.
Comment #53
quietone CreditAttribution: quietone as a volunteer commentedNR for testbot
Comment #54
quietone CreditAttribution: quietone as a volunteer commentedRemove postpone from the IS.
This is ready for review.
Comment #55
maxocub CreditAttribution: maxocub commentedI tested the patch manually and it worked as expected, the option translations from the Profile select fields were migrated properly.
But, the IS should be updated because it is misleading. It says that radio button and checkbox field translation should be migrated too in this issue, but they are not. I've checked on Drupal 6 what type of fields needs their option translation migrated and it seem that it is only the select boxes, as this patch does.
As for the code :
I'm still bugging over this static map and switch statement mismatch. The only field type that we need is the list_string, as we already have in the static map. So, we should not need the switch statement for list_integer and list_float, I think a simple if statement that checks if the field type is list_string should be enough.
A lonely @inheritdoc that has lost its method...
Comment #56
quietone CreditAttribution: quietone as a volunteer commentedFixes for #55.1 and #55.2. Still need to update the IS>
Comment #57
quietone CreditAttribution: quietone as a volunteer commentedUpdated IS and tried to keep some of the history.
Comment #58
quietone CreditAttribution: quietone as a volunteer commented@maxocub thanks for the review!
Comment #59
maxocub CreditAttribution: maxocub as a volunteer commentedAll feedbacks are addressed. Manual and automated tests are positive. IS has been updated. Let's RTBC this.
Comment #61
quietone CreditAttribution: quietone as a volunteer commentedFailure looked unrelated, so retesting
Comment #62
quietone CreditAttribution: quietone as a volunteer commentedTests passing again. There has been no changes to the patch since the RTBC so I am restoring the RTBC.
Comment #63
catchCommitted and pushed 75a4213161 to 8.7.x and cdd317ebc6 to 8.6.x. Thanks!