Migrate variables to config

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6ActionSettings.php
@@ -0,0 +1,56 @@
+      'value' => 'i:35;',

IMO this should be serialize($var); This way it is more maintainable.
and here is a code review

$phpcs  --standard=Drupal --extensions=php,module,inc,install,test,profile,theme core/mod
ules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/

FILE: ...ate_drupal\lib\Drupal\migrate_drupal\Tests\Dump\Drupal6NodeSettings.php
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
  2 | ERROR | Missing file doc comment
 12 | ERROR | Missing short description in function doc comment
 13 | ERROR | There must be an empty line before the parameter block
 13 | ERROR | Missing comment for param "$database" at position 1
--------------------------------------------------------------------------------


FILE: ...rate_drupal\lib\Drupal\migrate_drupal\Tests\MigrateForumConfigsTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 1 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 44 | WARNING | Line exceeds 80 characters; contains 168 characters
 45 | ERROR   | No space before comment text; expected "//
    |         | $this->assertIdentical($config->get('forum_nav_vocabulary'),
    |         | '1');" but found
    |         | "//$this->assertIdentical($config->get('forum_nav_vocabulary'),
    |         | '1');"
--------------------------------------------------------------------------------


FILE: ...grate_drupal\lib\Drupal\migrate_drupal\Tests\MigrateNodeConfigsTest.php
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 26 | ERROR | Missing function doc comment
 26 | ERROR | Visibility must be declared on method "testNodeSettings"
 32 | ERROR | Calling class constructors must always include parentheses
--------------------------------------------------------------------------------


FILE: ...ate_drupal\lib\Drupal\migrate_drupal\Tests\MigrateSystemConfigsTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 2 WARNING(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
  58 | WARNING | Line exceeds 80 characters; contains 81 characters
  91 | WARNING | Line exceeds 80 characters; contains 81 characters
 139 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
--------------------------------------------------------------------------------

Some doc fixes.

Some more doc fixes.

Thanks for adding the @todo. Here is an interdiff :). It is a long patch needs interdiff to help reviewer.

For future reference.

1. +++ b/core/modules/migrate_drupal/config/migrate.migration.d6_menu_settings.yml
   @@ -0,0 +1,14 @@
   \ No newline at end of file
   
   +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6MenuSettings.php
   @@ -0,0 +1,62 @@
   \ No newline at end of file
   

   EOL error.

2. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6MenuSettings.php
   @@ -0,0 +1,62 @@
   + * Contains \Drupal\migrate\Tests\Drupal6MenuSettings.
   
   +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/MigrateMenuConfigsTest.php
   @@ -0,0 +1,45 @@
   + * Contains \Drupal\system\Tests\Upgrade\MigrateMenuConfigsTest.
   

   Wrong namespace.

and fixes.

one more miner fix

Thank you!

21: 2154955_21.patch queued for re-testing.

@file doc is wrong in all files I'll fix it.

It is a simple reroll so back to RTBC.

Reroll as requested.

Fixed other assertions.

Spent some time reviewing this today with xjm, moshe, and eff.

Over-arching concern is:

- How do we a) know that, and b) continue to know that ;) the dump PHP files are accurate as compared to the source schemas? In the past, we used actual database dumps to test the upgrade path; this moves it to code-generated database dumps, which is better for modularity, but presumes that we've caught everything accurate and transcribed it into PHP. So, is this coming from a script? If so, where is this script? I didn't find it on looking in /scripts.

- The second unknown (without lengthy manual verification) is whether or not all of the values in the dumps are reflected in the assertions in the tests, along with the correct default values. Can we automate this somewhow so that if these two ever were to fall out of sync, we would know?

A few comments about the specific elements of the patch:

1. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6ContactSettings.php
   @@ -0,0 +1,59 @@
   +    $database->schema()->createTable('variable', array(
   +      'fields' => array(
   +        'name' => array(
   +          'type' => 'varchar',
   +          'length' => 128,
   +          'not null' => TRUE,
   +          'default' => '',
   +        ),
   +        'value' => array(
   +          'type' => 'blob',
   +          'not null' => TRUE,
   +          'size' => 'big',
   +          'translatable' => TRUE,
   +        ),
   +      ),
   +      'primary key' => array(
   +        'name',
   +      ),
   +      'module' => 'contact',
   +      'name' => 'variable',
   +    ));
   
   

   This hunk ends up getting copy/pasted like 600 times in this patch. It'd be far preferable, I think, it to be in the code only once and then called into the various tests that need it. While the risk of that schema changing in D6 is pretty low, that's not necessarily the case for D7 which still has another N years go to until 9.x LTS comes out.

2. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateSyslogConfigsTest.php
   @@ -0,0 +1,47 @@
   +    // @TODO: remove this typecast once there's schema of rhtis config.
   

   Just a silly typo.

3. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateSystemConfigsTest.php
   @@ -0,0 +1,191 @@
   +    config_context_leave();
   ...
   +    config_context_enter('config.context.free');
   

   Can we add some comments to explain why this was done? No other tests had to do this.

4. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6SystemSite.php
   @@ -40,35 +48,31 @@ public static function load(Connection $database) {
   -      'value' => 's:6:"drupal";',
   +      'value' => serialize('Drupal'),
   

   This whole section changes from serialized string values to serialize('Foo'). The rest of them don't. What's up with that? We should be consistent.

Cool, class Drupal6DumpCommon addresses point 1. The other points are addressed, too.

The test coverage is extremely thorough, so I'm not criticising it, but I'm a bit concerned about ongoing maintenance/upkeep.

Right now, each Dump file adds 1-N lines like this:

+    ->values(array(
+      'name' => 'actions_max_stack',
+      'value' => 'i:35;',
+    ))

Then, later, there's a test that pulls in that database dump and makes 1-N test assertions like so:

+    $this->assertIdentical($config->get('recursion_limit'), 35);

So my questions are:

a) Where did 'i:35' come from in the first place, and how can we verify that it's correct against a valid D6 schema?

b) How do we know, other than rigorous manual cross-referencing of Dumps to Tests, whether or not we indeed have assertions for all values inserted?

c) Is there a way we can automate the generation of these dumps/assertions to catch changes that may have happened in the upstream schema (both schema and default values)? While this is unlikely to happen in Drupal 6, it could definitely still happen in Drupal 7 sometime in the next 4+ years until it's EOLed.

Does that make sense?

These questions aren't commit-blockers btw, but I would love to have them answered before we go too far down the road of mass-copying this pattern to other migrations.

Also, this is totally unrelated but just a question I had when reviewing the patch:

+id: d6_action_settings
+source:
+  plugin: drupal6_variable
+  variables:
+    - actions_max_stack
+process:
+  recursion_limit: actions_max_stack
+destination:
+  plugin: d8_config
+  config_name: action.settings

For "source," I can grep for drupal6_variable, and that gets me to core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/d6/Variable.php.

For "destination," I can grep for d8_config (side-note: could we please make a minor follow-up to make those plugin IDs consistently spell out the word "drupal"? :D) and that gets me to core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/Config.php

However, I'm not sure how to see what "process" class is being used to convert the values. It took us a few minutes to realize that it was doing "new name : old name." Maybe something for a follow-up issue to discuss DX-wise, not sure.

Ok, so in a nutshell (summarizing from IRC): those default values are manually entered through inspecting the D6 code, and then they're manually copy/pasted to assertion messages. There's no real way of automating this... default variables don't get into the database unless you somehow loop through and save the config form for every module, and even then it doesn't cover variables like actions_max_stack which only appears in variable_get().

So this is a bit unfortunate, because the tests basically only test the D6 schema at some point in time, and assume that whoever wrote that part of the tests did so correctly and was meticulous about finding all of the variables and transcribing their values properly in two different places. It also means that we will only catch holes and bugs in these default values/tests as people report bugs with the migration path.

However, what's here now is definitely far more complete than the current upgrade path tests, so I think in the end we come out ahead here.

These changes looks pretty good. I am not sure whether it is a good idea to move the custom homepage and site name to pretty much the default one.
This removes the certainty that the patch really works, but I don't block based upon that.

In general I wonder whether migrate in core would migrate the views settings as well

Ok, awesome!

Committed and pushed to 8.x. Thanks!