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.

CommentFileSizeAuthor
#12 interdiff-10-12.txt1.38 KBmaxocub
#12 2806285-12.patch4.56 KBmaxocub
#10 interdiff-9-10.txt896 bytesmaxocub
#10 2806285-10.patch4.47 KBmaxocub
#9 interdiff-6-9.txt4.83 KBmaxocub
#9 2806285-9.patch4.32 KBmaxocub
#6 interdiff-3-6.txt468 bytesmaxocub
#6 2806285-6.patch5.15 KBmaxocub
#3 2806285-3.patch5.12 KBmaxocub
#3 2806285-3-test-only.patch2.55 KBmaxocub

Comments

maxocub created an issue. See original summary.

maxocub’s picture

maxocub commented
Issue summary: View changes
maxocub’s picture

maxocub commented
Status: Active » Needs review
Issue tags: +Dublin2016
StatusFileSize
new 2806285-3-test-only.patch2.55 KB
new 2806285-3.patch5.12 KB

The last submitted patch, 3: 2806285-3-test-only.patch, failed testing.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/language/migration_templates/default_language.yml
@@ -4,7 +4,7 @@ migration_tags:
-  plugin: variable
+  plugin: language_default
   variables:
     - language_default

    Why do you still have the variables key here given the plugin does not use it anymore? 

  2. +++ b/core/modules/language/src/Plugin/migrate/source/LanguageDefault.php
@@ -0,0 +1,71 @@
+ * @MigrateSource(
+ *   id = "language_default"
+ * )
+ */
+class LanguageDefault extends DrupalSqlBase {

    Any reason not to extend from the Variable plugin directly? Too messy?

maxocub’s picture

maxocub commented
Status: Needs work » Needs review
StatusFileSize
new 2806285-6.patch5.15 KB
new interdiff-3-6.txt468 bytes

Re #5:

  1. Removed
  2. The Variable plugin set the 'variables' property in his constructor, based on the 'variables' key which does not exist here
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

Ok, I trust that you considered extending the variable plugin and this was cleaner. The implementation and tests look fine.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thx 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.

maxocub’s picture

maxocub commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2806285-9.patch4.32 KB
new interdiff-6-9.txt4.83 KB

@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:

  • I removed the source plugin and use the default_value plugin instead to deal with a non-existent language_default variable.

Now the long story:

  • I tried to add a test for the new plugin, but I hit a wall when the MigrateSourceTestBase was trying to convert the stdClass of the language_default to a string.
  • Then I tried to return an array instead of a stdClass, but I ended up doing a lot of processing in a source plugin.
  • Then I tried to use a process plugin, but @phenaproxima told me that what I was doing could be done with existing plugins (default_value, callback & extract)
  • Then I found that I could not use default_value because get_object_vars (the callback used with the Callback process plugin) expected an object, which cannot be set in a yaml file
  • Then @phenaproxima suggested that I modify the callback plugin to accept arguments and use settype with 'array' as an argument to cast the stdClass to an array.
  • I tried @phenaproxima suggestion, but it was a lot of work and a bit out of scope to make it solid, add test coverage and documentation. (Not to mention that settype pass the value by reference, contrary to get_oject_vars).
  • So finally, I found another way (a bit hackish you might say) by using json_encode and json_decode to be able to set an array as a default value and still and up with an object.

What do you think?

maxocub’s picture

maxocub commented
StatusFileSize
new 2806285-10.patch4.47 KB
new interdiff-9-10.txt896 bytes

@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.

quietone’s picture

quietone commented

Wow, 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.

maxocub’s picture

maxocub commented
StatusFileSize
new 2806285-12.patch4.56 KB
new interdiff-10-12.txt1.38 KB

Some comments improvements suggested by @phenaproxima on IRC.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

Woah, nice trick, and thanks for documenting how you got there. Let's get this in then :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2806285-12.patch, failed testing.

maxocub’s picture

maxocub commented
Status: Needs work » Needs review

That looks like random fail. Let's re-roll.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

Unrelated fail in Node.Drupal\node\Tests\NodeTypeTranslationTest.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2806285-12.patch, failed testing.

maxocub’s picture

maxocub commented
Status: Needs work » Needs review

Another unrelated fail...

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Isn't there a skip_on_empty option? This looks like a clever way to do a no-op at the moment.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

@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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed ecd6148 to 8.3.x and f4c1f48 to 8.2.x. Thanks!

+++ b/core/modules/language/migration_templates/default_language.yml
@@ -10,9 +10,22 @@ source:
+      plugin: callback
+      callable:
+        - '\Drupal\Component\Serialization\Json'
+        - 'encode'
+    -
+      plugin: callback
+      callable:
+        - '\Drupal\Component\Serialization\Json'
+        - 'decode'

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.

  • alexpott committed ecd6148 on 8.3.x 
    Issue #2806285 by maxocub, Gábor Hojtsy, quietone, catch: If the default...

  • alexpott committed f4c1f48 on 8.2.x 
    Issue #2806285 by maxocub, Gábor Hojtsy, quietone, catch: If the default...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

maxocub’s picture

maxocub commented
Assigned: maxocub » Unassigned
maxocub’s picture

maxocub commented