Upgrade path for Color 7.x

This patch does migrate the D7 color data but there are two problems that I know of. One is a schema error for the variable 'screenshot' and the other is there is no test for the destination plugin.

For the first problem, the error is 'color.theme.garland:screenshot missing scheme'. It seems the solution is to add the screenshot variable to the schema. If that is true, then that needs to be done use the update process?

For the destination plugin, I'll work on that again tomorrow. In the meantime, I'd like to see how testing goes.

Look Ma, no hash!!!

As expected, fail on 'color.theme.garland:screenshot missing schema'.

Talked to phenaproxima on IRC and got this sorted. Thx!

1. Absolutely
2. Yes, of course.
3. Oh my, I need more sleep. Fixed.

Kudos to phenaproxima!

+++ b/core/modules/color/src/Plugin/migrate/process/d7/Color.php
@@ -0,0 +1,35 @@
+    // Result array is configuration name, configuration sub element and value.

For those of us not already conversant with the color variables, could the comment show by example what's happening? I take it we're taking something like variable color_bartik_palette with value array(stuff), and turning it into array('color.theme.bartik', 'palette', array(stuff))...

Apart from that, looks good, but I'd leave the RTBC to someone who can test this manually and see that they get the desired color settings.

Actually, just had a second thought - should we be checking to be sure the referenced theme exists in D8? If we were to blindly migrate the color settings for a theme that isn't there, and the theme were installed later, would it pick up the migrated values? I'm guessing it'll install its defaults...

@mikeryan and @phenaproxima, thx for the review. I appreciate it, esp as this is a low priority item.

I've addressed all your concerns, and will do some testing in the next few days paying attention to what happens when a theme is installed after a migration.

This and another theme 'variable to config' issue both need and an explode process plugin. And since there was an existing issue just for that, #2575101: Add an explode/separator process plugin I posted a patch there. I rewrote this to use that plugin and I think it is easier to follow now.

Still needs testing as suggested in #11.

And blocked by #2575101: Add an explode/separator process plugin

#2575101: Add an explode/separator process plugin is in so no longer postponed.

Actually, just had a second thought - should we be checking to be sure the referenced theme exists in D8? If we were to blindly migrate the color settings for a theme that isn't there, and the theme were installed later, would it pick up the migrated values? I'm guessing it'll install its defaults...

Need to follow up this question from mikeryan's query in #11.

Let's see what the testbot finds.

Postponed on #2675164: Pipeline using explode and concat fails and #2675156: Pipeline using explode and extract fails.

Too much time has passed. Removing the novice flag because someone new isn't going to be able to work on any actionable tasks on this issue.

> core/modules/color/src/Plugin/migrate/destination/d7/Color.php

This shouldn't exist. Destinations are not per D7 they are per D8. So: core/modules/color/src/Plugin/migrate/destination/Color.php .

Now that #2575101 has been committed, this can be worked on again.

Rerolled.

Fix namespace error.

Correct test fails.

@phenaproxima, thanks. Fixed all the nits.

About #3, the second item in the array, which appears empty. is the value piped in from the previous plugin. However, it is odd and to avoid further questions about it I have changed the yml to be more explicit. The pipeline is now split into two step, the first getting the 'theme_name' and in the second step '@theme_name' is an input value. That should make it clearer.

Fudge. Here is the correct one.

+++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
@@ -43524,6 +43524,42 @@
+->values(array(
+  'name' => 'color_garland_files',
+  'value' => 'a:19:{i:0;s:50:"public://color/garland-b69cfcec/menu-collapsed.gif";i:1;s:54:"public://color/garland-b69cfcec/menu-collapsed-rtl.gif";i:2;s:49:"public://color/garland-b69cfcec/menu-expanded.gif";i:3;s:45:"public://color/garland-b69cfcec/menu-leaf.gif";i:4;s:67:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/body.png";i:5;s:69:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/bg-bar.png";i:6;s:75:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/bg-bar-white.png";i:7;s:69:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/bg-tab.png";i:8;s:76:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/bg-navigation.png";i:9;s:78:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/bg-content-left.png";i:10;s:79:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/bg-content-right.png";i:11;s:73:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/bg-content.png";i:12;s:81:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/bg-navigation-item.png";i:13;s:87:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/bg-navigation-item-hover.png";i:14;s:77:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/gradient-inner.png";i:15;s:67:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/logo.png";i:16;s:73:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/screenshot.png";i:17;s:41:"public://color/garland-b69cfcec/style.css";i:18;s:45:"public://color/garland-b69cfcec/style-rtl.css";}',
+))
+->values(array(
+  'name' => 'color_garland_logo',
+  'value' => 's:40:"public://color/garland-b69cfcec/logo.png";'
+))
+->values(array(
+  'name' => 'color_garland_palette',
+  'value' => 'a:5:{s:4:"base";s:7:"#d0cb9a";s:4:"link";s:7:"#917803";s:3:"top";s:7:"#efde01";s:6:"bottom";s:7:"#e6fb2d";s:4:"text";s:7:"#494949";}',
+))
+->values(array(
+  'name' => 'color_garland_screenshot',
+  'value' => 's:73:"/var/www/drupal/sites/default/files/color/garland-b69cfcec/screenshot.png";'
+))
+->values(array(
+  'name' => 'color_garland_stylesheets',
+  'value' => 'a:2:{i:0;s:41:"public://color/garland-b69cfcec/style.css";i:1;s:45:"public://color/garland-b69cfcec/style-rtl.css";}',
+))

Is there anything that tests this is not migrated?

There's also an array syntax error...

FILE: ...upal/core/modules/color/src/Plugin/migrate/destination/Color.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 63 | ERROR | [x] Short array syntax must be used to define arrays
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Fixed the short array syntax error.

Is there anything that tests this is not migrated?

No. Why is one needed here? What is the use case?

@quietone because we need to ensure that we don't migrate color settings for themes that don't exist - right?

Patch in #38 no longer applies. Re-rolled.

Correct test failures.

I'm not sure the best way to filter this migration - are we talking a custom process or is there some way to do this in the yaml?

Assigning for review.

This looks good, but still needs work for two things:

1. The directory migration_templates should be changed for migrations.
2. The non-existent themes settings should not be migrated as noted in previous comments. I think one way to achieve that would be in the source plugin, we could get a list of available themes and filter the query whit those.

This should address all the points in #47.

Thanks @quietone! I think that the issue with active themes is on the destination site, not the source.

One way to do that would be to inject the theme_handler service in the source plugin and use listInfo() method.

We'll also need to test that garland theme settings are not migrated as suggested in #37.

@maxocub, thx. Must still be in holiday mode.

This injects the theme_handler in the source plugin to get the themes installed on the destination. The test now installs 5 themes and a test has been added to confirm that garland was not migrated.

There is still a problem with the source plugin test, it fails on $this->assertCount($expected_count, $plugin); in MigrateSourceTestBase. I don't know how to fix the query so that that will work. The query could be changed and a prepareRow added to remove the unwanted rows but doesn't seem the right way to go.

Tried simplifying the query in the source plugin but the source plugin test still fails.

Adding to my todo list!

Let's try this. Reworked the source plugin query again and updated the tests. This required changes to the migration as well for checking if the theme is installed on the destination and to exclude the screenshot variables.

Add Configuration tag to the migration and updated the UI tests.

Ah, I see the new destination needs to be added to getConfigurationClasses in the test.

Fixed a few comments.

This patch looks quite good, I just found a few points that could be improved.

1. +++ b/core/modules/color/src/Plugin/migrate/source/d7/Color.php
   @@ -0,0 +1,102 @@
   +    // Screenshot is ignored and will be skipped in the process pipeline.
   +    if ($name[2] != 'screenshot') {
   +      $row->setSourceProperty('screenshot', TRUE);
   +    }
   
   +++ b/core/modules/color/migrations/d7_color.yml
   @@ -0,0 +1,48 @@
   +  # Skip if the variable name ends in 'screenshot'.
   +  screenshot:
   +    plugin: skip_on_empty
   +    source: screenshot
   +    method: row
   

   I find this code unintuitive. Since we have access to the exploded name in the yml file, we could use a combinaison of static_map & skip_on_empty. I think it might be cleaner and more intuitive :

   screenshot:
     -
       plugin: static_map
       source: '@element_name'
       bypass: true
       map:
         - screenshot: false
     -
       plugin: skip_on_empty
       method: row
   

   Also, we don't test that screenshots are not migrated. In the fixtures, there's a screenshot variable for garland, but those variables are skipped because garland is not installed on the destination. I think we should add a screenshot for bartik in the fixtures and add an assert that it is in fact not migrated.

2. +++ b/core/modules/color/tests/src/Kernel/Migrate/d7/MigrateColorTest.php
   @@ -0,0 +1,65 @@
   +      'classy',
   +      'seven',
   +      'stable',
   +      'stark',
   

   Do we need to install all those themes since we only test bartik?

1. OK. implemented your suggestion.
2. True, we probably don't need to install all the themes for the test.

Thanks @quietone!
Could you add a $this->assertNull($config->get('screenshot')); in MigrateColorTest.php though, to make sure they are not migrated and don't cause schema errors?

+++ b/core/modules/color/tests/src/Kernel/Plugin/migrate/source/d7/ColorTest.php
@@ -0,0 +1,129 @@
+      [
+        'name' => 'color_custom_stylesheets',
+        'value' => ['public:://color/custom-beadedff/screenshot.png'],
+      ],
...
+      [
+        'name' => 'color_custom_stylesheets',
+        'value' => ['public:://color/custom-beadedff/screenshot.png'],
+      ],

This is a nit, but since this variable is supposed to be a stylesheet, it should be a CSS file.

Back to NW to address #65.

Sorry, I had intended to add the test that screenshot was not migrated.

Both fixed.

Odd. the patch isn't showing in the comment.

Well, the fixes for #65 are complete and the patch has passed tests. It, 2500495-67.patch, just isn't showing up in the comment.

Weird that the files don't show up in comment #67, but they do show up in the IS.
I thinks this is ready, RTBCed.

+++ b/core/modules/color/migrations/d7_color.yml
@@ -0,0 +1,54 @@
+  plugin: d7_color

+++ b/core/modules/color/src/Plugin/migrate/destination/Color.php
@@ -0,0 +1,90 @@
+ *   id = "d7_color"

I think this should be just color since destinations shouldn't be for a specific source. This is a similar comment to #22. Guess this is just a missed legacy of the earlier patches.

Yes, that got missed. Fixed now.

Tests are green, back to RTBC.

Committed and pushed d25a77e0ec to 8.6.x and 1cc630399e to 8.5.x. Thanks!

Backported to 8.5.x since migrate_drupal is still in beta.