Move subscription and SPI data out of config and into state API

I'm seeing a write in expiration_date, likely due to the start of a new year,

--- a/mnt/tmp/massgov/drush_tmp_1516115738_5a5e171a71f19/acquia_connector.settings.yml
+++ b/mnt/tmp/massgov/drush_tmp_1516115741_5a5e171d6a876/acquia_connector.settings.yml
@@ -187,7 +187,7 @@ subscription_data:
   uuid: ff8ed1de-b8bc-48a4-b316-cd91bfa192c4
   subscription_name: 'massgov (ACE)'
   expiration_date:
-    value: '2017-10-19T00:00:00'
+    value: '2018-10-19T00:00:00'
   product:
     view: 'Acquia Network'

While I think that everything in the subscription_data key should be moved into the state system, I've been advised that this would probably break other Acquia products and internal systems that apparently read directly from Acquia Connector's config to get subscription data.

I'd think that we could at least move the expiration date safely out of the config, but who knows for sure.

Ideally, Acquia Connector would have a set of public 'getter' functions for subscription data so that other products don't have to directly read its config. This would have made this fix a whole lot simpler.

Thanks for the background. Guess someone should create those methods and then perhaps the Acquia teams can have 3 months (or whatever) to convert to new API.

We are running into this issue on all of our subscriptions. We do a config import after all of our deployments, and the subscriptions data always gets overwritten and the the status shows as inactive. Has there been any progress on this?

this is now causing major issues on client sites, after a deploy the subscription data is reset and then searches stop working which ends up with fatal errors

Uncaught PHP Exception Drupal\search_api_solr\SearchApiSolrException: "An error occurred while trying to search with Solr: Solr endpoint http://{host}.acquia-search.com:80/solr/{something}/ unreachable..

We have always used config_ignore to not export/store Acquia Connector module configuration in code and let it be generated automatically in the environment when the module is enabled. Would that help anyone with the issue outlined here?

Are there any plans to address this?

@timwood have you found a way to ignore configuration while also allowing modules to be enabled/disabled per environment splits?

Hey @rbp. We haven't had a use case where we needed to ignore config related to modules which are enabled/disabled per environment splits. In the case of the acquia_connector module, we just enable it across the board.

I did find this module, which looks like it does exactly what you are looking for: https://www.drupal.org/project/config_split_ignore

This is a related issue that can help relieve the "resaving the config object on all cron runs" issue (which also then triggers unneeded cache tag invalidations) : #3105440: Acquia Connector's calling Subscription::update() sometimes invalidates cache tags unnecessarily

Here's a first pass at a patch for 8.x-2.x. It seems to work for new installations, but upgrading installations will need an update hook.

I'm not sure if we'll be able to backport this or not. There's a larger scope since Acquia Search would also need to be updated, and it's potentially breaking if anyone has code that relies on subscription configuration being present. The Acquia Connector 8.x-2.x branch seems like the perfect place for this change since it's still in beta.

We should see if there's any other configuration that needs to move to state, since this may be our only shot to make it happen. Please let me know if you can think of anything.

+++ b/src/Client.php
@@ -167,9 +167,9 @@ class Client {
+        if (is_numeric(\Drupal::state()->get('acquia_subscription_data')) && is_array($response['result']['body'])) {

+++ b/src/Subscription.php
@@ -45,12 +45,13 @@ class Subscription {
-    $current_subscription = $config->get('subscription_data');
+    $current_subscription = \Drupal::state()->get('acquia_subscription_data');
...
+      \Drupal::state()->delete('acquia_subscription_data');
+      \Drupal::state()->set('acquia_subscription_data', ['active' => FALSE]);

@@ -78,7 +79,7 @@ class Subscription {
-        $config->set('subscription_data', $subscription)->save();
+        \Drupal::state()->set('acquia_subscription_data', $subscription);

@@ -101,7 +102,7 @@ class Subscription {
-      $subscription = $config->get('subscription_data');
+      $subscription = \Drupal::state()->get('acquia_subscription_data');

should the state service be injected here?

Agree, needs an update path to copy the current values and delete the existing config entry

Looking good though - thanks!

Thanks for the feedback. This patch should be ready to go, I appreciate any additional input. Like I mentioned previously, it's really important that we get this right now because it will be difficult to make similar changes once 8.x-2.x is stable.

Changes from #13:

• Adds update hook to move existing config into state
• Adds service injection where possible
• Adds select SPI keys to state (not all of it, since much of the SPI data can be set via the admin UI and should remain in config)

Change looks good, not sure if you normally require update path tests for this module or not, other than that - looks good

Like I mentioned previously, it's really important that we get this right now because it will be difficult to make similar changes once 8.x-2.x is stable.

If you create service or wrapper around Drupal::state()->get('acquia_subscription_data') (for example) then you can change implementation later without bothering downstream.

I think a service that contains the logic and gets the state service injected behind the scenes does sound like a good idea. It would have been possible to do in 1.x if that had been the case.

A wrapper service does sound nice to have, but my biggest concern is moving additional settings out of configuration and into state, which a wrapper service won't help with. I also wasn't able to find any examples of custom or contributed code in D8 that access these settings.

Without any evidence that people would actually use such a wrapper, it feels premature. I appreciate the irony that I was the first person to suggest it in this thread two years ago :)

Committed to 8.x-2.x, backporting to 8.x-1.x. I also tweaked the update function in the 2.x branch to be idempotent for upgrading users.