Stop storing Settings singleton in object properties

I do wonder many how much of settings.php should really be container parameters, if we are to follow Symfony more closely. We have too many places to store configuration :)

e.g.

# $settings['http_client_config']['proxy']['http'] = 'http://proxy_user:proxy_pass@example.com:8080';
# $settings['http_client_config']['proxy']['https'] = 'http://proxy_user:proxy_pass@example.com:8080';
# $settings['http_client_config']['proxy']['no'] = ['127.0.0.1', 'localhost'];

This is only used in \Drupal\Core\Http\ClientFactory so could just be injected from the container.

Similarly the reverse proxy settings could be injected into the relevant services as container parameters, omit_vary_cookie is only used in one event subscriber, cache_ttl_4xx could even be config for the page_cache module, file_public_base_url could be injected into the PublicStream wrapper, etc.

Duplicate of (or closely related to) #2443351: Ensure that settings can't be serialized by not injecting them / replace stuff with container parameters.

See also #3061535: Add settings from settings.php to the container as parameters

I do wonder many how much of settings.php should really be container parameters

IMO this is a case of older-school Drupalism that we've hung on to, far too long. In my custom (and contrib, when I'm writing greenfield) code, it's all container parameters all the time. Paired with a module like services_env_parameter, this works really well. The challenge is that Drupal has its own version of the Yaml parser (which I've filed some issues relating to, elsewhere) so setting parameters in a 12-factor way akin to what Symfony core provides is a blocker to sunsetting $settings. But it's for sure a noble cause.

1. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   @@ -36,14 +29,11 @@ class CacheFactory implements CacheFactoryInterface, ContainerAwareInterface {
      /**
       * Constructs CacheFactory object.
       *
   -   * @param \Drupal\Core\Site\Settings $settings
   -   *   The site settings.
       * @param array $default_bin_backends
       *   (optional) A mapping of bin to backend service name. Mappings in
       *   $settings take precedence over this.
       */
   -  public function __construct(Settings $settings, array $default_bin_backends = []) {
   -    $this->settings = $settings;
   +  public function __construct(array $default_bin_backends = []) {
        $this->defaultBinBackends = $default_bin_backends;
      }
    
   

   We'd normally ask for a 'best effort' bc layer for changes like this, but this is a tricky one.

   Would have to:

   - remove the type hint
   - check if it's a Settings object
   - get $default_bin_backends using func_get_args() if not.

   Maybe we can grep contrib for CacheFactory to see if anything's inheriting from it, and if not just leave it?

   Some more similar ones like this where it's not the last argument.

2. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
   @@ -35,15 +28,12 @@ class DatabaseBackendFactory implements CacheFactoryInterface {
       *
       * @throws \BadMethodCallException
       */
   -  public function __construct(Connection $connection, CacheTagsChecksumInterface $checksum_provider, Settings $settings = NULL) {
   +  public function __construct(Connection $connection, CacheTagsChecksumInterface $checksum_provider) {
        $this->connection = $connection;
        $this->checksumProvider = $checksum_provider;
   -    $this->settings = $settings ?: Settings::getInstance();
      }
    
   

   Ones like this on the other hand we can just ignore entirely since it's the last argument.

(reviewing the 10.x patch).

The bc layers all look good to me.

Main question is this mismatch in the deprecation versions:

1. +++ b/core/core.services.yml
   @@ -463,12 +462,12 @@ services:
      settings:
        class: Drupal\Core\Site\Settings
        factory: Drupal\Core\Site\Settings::getInstance
   +    deprecated: The "%service_id%" service is deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Site\Settings instead. See https://www.drupal.org/node/3306646
      state:
        class: Drupal\Core\State\State
   

   This says deprecated in drupal:9.5.0 and removed in drupal:10.0.0

2. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   @@ -36,14 +29,16 @@ class CacheFactory implements CacheFactoryInterface, ContainerAwareInterface {
   -    $this->settings = $settings;
   +  public function __construct($default_bin_backends = []) {
   +    if ($default_bin_backends instanceof Settings) {
   +      @trigger_error('The settings service should not be passed to ' . __METHOD__ . ' since drupal:9.5.0. The service will be removed in drupal:11.0.0. See https://www.drupal.org/node/3306646', E_USER_DEPRECATED);
   +      $args = func_get_args();
   +      $default_bin_backends = $args[1];
   +    }
   

   But this says deprecated in 9.5.0 and removed in 11.0.0.

We broadly have three options I think:

1. Deprecate in 9.5.0 and removed in 10.0.0 - disadvantage is it will potentially break modules that are already marked compatible with 10.0.x if they inject the service themselves or modify one of the services if we remove the constructor bc in 10.x

2. Deprecate in 10.1.x and remove in 11.0.0 (but with all the other service/constructor changes committed to 9.5.x) - less likely to break modules but also means that modules could end up with issues on PHP 8.2 and without any useful notice from us what the problem is.

3. Deprecate in 9.5.x for removal in 11.x - IMO this is the least worst option. Modules that are already compatible with 10.0.x won't break, they'll just get a new deprecation, everyone gets as much warning as possible about the change to help with PHP 8.2 compatibility. Also means least churn between 9.5 and 10.x just before beta.

Will ping other committers to get additional opinions too.

Latest wording looks good to me for the constructor deprecations.

Tagging

@andypost what are the actual issues with having this as a service? The IS doesn't explain, and the parent issue just says "after PM discussion with chx" but doesn't give an explanation either that I can see.

How does the BC promise work if we are removing arguments? If someone has subclassed one of these services and swapped it in, and then relies on @settings being passed in, or $this->settings being set correctly, what happens?

Also if we are going to convert settings to container parameters, I wonder if we should be removing the constructor arguments here, as maybe we could switch @settings for %some_container_parameter%? In that case maybe we should pass NULL in instead for now?

1. +++ b/core/modules/system/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
   @@ -311,7 +301,8 @@ protected function isApplicable(SecurityAdvisory $sa): bool {
   +    $withHttpFallback = Settings::get('update_fetch_with_http_fallback', FALSE);
   

   This variable should be snake_case (or just inline it into the if statement).

2. +++ b/core/phpstan-baseline.neon
   @@ -1483,11 +1483,6 @@ parameters:
   diff --git a/core/phpstan-baseline.neon b/core/phpstan-baseline.neon.orig
   
   diff --git a/core/phpstan-baseline.neon b/core/phpstan-baseline.neon.orig
   similarity index 100%
   
   similarity index 100%
   copy from core/phpstan-baseline.neon
   
   copy from core/phpstan-baseline.neon
   copy to core/phpstan-baseline.neon.orig
   

   This shouldn't be in the patch.

+++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
@@ -44,10 +42,10 @@ class ChainedFastBackendFactory implements CacheFactoryInterface {
    */
-  public function __construct(Settings $settings = NULL, $consistent_service_name = NULL, $fast_service_name = NULL) {
+  public function __construct($consistent_service_name = NULL, $fast_service_name = NULL) {
     // Default the consistent backend to the site's default backend.
     if (!isset($consistent_service_name)) {
-      $cache_settings = isset($settings) ? $settings->get('cache') : [];
+      $cache_settings = Settings::get('cache', []);
       $consistent_service_name = $cache_settings['default'] ?? 'cache.backend.database';
     }

This looks like it also needs the func_get_args() treatment.

Apart from that, all looks great.

If we did #3061535: Add settings from settings.php to the container as parameters first then we'd have way less constructor change. Because we could then use this issue to properly inject the container parameters.

I'm not sure about #3061535: Add settings from settings.php to the container as parameters at least in its current state:

- has to special case hash_salt to avoid it getting written to the database, but there can be similar custom/contrib settings you might also not want to get into the db.

- it's not impossible that people put downright strange things into $settings that would break.

I think we have two possible approaches here:

1. Keep settings.php in its current form and inject all (or almost all) settings into the container automatically
2. Manually convert each setting into a separate container parameter and slowly deprecate (most of?) settings.php entirely

Not sure which is the best route.

Given that Symfony has explicit support for injecting environment vars as container parameters, and also when dumping a container it can dump the original reference to the env var name (instead of the env var value), we could leverage or duplicate this technique for injecting settings variables?

See https://github.com/symfony/dependency-injection/blob/c1831682d0a02ee2ce0...

I just learned that the Drupal 10 beta will likely be held up over this. So I'm trying to find my bearings.

• @alexpott seems concerned in #52 about the direction here, and points to #3061535: Add settings from settings.php to the container as parameters as something he believes should happen first.
• @catch seems to disagree with @alexpott in #53.
• @longwave seems to learn towards what @alexpott suggested in #52, providing a helpful Symfony example we could borrow from.
• @andypost is +1 in #57 but then links to another issue: #2443351: Ensure that settings can't be serialized by not injecting them / replace stuff with container parameters..

I think it'd be helpful if somebody who's familiar with this area could suggest concrete next steps. I'm also fine with @catch and @alexpott as core committers simply deciding a direction given the very narrow timeframe before Drupal 10 beta.

Discussed this with @alexpott at Drupalcon. We talked about a number of issues:

1. settings.php sometimes contains dynamic code, often to set something based on the environment. Modern web applications generally just inject environment variables to handle this, e.g via .env or into a container, so we should ensure that Symfony's container parameter injection can work in Drupal.
2. services.yml files need to declare all child parameters inside a top level key. Ideally we would have a way of providing a fallback/default, allowing contrib to augment parameters, and then allowing the site owner to override these further in their site settings.yml. But merging YAML is non trivial, there is no easy way to remove keys, etc. - this is a difficult problem to solve.
3. We should review all existing uses of Settings::get(), look at converting some of the easier ones to container parameters, and adding a deprecation layer where we can inform users of where to migrate their settings to.

@andypost we will be in the contribution room for most of today if you want to discuss this!

#61.1 has some related issues already:

• #3249970: Support setting service parameters via environment variables
• #2879846: Provide better support for environment variables
• #2501901: Allow configuring the environment via an environment variable
• #2979749: Allow to pick up settings & DB creds from environment variables

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.