Acquia connector "Subscription" service constructor may cause downstream chaos

Let's see what the test bot thinks.

NW for test fails.

Can confirm, a handful of us have been chasing this around for months. AC is a common module between us.

I am adding more detail to the proposed resolution and replacing links to issues with tokens.

The first test failure I looked at is in ConfigureApplicationFormTest. It looks as though ConfigureApplicationForm::submitForm() calls Subscription::getSubscription(), which calls Subscription::hasCredentials(), and that function uses $this->settings directly instead of getSettings().

I think all references to $this->settings should be replaced with $this->getSettings(). (No, not the reference inside populateSettings().) That might fix a lot of the test failures.

Took a crack at this. For the most part, I agree with the changes.

I think all references to $this->settings should be replaced with $this->getSettings(). (No, not the reference inside populateSettings().) That might fix a lot of the test failures.

+1 -- one of the issues with populateSettings is that its supposed to repopulate (execute the event again) settings whenever its called. Adding the check in this method means that the first time its invoked, you could end up with the wrong settings. This is the case with tests because they call populateSettings directly after some values are changed, and the settings object gets first set when installing the module, with unfortunately null values within the object, causing it to be a valid object just with null values within it.

I've made some tweaks, we'll see if the tests are passing now.

!48 looks good to me. I understand this is basically impossible to test, but there shouldn't be any harm in delaying the collection of settings.

We've sporadically experienced weird "should never happen" things on the platform, so after this lands, I'll keep this issue in mind and chime in if another inexplicable thing (i.e. cache corruption) happens afterwards. I think that's the only reasonable evaluation we can do against the theory written out in the I.S.

Committed! Thanks everyone for your reports and testing of it.

Many thanks

Automatically closed - issue fixed for 2 weeks with no activity.