Contact: Not all values are being migrated

A migration with the following manifest completes with no errors.

Forgot to post it?

D8 stores the recipients as an array. Please try the attached patch.

Yes I see the problem, the category is selected in the contact table but in Drupal 8 we need to set the default_category key in the contact.settings.yml config.

Proposed Solution

Create a new source for the d6_contact_settings migration which extends variable, gets the existing variables and then queries the contact table for the "default" category. The default can then be mapped in the migration to default_category.

Benjy,

Tell me if I'm on the right track...

1. I'll need to update migrate.migration.d6_contact_settings.yml, change the source plugin to a new ContactSettings plugin (migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/ContactSetting.php?)
2. Add "- contact_default_selected" to source: variables: in migrate.migration.d6_contact_settings.yml

I'd like to take this one on - this seems like something relatively straight-forward that I can get my feet wet on. Can you point me in the direction of a similar source migration that extends variable and does something similar? I went through all the existing D6 source migrations and didn't find any...

Thanks,
-mike

To elaborate

1. Create a Drupal\migrate_drupal\Plugin\migrate\source\d6\ContactSettings class/plugin extending Variable.
2. In Variable move array_map('unserialize', $this->prepareQuery()->execute()->fetchAllKeyed() into a new method, I recommend calling it values
3. ContactSettings now can do
   

   function runQuery() {
     $default_category = $this->select('contact')->...->execute()->fetchField()
     return new \ArrayIterator(array($this->values() + array('default_category' => $default_category)));
   }
   

4. Change the source plugin to contact_settings
5. Add the necessary line to process
6. Add an assert to the test.

Benjy, chx - thanks for the info above (and thanks, chx, for your time on IRC).

Attached is a patch that adds a new D6 ContactSettings class that takes care of migrating the default category to D8. I've added a new assertion to the MigrateContactConfigsTest as well, and it passed on my local machine.

-mike

Updated previous patch with proper dependency and migration process plugin. Tests still pass!

-mike

1. There are trailing spaces
2. values() should be in Variable, it's factored out from its runQuery, it's the reusable part of runQuery
3. The runQuery in ContactSettings is indented too much, see for example Variable::query.

Re-rolled patch.

1. Removed trailing spaces.
2. Moved values() from ContactSettings to Variable, update runQuery in Variable to utilize it.
3. Fixed up runQuery indentation in ContactSettings.

Third time's a charm!

-mike

This is looking good, just some super minor formatting errors.

1. +++ b/core/modules/migrate_drupal/config/migrate.migration.d6_contact_settings.yml
   @@ -1,12 +1,18 @@
   \ No newline at end of file
   

   Missing new line.

2. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/Variable.php
   @@ -37,7 +37,11 @@ public function __construct(array $configuration, $plugin_id, array $plugin_defi
   +  ¶
   

   Trailing space.

3. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/d6/ContactSettings.php
   @@ -0,0 +1,29 @@
   +  }
   

   Lets add a new line here as well.

4. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/d6/ContactSettings.php
   @@ -0,0 +1,29 @@
   \ No newline at end of file
   

   Missing new line.

Benjy,

Thanks for the review. I just updated the patch with your changes as well as strengthened the MigrateContactCategoryTest with some additional dump data and assertions.

I locally ran the following tests, with everything passing:

1. MigrateContactCategoryTest
2. MigrateContactConfigsTest
3. MigrateDrupal6Test
4. Manual test from a real-ish D6 database with 3 contact categories with various settings.

Thanks,
-mike

Hmm - all tests pass on my local via CLI. Is this a testbot glitch? Should I resubmit the patch for re-testing?

-mike

13: 2214313-13.patch queued for re-testing.

Being a bit paranoid, I just:

1. Updated my local copy of D8.
2. Ensured 2214313-13.patch still applies cleanly.
3. Ran MigrateContactCategoryTest locally via CLI (using run-tests.sh) - passed.
4. Ran MigrateContactConfigsTest locally via CLI (using run-tests.sh) - passed.
5. Ran MigrateDrupal6Test locally via CLI (using run-tests.sh) - passed.

I'm also hiding the previous patches (hoping that the testbot doesn't test patches that don't have their "Display" option selected).

I'm re-submitting the patch for testing...

-mike

13: 2214313-13.patch queued for re-testing.

@ultimike, what version of PHP are you using? The testbot is using PHP 5.4 so if you're using a version less, that would probably be your issue.

Recipients is an array and yes this is possibly passing on PHP 5.3 but def not on PHP 5.4

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Plugin/migrate/source/Variable.php
@@ -37,7 +37,11 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
+  protected function values() {
+    return array_map('unserialize', $this->prepareQuery()->execute()->fetchAllKeyed());

Some docs here would be good.

What for? https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/variab... has the same query and has no query-related comments.

Oh I see you wanted doxygen!

29: 2214313_29.patch queued for re-testing.

Benjy, chx,

I'm running PHP 5.4.4 locally and I run all tests via the CLI:

~/Sites/drupal8 $ which php
/Applications/MAMP/bin/php/php5.4.4/bin/php
~/Sites/drupal8 $ php core/scripts/run-tests.sh --url 'http://drupal8.dev/' --class 'Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test'

Good catch on the recipients array stuff.

I just pulled 8.x, applied 2214313_29.patch, and all tests pass on my local.

The latest testbot fail (#32) appears to be due to it running out of free disk space (file_put_contents(): Only 0 of 74 bytes written, possibly out of free disk)...

-mike

29: 2214313_29.patch queued for re-testing.

Looks good to me.

Committed and pushed to 8.x. Thanks!