Variable to config: theme settings [d7]

+++ b/core/modules/system/src/Plugin/migrate/destination/d7/ThemeSettings.php
@@ -0,0 +1,92 @@
+ * Persist theme settings tohe config system.

Typo? Should this say "to the"?

Otherwise this looks great to me - just one small step away from RTBC.

Patch :)

@amoebanath - you have added wrong patch, I have added a correct patch :)

Great, thanks both.

Retests passed successfully - back to RTBC.

Tests are green - back to RTBC.

Any Updates on this issue ?

+++ b/core/modules/system/src/Plugin/migrate/destination/d7/ThemeSettings.php
@@ -0,0 +1,92 @@
+   * Constructs a BlockedIP object.

This definitely doesn't construct a BlockedIP object. c&p error?

+++ b/core/modules/system/tests/src/Kernel/Plugin/migrate/source/d7/ThemeSettingsTest.php
@@ -0,0 +1,79 @@
+
+    // The expected results are identical to the source data.
+    $tests[0]['expected_data'] = [

If the expected results are identical to the source data, why not do $tests['0]['expected_data'] = $tests[0]['source_data']['variable']; instead of duplicating the array manually?

Adjustments suggested in #29.

Aha, apparently the source data and expected results are nearly identical.

Adjusted the code to make the similarities/differences clear (and actually pass the test this time!)

Looks good. Back to RTBC.

Mostly just found minor things.

1. +++ b/core/modules/system/migration_templates/d7_theme_settings.yml
   @@ -0,0 +1,44 @@
   +  # build the configuration name from the variable name, i.e.
   +  # theme_bartik_settings becomes bartik.settings
   

   Make this a sentence IMHO.

2. +++ b/core/modules/system/migration_templates/d7_theme_settings.yml
   @@ -0,0 +1,44 @@
   +# Ignore settings not present in Drupal 8
   

   Comment standards need a . at the end IMHO.

3. +++ b/core/modules/system/src/Plugin/migrate/destination/d7/ThemeSettings.php
   @@ -0,0 +1,92 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function fields(MigrationInterface $migration = NULL) {
   +  }
   

   I guess we need this for the interface, can we document why we don't implement it?

4. +++ b/core/modules/system/src/Plugin/migrate/source/d7/ThemeSettings.php
   @@ -0,0 +1,48 @@
   +class ThemeSettings extends VariableMultiRow {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected $variables;
   

   Hm, if its inheritdoc, why are we defining it again? (If we need to, we need a newline above).

Tagging for the response to #34.

Looks like the {@inheritdoc} in #3 is referring to the method defined in the MigrateDestinationInterface interface. I'm no idea why the method is just a stub. Is it because we are migrating the equivalent of Drupal variables, and there aren't any fields? Should it return an empty array?

I don't see where $variables in #4 is defined or used. Where did this come from?

Patch for 1 and 2 only for #34.

I looked into #34.3 and #34.4:

• Let's make ThemeSettings::fields() return an empty array for interface conformance, and add an inline comment (not in the doc block) which says something like "Theme settings vary by theme, so no specific fields are defined."
• ThemeSettings::$variables is never used. Let's just remove it entirely.

Once these two points are done, I think this is RTBC.

phenaproxima's suggestions applied. I used your comment for #34.3 verbatim.

I realized tha I was working off the 8.4.x branch. Everything seemed to apply cleanly though, we'll see if the tests complete.

+++ b/core/modules/system/src/Plugin/migrate/destination/d7/ThemeSettings.php
@@ -87,6 +87,8 @@ public function getIds() {
+    return array();

One small thing -- the coding standards now require us to use short array syntax in core. So this should be [], not array().

Other than that, this looks perfect.

Ah, thanks for the catch. Updated patch included.

Werd up. #40 passed Drupal CI, so I am pre-RTBCing #42 since it was just a syntax change. Nice one.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!