Update settings migration to target state API

adding static file with patch so it can be applied separately from the MR (in case any changes happen in the MR)

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😬

I was been able to create a source plugin which gets the correct variables based on the module version, like this,

  /**
   * {@inheritdoc}
   */
  public function query() {
    // If 7.x-4.x override variables to new naming convention.
    if ($this->getModuleSchemaVersion('acquia_agent') >= 7004) {
      $this->variables = array_unique(array_merge(array_values($this->getNewMapping()), array_values($this->variablesNoRowIfMissing)));
    }
    return $this->getDatabase()
      ->select('variable', 'v')
      ->fields('v', ['name', 'value'])
      ->condition('name', $this->variables, 'IN');

Where getNewMapping() is:

  /**
   * The new mapping
   * @return string[]
   */
  protected function getNewMapping() {
    return [
      'acquia_application_uuid' => 'acquia_application_uuid',
      'identifier' => 'acquia_identifier',
      'key' => 'acquia_key'
    ];
  }

Which works to get the correct variables, but now i have to change the destination to STATE from CONFIG, and alter the prefix...something like

  /**
   * {@inheritdoc}
   */
  public function prepareRow(Row $row) {
    // If 7.x-4.x override to state system from config.
    if ($this->getModuleSchemaVersion('acquia_agent') >= 7004) {
      $row->setDestinationProperty('plugin', 'state');
      $row->setDestinationProperty('state_prefix', 'acquia_connector.');
      $row->setSourceProperty('variables', $this->variables);
    }
  }

But dont have it all quite fleshed out yet...just wanted to show im still noodling on this 😅

EDIT AFTER A WALK: Why do we even need to try and preserve these old values? If a user is on 7.x-3.x, then the new variables which are needed for D10 Acquia connector to work aren't present in the D7 database anyway...right? I tried to set up the 7.x-3.x version of the module locally and wasn't able to get it to connect to Acquia Cloud to even check out the contents of the variable table. So it would seem that in order for a D7 site to have a functioning Connector Module, it needs to be on Acquia Connector 7.x-4.x at this point.

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!

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.

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 🫣

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.

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

🤔

There was 1 failure:

1) Drupal\Tests\acquia_connector\Kernel\Migrate\d7\MigrateAcquiaConnectorConfigurationTest::testConfigurationMigration
Migration d7_acquia_connector_settings did not meet the requirements. Acquia Agent module version incompatible with this migration 
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'status'
+'error'

I think i need the change in the Kernel test in order for this to work 🙃 ....this feels like a 🐥 and 🥚 issue