Problem/Motivation
Follow-up to #2657978: Variable to config: language_default [D6 & D7]. If we have a multilingual site and have not changed the default language, the lanuage_default variable is not set. When we migrate the site, there's an error in the migration log :
Source ID language_default: Input should be an array.
Proposed resolution
Use 'en' as a default value with the default_value process plugin.
Remaining tasks
Write a patch. Add test. Review.
User interface changes
There won't be an error in the migration log.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#12 | interdiff-10-12.txt | 1.38 KB | maxocub |
#12 | 2806285-12.patch | 4.56 KB | maxocub |
#10 | interdiff-9-10.txt | 896 bytes | maxocub |
#10 | 2806285-10.patch | 4.47 KB | maxocub |
#9 | interdiff-6-9.txt | 4.83 KB | maxocub |
Comments
Comment #2
maxocub CreditAttribution: maxocub commentedComment #3
maxocub CreditAttribution: maxocub commentedComment #5
Gábor HojtsyWhy do you still have the variables key here given the plugin does not use it anymore?
Any reason not to extend from the Variable plugin directly? Too messy?
Comment #6
maxocub CreditAttribution: maxocub commentedRe #5:
Comment #7
Gábor HojtsyOk, I trust that you considered extending the variable plugin and this was cleaner. The implementation and tests look fine.
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedThx maxocub. Can you say why the source plugin isn't extended from Variable? To me that would be cleaner. Either way, the new source plugin needs tests.
I know this was RTBC but I've had to rework patches to not use assertIdentical and assertEqual. Kinda confused by the inconsistency of that.
Comment #9
maxocub CreditAttribution: maxocub commented@quietone: Thanks for the review, those are valid points. First off, me too I had to rework patches to not use assertIdentical and assertEqual, so I converted those to assertSame.
That was the easy part.
For the rest, here's the short story:
Now the long story:
What do you think?
Comment #10
maxocub CreditAttribution: maxocub commented@phenaproxima had the excellent idea that we use \Drupal\Component\Serialization\Json::encode & decode, thus not needing the get_object_vars callback anymore. Here's the updated patch.
Comment #11
quietone CreditAttribution: quietone as a volunteer commentedWow, nice record of your journey on this patch. And neat way to avoid a new source plugin and associated tests.
There is a comment on the callback handbook page that additional arguments can be passed to the callback as this would make the migration YAML file too complex. Which you too discovered.
I agree with phenaproxima's comment about the tests being unusual.
Comment #12
maxocub CreditAttribution: maxocub commentedSome comments improvements suggested by @phenaproxima on IRC.
Comment #13
Gábor HojtsyWoah, nice trick, and thanks for documenting how you got there. Let's get this in then :)
Comment #15
maxocub CreditAttribution: maxocub commentedThat looks like random fail. Let's re-roll.
Comment #16
Gábor HojtsyUnrelated fail in Node.Drupal\node\Tests\NodeTypeTranslationTest.
Comment #18
maxocub CreditAttribution: maxocub commentedAnother unrelated fail...
Comment #19
Gábor HojtsyComment #20
catchIsn't there a skip_on_empty option? This looks like a clever way to do a no-op at the moment.
Comment #21
Gábor Hojtsy@catch: if the D6/D7 site did not have a language_default variable, it means its default language was English. It does not mean that its default language was the same as whatever the default language is on the target site. So logically at least, the information that the default language is English should be migrated to the target site as opposed to not changing the default language of the target site at all.
Comment #22
alexpottCommitted and pushed ecd6148 to 8.3.x and f4c1f48 to 8.2.x. Thanks!
I've stared at this several times and wondered about the hackyness of this. It does not look ideal but in the long run this fixes a tricky bug and we can refactor later if someone chooses to - and we've already got a test. So nice work.
Comment #26
maxocub CreditAttribution: maxocub commentedComment #27
maxocub CreditAttribution: maxocub as a volunteer and commented