Remove immutable config from default form values [was: Move API credentials to config]

@aaronbauman

But, you can override state vars from settings.php similar to config...

I'm not finding this to be the case. If I set one of these in my settings.php file as follows:

$settings['salesforce.consumer_key'] = 'foo';

This has absolutely no effect on what comes back from a drush eval "var_export(\Drupal::state()->get('salesforce.consumer_key'));". What's in my database always comes back - and if I clear the stored value from the DB I get back NULL, regardless of what's in my settings.php file. This corresponds to what I see on the Salesforce authorize form, which I understand to use the same \Drupal::state()->get().

If instead I issue a drush eval "var_export(\Dr\Core\Site\Settings::get('salesforce.consumer_key'));", I do see the "foo" value come back.

Am I missing something? Is this actually working for you?

If not, my proposal is to shift these three API configuration items (salesforce.consumer_key, salesforce.consumer_secret, and salesforce.login_url) from state storage into a storage system that can be easily overridden per environment. Originally I was thinking of just having this stuff defined in settings.php (no form fields / no db storage). I think this could would work well, but might be disruptive to the existing installations that won't have anything defined in settings.php. I suppose we could look first for a setting and fall back to state when that doesn't exist. Looking at other modules I see a pretty big trend toward utilizing config (which does allow settings.php overrides - so this would work too). Config is nice in that a module update hook may be able to move the existing state values into the site's active config, and would still allow for setting the values in the authorize form, which users are accustomed to. You could use settings.php to get the job done, but you wouldn't have to.

Anyway just ideas - thoughts?

Maybe a primary source in Settings, with fallback to State would work?

This certainly seems like the simplest solution in terms of implementation and it should have zero impact on existing installations.

My only hesitation with this approach is that every module I've analyzed that also deals with this where-to-store-the-api-keys space is using config. Here are some examples (think of any API-connected Drupal module you've used and you may find the same):

- Commerce stripe
- Commerce paypal
- S3 filesystem
- Mailchimp

Talking with others at DrupalCon it's clear config can be used to supply API keys to Drupal in a way that keeps the (real) keys out of version control and out of the database:

Without contrib:

1. Save and export dummy/placeholder values for the API keys - like "[value-supplied-via-config-override]" or "0000" - you get the idea. These non-sensitive values do get tracked in version control. Or if the API fields are optional you just don't put values into the fields.
2. Settings.php / settings.local.php is used to replace those placeholder/empty values with real ones (currently, awkwardly the overridden values or even the fact that overrides are in place will not show in the UI - but there's a fix in the works for this). See Configuration override system for details and why overridden values aren't shown as default values in config form fields.

With contrib:

While I haven't personally verified this to work, it sounds like folks are using Config ignore in combination with Settings.php / settings.local.php config overrides to completely keep sensitive configuration (like API keys/endpoints) out of their codebase. Again, I haven't verified, but it sounds like dummy/placeholder values aren't necessary because those sensitive items you've config-ignored are not included in the exported config files at all.

Anyway that's what's pulling me toward config here. I don't love it, but it is what the larger community appears to be doing - and it does allow for setting / updating the values through the UI, overriding the values in settings.php, and (in round-about way) keeping the values out of version control (if you know what you're doing).

Thoughts? Personally I'm on the fence...

Added login URL to what should move into config (login URL also changes for us as we move from prod to staging & development environments - so having that in config means we can override that per environment too).

Patch attached. Working well for me, but certainly needs review.

Also wasn't sure where to include details about this in the salesforce_example module, but I've got a new section in the readme dedicated to this.

Anyway let me know what you think and if you have any questions about what's going on in here. Thanks!

Whoops sorry it looks like I broke some tests - updated patch coming soon...

Yeah it seemed like the config object in the test was stale. I think this will pass now.

Good catch. I reworked Drupal\salesforce_encrypt\Rest\RestClient so that it focuses more on becoming an encryption/decryption layer atop its parent Drupal\salesforce\Rest\RestClient, rather than doing direct storage and retrieval itself. It no longer cares where each value ends up or is sourced from (state vs config - that's left to the parent to decide), so the move to config no longer breaks it. Should make ongoing maintenance / adjustments easier as they'll cascade better from parent to child (rather than needing to be repeated in both).

Anyway this is working well for me in a test environment but it certainly needs review and more testing.

Awesome! Thanks for bringing this in. We're using it across a number of environments and it seems to be working well.

One concern with the change to RestClient::getConsumerKey and ::getConsumerSecret for default values in the auth form:

RestClient::getConsumerKey and ::getConsumerSecret return immutable config, which means overrides come through. Populating the form with overrides (immutable config) means overrides can creep into config any time someone authorizes a site. It's basically what the new config system is trying to eliminate:

...Drupal 7 had the global $conf variable that was usually populated in settings.php with conditional override values for configuration. A big drawback of that system was that the overrides crept into actual configuration. When a configuration form that contained overridden values was saved, the conditional override got into the actual configuration storage.

- Configuration override system doc page

Suggestion: We show (mutable / non-overridden) config values here in the form, decrypted if necessary.

I'll try and follow with a patch here soon so you can see better what I'm proposing. I think it'll be a pretty simple/small change.

Attached patch shows mutable / non-overridden config values in the auth form, decrypted when necessary. Needs review.