Update settings migration to target state API

So was #3204533: Migrate Acquia Connector configurations from Drupal 7 → wrong? Or is it just outdated at this point, because the Drupal 7 module has changed in the past ~32 months? 🤔

It seems likely the module has changed since then, because https://www.drupal.org/project/acquia_connector/releases/7.x-4.0 → was released in November 2022, but #3204533 happened in 2021. The migration was simply never updated 🙈😬

Looks like we'll need to expand the test coverage/fixture in MigrateAcquiaConnectorConfigurationTest. See \Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase::getModuleSchemaVersion() for how to make version-dependent migrations.

#4: cool — but did you see #3? 😇

I did! I'm still not sure what to make of the guidance right now though. Should I be creating an entirely new source plugin which varies the variable names based on the underlying module version? If so, how do we account for this in the migration yaml mapping?

For example in d7_acquia_connector_settings the following variables are mapped:

   source:
     plugin: variable
     variables:
       - acquia_agent_debug
       - acquia_spi_cron_interval
       - acquia_spi_cron_interval_override
       - acquia_agent_hide_signup_messages
     source_module: acquia_agent
   process:
     debug: acquia_agent_debug
     cron_interval: acquia_spi_cron_interval
     cron_interval_override: acquia_spi_cron_interval_override
     hide_signup_messages: acquia_agent_hide_signup_messages

If I change the variable names coming from the source this mapping is no longer valid. Should I be overriding these values somewhere as well? Perhaps if i create a custom source plugin for this, I also need to make a custom process plugin (to determine correct mapping)... am I overthinking it? 😁 I also had the thought of doing this in an EventSubscriber, but wasn't sure the best way to access the D7 database in a consistent manner (as it wont always be "migrate" in settings.php) ... sorry if i am missing something obvious😬

Should I be creating an entirely new source plugin which varies the variable names based on the underlying module version? If so, how do we account for this in the migration yaml mapping?

IIRC if a migration source plugin's checkRequirements() throws an exception, that migration is automatically excluded by AM:A. IIRC there's lots of uses of that in the media_migration module, because that migrates from a LOT of different sources into core media.

Therefore:

• two different migration source plugins, one for each version, with an explicit check in ::checkRequirements()
• two different migration plugins (each using one of the two migration source plugins)

… should be enough, because AM:A will automatically eliminate the one that doesn't work

because AM:A will automatically eliminate the one that doesn't work

I did not know about this 😅 - that seems more appropriate than what I was attempting to accomplish...one migration which varies source, process, and destination key+values 🙃. Will give it a go!

I think I spotted what's wrong 🤓

Oh! Whoopsies, good find 😬 After making these changes I altered the tests a little bit and tried running them. I was met with:

  www-data@2fdc9bf0644d:/app/docroot/core$ ../../vendor/bin/phpunit  ../modules/contrib/acquia_connector/tests/src/Kernel/Migrate/d7/MigrateAcquiaConnectorConfigurationTest.php
  PHPUnit 9.6.16 by Sebastian Bergmann and contributors.

  Testing Drupal\Tests\acquia_connector\Kernel\Migrate\d7\MigrateAcquiaConnectorConfigurationTest
  E                                                                   1 / 1 (100%)R

  Time: 00:09.830, Memory: 10.00 MB

  There was 1 error:

  1) Drupal\Tests\acquia_connector\Kernel\Migrate\d7\MigrateAcquiaConnectorConfigurationTest::testConfigurationMigration
  PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught AssertionError: The container was serialized. in /app/docroot/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php:88
  Stack trace:
  #0 /app/docroot/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php(88): assert(false, 'The container w...')
  #1 [internal function]: Drupal\Core\DependencyInjection\ContainerBuilder->__sleep()
  

I was able to get around this by implementing the same solution from here - https://www.drupal.org/project/drupal/issues/3197324 🐛 Exception trace cannot be serialized because of closure Needs work .

I made a few updates to the MR which i believe also allows it to have a passing test now 😎

  www-data@2fdc9bf0644d:/app/docroot/core$ ../../vendor/bin/phpunit  ../modules/contrib/acquia_connector/tests/src/Kernel/Migrate/d7/MigrateAcquiaConnectorConfigurationTest.php
  PHPUnit 9.6.16 by Sebastian Bergmann and contributors.

  Testing Drupal\Tests\acquia_connector\Kernel\Migrate\d7\MigrateAcquiaConnectorConfigurationTest
  .                                                                   1 / 1 (100%)

  Time: 00:08.521, Memory: 10.00 MB

  OK (1 test, 7 assertions)
  

Do you mind explaining more about how I might use the minimum_version here? My conditions are:
if >= 7004, run variables to state
if < 7004, run prior config migration
ill need to ponder more about how to accomplish this with annotation based 🫣

The last submitted patch, 4: 3416551_update_settings_migration.patch, failed testing. View results →

This patch is failing to apply for me locally for some reason, not sure why. The part which is failing is related to the additions in the test fixture:

diff --git a/tests/fixtures/drupal7.php b/tests/fixtures/drupal7.php
index 630b6d0..9b8a86d 100644
--- a/tests/fixtures/drupal7.php
+++ b/tests/fixtures/drupal7.php
@@ -82,6 +82,18 @@ $connection->insert('variable')
     'name' => 'acquia_subscription_data',
     'value' => 'a:12:{s:9:"timestamp";i:1234567890;s:6:"active";s:1:"1";s:4:"href";s:83:"https://insight.acquia.com/node/uuid/1b2c3456-a123-456d-a789-e1234567895d/dashboard";s:4:"uuid";s:36:"1b2c3456-a123-456d-a789-e1234567895d";s:17:"subscription_name";s:4:"Test";s:15:"expiration_date";a:1:{s:5:"value";s:19:"2042-12-30T00:00:00";}s:7:"product";a:1:{s:4:"view";s:14:"Acquia Network";}s:16:"derived_key_salt";s:32:"1234e56789979a1d8ae123cd321a12c7";s:14:"update_service";s:1:"1";s:22:"search_service_enabled";i:1;s:6:"update";a:0:{}s:14:"heartbeat_data";a:4:{s:11:"acquia_lift";a:3:{s:6:"status";b:0;s:8:"decision";a:2:{s:10:"public_key";s:0:"";s:11:"private_key";s:0:"";}s:7:"profile";a:5:{s:12:"account_name";s:0:"";s:8:"hostname";s:0:"";s:10:"public_key";s:0:"";s:10:"secret_key";s:0:"";s:7:"js_path";s:0:"";}}s:22:"search_service_enabled";i:1;s:12:"search_cores";a:7:{i:0;a:2:{s:8:"balancer";s:28:"useast1-c1.acquia-search.com";s:7:"core_id";s:11:"TEST-123456";}i:1;a:2:{s:8:"balancer";s:28:"useast1-c1.acquia-search.com";s:7:"core_id";s:19:"TEST-123456.prod.v2";}i:2;a:2:{s:8:"balancer";s:28:"useast1-c1.acquia-search.com";s:7:"core_id";s:19:"TEST-123456.test.v2";}i:3;a:2:{s:8:"balancer";s:28:"useast1-c1.acquia-search.com";s:7:"core_id";s:18:"TEST-123456.dev.v2";}i:4;a:2:{s:8:"balancer";s:29:"useast1-c26.acquia-search.com";s:7:"core_id";s:24:"TEST-123456.prod.default";}i:5;a:2:{s:8:"balancer";s:29:"useast1-c26.acquia-search.com";s:7:"core_id";s:24:"TEST-123456.test.default";}i:6;a:2:{s:8:"balancer";s:29:"useast1-c26.acquia-search.com";s:7:"core_id";s:23:"TEST-123456.dev.default";}}s:21:"search_service_colony";s:28:"useast1-c1.acquia-search.com";}}',
   ])
+  ->values([
+    'name' => 'acquia_application_uuid',
+    'value' => 's:36:"297dddeb-a730-422d-bf87-f0fb4b0eaa31";',
+  ])
+  ->values([
+    'name' => 'acquia_identifier',
+    'value' => 's:10:"ABCD-12345";',
+  ])
+  ->values([
+    'name' => 'acquia_key',
+    'value' => 's:32:"0db2c95529d76edfc282e9eed0800b21";',
+  ])
   ->execute();

 $connection->insert('system')

Not sure what about this is problematic. The message from patch output is:

patch '-p1' --no-backup-if-mismatch -d 'docroot/modules/contrib/acquia_connector' < '/app/patches/breaking-test.patch'
Executing command (CWD): patch '-p1' --no-backup-if-mismatch -d 'docroot/modules/contrib/acquia_connector' < '/app/patches/breaking-test.patch'
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/tests/fixtures/drupal7.php b/tests/fixtures/drupal7.php
|index 630b6d0..9b8a86d 100644
|--- a/tests/fixtures/drupal7.php
|+++ b/tests/fixtures/drupal7.php
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

1 out of 1 hunk ignored

patch '-p0' --no-backup-if-mismatch -d 'docroot/modules/contrib/acquia_connector' < '/app/patches/breaking-test.patch'
Executing command (CWD): patch '-p0' --no-backup-if-mismatch -d 'docroot/modules/contrib/acquia_connector' < '/app/patches/breaking-test.patch'
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/tests/fixtures/drupal7.php b/tests/fixtures/drupal7.php
|index 630b6d0..9b8a86d 100644
|--- a/tests/fixtures/drupal7.php
|+++ b/tests/fixtures/drupal7.php
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

1 out of 1 hunk ignored

patch '-p2' --no-backup-if-mismatch -d 'docroot/modules/contrib/acquia_connector' < '/app/patches/breaking-test.patch'
Executing command (CWD): patch '-p2' --no-backup-if-mismatch -d 'docroot/modules/contrib/acquia_connector' < '/app/patches/breaking-test.patch'
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/tests/fixtures/drupal7.php b/tests/fixtures/drupal7.php
|index 630b6d0..9b8a86d 100644
|--- a/tests/fixtures/drupal7.php
|+++ b/tests/fixtures/drupal7.php
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

1 out of 1 hunk ignored

patch '-p4' --no-backup-if-mismatch -d 'docroot/modules/contrib/acquia_connector' < '/app/patches/breaking-test.patch'
Executing command (CWD): patch '-p4' --no-backup-if-mismatch -d 'docroot/modules/contrib/acquia_connector' < '/app/patches/breaking-test.patch'
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/tests/fixtures/drupal7.php b/tests/fixtures/drupal7.php
|index 630b6d0..9b8a86d 100644
|--- a/tests/fixtures/drupal7.php
|+++ b/tests/fixtures/drupal7.php
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

1 out of 1 hunk ignored

   Could not apply patch! Skipping. The error was: Cannot apply patch ./patches/breaking-test.patch

The only info i can really glean from that is can't find file to patch at input line 5 - but i have no idea why this would be the case.

The only info i can really glean from that is can't find file to patch at input line 5 - but i have no idea why this would be the case.

It's because this patch is patching tests. But Drupal modules' composer packages have their tests (and test fixtures) removed. So in situations like this, you'll want to create a BLABLABLA-fix-only.patch subset of the real patch, without those test-related changes.

ooooh, ok! i ran this instead ... git diff origin/4.x -- ':!./tests/*' > ~/Desktop/3416551-change-settings-fix-only.patch and attached that 😄

The last submitted patch, 17: 3416551-change-settings-fix-only.patch, failed testing. View results →

Yes, the fix-only patch is not supposed to pass tests :D

In July, DrupalCI will stop working, and patches won't be tested. So then the "CI FAILURE RED RED RED" thing will disappear. For now, name patches WHATEVER-do-not-test.patch, and they won't be tested 👍

Tests are passing for the MR, and that's all that matters! 👍

Released in 4.0.6