Ensure that settings can't be serialized by not injecting them / replace stuff with container parameters.

this makes a difference locally

and so does this

1. +++ b/core/core.services.yml
   @@ -5,6 +5,11 @@ parameters:
   +  session_write_interval: 180
   

   I recently saw that we have a SessionConfiguration service, that makes the session configuration stuff easier to access, just wondering if it would make sense to put that in there as well?

2. +++ b/core/core.services.yml
   @@ -277,7 +282,7 @@ services:
   -    arguments: ['@settings']
   

   Do you plan to actually remove it from the container?

   We tried to be as nice as possible with the error when the container is serialized (php notice/warning), but at the moment that is pointless because it always results in an exception due to settings being serialized as well.

3. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -48,19 +40,31 @@ class FileSystem implements FileSystemInterface {
   +    $file_chmod_configuration += [
   +      'directory' => static::CHMOD_DIRECTORY,
   +      'file' => static::CHMOD_FILE,
   +    ];
   +    $this->fileChmodConfiguration = $file_chmod_configuration;
   

   Are there more keys than those two? Should we maybe make them separate properties instead of accessing it as an array?

4. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -23,12 +22,11 @@ class MetadataBag extends SymfonyMetadataBag {
   +  public function __construct($session_write_interval = 180) {
   

   does the default value like this actually make sense? if it's always called through the container, not so much?

   Maybe it should be an if (empty) set default check?

5. +++ b/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php
   @@ -23,23 +22,28 @@ class ReverseProxyMiddleware implements HttpKernelInterface {
   +    $reverse_proxy_configuration += [
   +      'header' => 'X_FORWARDED_FOR',
   +      'addresses' => [],
   +    ];
   +    $this->reverseProxyConfiguration = $reverse_proxy_configuration;
   

   same here, for configurations where we have a fixed set of keys, I think separate properties would be easier to write/document. We could still pass it in as an array...

6. +++ b/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php
   @@ -47,10 +51,10 @@ public function __construct(HttpKernelInterface $http_kernel, Settings $settings
   -      $proxies = $this->settings->get('reverse_proxy_addresses', array());
   +      $proxies = $this->reverseProxyConfiguration['addresses'];
          if (count($proxies) > 0) {
            $request::setTrustedProxies($proxies);
          }
   

   For example here, you could just replace $proxies with $this->proxies or proxyAddresses or something.

7. +++ b/core/modules/language/src/LanguageNegotiator.php
   @@ -190,7 +180,7 @@ protected function negotiateLanguage($type, $method_id) {
          // Check for a cache mode force from settings.php.
   -      if ($this->settings->get('page_cache_without_database')) {
   +      if (Settings::get('page_cache_without_database')) {
            $cache_enabled = TRUE;
   

   this will go away in the page_cache.module issue, but I'm not actually sure we thought of removing the constructor argument there...

8. +++ b/core/modules/system/src/Tests/Session/SessionTest.php
   @@ -228,13 +230,13 @@ function testSessionWrite() {
   +    $filename = $this->siteDirectory . '/services.yml';
   +    $data = Yaml::decode(file_get_contents($filename));
   +    $data['parameters']['session_write_interval'] = 0;
   +    file_put_contents($filename, Yaml::encode($data));
   +    YamlFileLoader::reset();
   +    $this->rebuildContainer();
   

   Don't we have a helper for this? or only for settings.php? should we have a helper?

   Just checked, didn't find anything, but might be worth it, especially considering that we might end up doing this multiple times, and the helper should be intelligent enough to remember existing settings or so?

   For example, this might overwrite the existing services.yml written in WebTestBase::setUp().

On 8., found it, there's setContainerParameter in WebTestBase.

Also forget the stuff about remember existing values, didn't notice that you read the file first.

Nice! :) Also makes the DX more consistent, by moving as much as possible out of settings.php, into the site-specific services.yml.

1. +++ b/sites/default/default.services.yml
   @@ -88,3 +88,71 @@ parameters:
   +  # factory.queue: {}
   ...
   +  # session_write_interval: 180
   

   Let's not comment these; they match the default values.

   That also makes this a bit easier to parse.

   (Right now it's very confusing to read: it's difficult to see what's a header versus explanation versus value.)

2. +++ b/sites/default/default.services.yml
   @@ -88,3 +88,71 @@ parameters:
   +  # reverse_proxy:
   ...
   +  # file_chmod:
   

   And let's add

   reverse_proxy/file_chmod:
     {}
   

   for these, for similar reasons. This is consistent with how factory.keyvalue etc. are handled.

+++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
@@ -20,9 +20,9 @@ class CacheFactory implements CacheFactoryInterface,  ContainerAwareInterface {
   protected $settings;

@@ -49,7 +49,7 @@ class CacheFactory implements CacheFactoryInterface,  ContainerAwareInterface {
+    $this->cacheSettings = $settings->get('cache');

Now no longer matching.

Re-rolled and fixed.

How is this a bug? I agree that in an ideal world these things should be container params but drupal currently works fine with them not being.

@alexpott, I believe we are going to get surprise regressions like #2502195: \Drupal\Core\StringTranslation\Translator\CustomStrings should be serializable, but contain \Drupal\Core\Site\Settings which is not until we have settings as a service in core. IMHO, this issue should be a 8.0.x bug.

I think we should look at:

- add the container parameters

- check settings first

- eventually E_DEPRECATED when the setting is found.

That lets us transition gradually to container parameters while maintaining bc.

Also using the settings singleton directly instead of the service looks fine for minor releases, even if we leave the service itself in and deprecated.

I think we can do something like #3061535: Add settings from settings.php to the container as parameters to help us move to not using settings.php.

It still makes a lot of sense for #3299828: Stop storing Settings singleton in object properties

proper status