Add a Settings storage object

Makes sense to me in general. To think through this verbosely:

Since Settings are read-only, theoretically it should be safe and should not matter that the services are getting the value object injected.

The proposed construct may cause those injected objects to diverge from the object in the singleton. Special attention to serialization scenarios.

Wondering whether we can proactively prevent that somehow? Could we implement Serializable and make SettingsStorage::unserialize() instantiate itself by calling Settings::getStorage()?

Aside from the above, minor: Not sure whether SettingsStorage is the best name; it's not really a storage, but a fancy hashmap / custom ArrayObject-alike value object. How about SettingsBag?

Generally I think this looks like a good plan. Gives us a bit more flexibility. We started using Settings:get() directly in code, but this could enable passing it back as param where it makes sense. This can be a follow up though, for sure.

Would be interested to see what sun thinks about this. I know he wanted to do something with the current state of settings. x post :)

Couple of nits:

1. +++ b/core/lib/Drupal/Core/Site/SettingsStorage.php
   @@ -0,0 +1,58 @@
   +   * Constructor.
   

   I am fine with that. Not sure if we officially have to use 'Constructs a..' ?

2. +++ b/core/lib/Drupal/Core/Site/SettingsStorage.php
   @@ -0,0 +1,58 @@
   +   *   The value of the setting, the provided default if the value is not set.
   

   I think we can squeeze an ", or .." on that line too?

SOrry

Existing issues regarding the serialization problem space mentioned in #2.

Several issues also discussed that our ideal end goal should actually be to remove the singleton pattern from Settings — at which point the Settings class itself would turn into the bag / value object.

In essence, the idea would be:

core/lib/Drupal/Core/DrupalKernel.php
@@
-  public function __construct($environment, ClassLoader $class_loader, $allow_dumping = TRUE) {
+  public function __construct($environment, ClassLoader $class_loader, SettingsBag $settings, $allow_dumping = TRUE) {
+    $this->settings = $settings;

@@
   protected function attachSynthetic(ContainerInterface $container) {
+    $container->set('settings', $this->settings);

Now that I think of it, that approach would actually inject this new SettingsBag value object, so this patch here seems to cleanly contribute towards that goal. :-)

@neclimdul: Can you re-roll to address #3 + potentially rename to SettingsBag?

Looks like this is a quick win.

How about reusing Symfony\Component\DependencyInjection\ParameterBag\ParameterBag and [...]\FrozenParameterBag?

@znerol
Well, sure that class would fit quite well, but it feels wrong to depend on a DependencyInjection class.

+++ b/core/lib/Drupal/Core/Site/SettingsBag.php
@@ -0,0 +1,58 @@
+/**
+ * Read only settings data object.
+ *
+ * @ingroup utility
+ */
+class SettingsBag {

Did we considered to introduce a Bag component which has no dependency at all, and settings would just use it?

Patch no longer applies.

We will take this issue on Drupal Mumbai code sprint today.

Cleaning unwanted tags

We could add a new settings system alongside the old one, both can take from $settings in settings.php. Move usages to the new one and deprecate the old one. Ugly but not impossible in a minor release, so moving back.

Rerolled against 8.3.x, please review =)

Files that had conflicts:

• core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
• core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorTest.php
• core/tests/Drupal/Tests/Core/EventSubscriber/ReverseProxySubscriberUnitTest.php (deleted)
• core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php
• core/modules/language/src/LanguageNegotiator.php
• core/modules/language/src/HttpKernel/PathProcessorLanguage.php
• core/lib/Drupal/Core/Session/SessionManager.php
• core/lib/Drupal/Core/Routing/UrlGenerator.php
• core/lib/Drupal/Core/EventSubscriber/ReverseProxySubscriber.php (deleted)
• core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php

These files no longer use Drupal\Core\Site\Settings; so I've removed the changes that we were doing there:

• core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
• core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorTest.php
• core/modules/language/src/HttpKernel/PathProcessorLanguage.php
• core/lib/Drupal/Core/Session/SessionManager.php
• core/lib/Drupal/Core/Routing/UrlGenerator.php

Files that previous patch was making changes but are now deleted:

• core/tests/Drupal/Tests/Core/EventSubscriber/ReverseProxySubscriberUnitTest.php
• core/lib/Drupal/Core/EventSubscriber/ReverseProxySubscriber.php

Here is the error that the bot found apparently:

13:51:46 PHP Catchable fatal error:  Argument 2 passed to Drupal\Core\File\FileSystem::__construct() must be an instance of Drupal\Core\Site\Settings, instance of Drupal\Core\Site\SettingsBag given in /var/www/html/core/lib/Drupal/Core/File/FileSystem.php on 
line 55