Move Settings into Drupal\Core\Site\Settings

Can you explaing in what way Settings is a Drupalism? I am quite astonished by that statement, to be honest.

It is quite a common pattern for PHP projects to include a PHP file that exposes configuration variables in file scope that are then exported to global variables for use in the rest of the project. I.e. $conf in Drupal <=7. Settings is just a wrapper around that which nicely implements the Singleton pattern, because that's exactly what such variables are.

I can totally see other projects using this in exactly the same way we do in drupal_settings_initialize().

I obviously have nothing against *extending* the Component Settings in Core to provide things like Settings::getHashSalt() as discussed in #2207585: Find a new OO home for drupal_get_hash_salt() but I would like some rationale for

Drupal\Component\Utility\Settings is a Drupalism and thus has no place in Component.

before I believe it.

I don't want to hold this up, as I'm not married very much to \Drupal\Component, but FTR I disagree 1000% with #7.

1. +++ b/core/core.services.yml
   @@ -128,8 +128,8 @@ services:
   +    class: Drupal\Core\Site\Settings
   +    factory_class: Drupal\Core\Site\Settings
   

   Is there any reason why we need to specify both the factory_class and class?

2. +++ b/core/includes/bootstrap.inc
   @@ -2,7 +2,7 @@
   -use Drupal\Component\Utility\Settings;
   +use Drupal\Core\Site\Settings;
   

   Given the definition of components: "Drupal Components are independent libraries that do not depend on the rest of
   Drupal in order to function." I don't really see a reason why this should not be a component.

3. +++ b/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageTest.php
   @@ -8,7 +8,7 @@
   diff --git a/core/tests/Drupal/Tests/Component/Utility/SettingsTest.php b/core/tests/Drupal/Tests/Component/Utility/SettingsTest.php
   
   index 7117bcb..ec51380 100644
   --- a/core/tests/Drupal/Tests/Component/Utility/SettingsTest.php
   
   --- a/core/tests/Drupal/Tests/Component/Utility/SettingsTest.php
   +++ b/core/tests/Drupal/Tests/Component/Utility/SettingsTest.php
   
   +++ b/core/tests/Drupal/Tests/Component/Utility/SettingsTest.php
   @@ -2,18 +2,18 @@
   - * Contains \Drupal\Tests\Component\Utility\SettingsTest.php
   + * Contains \Drupal\Tests\Core\Site\SettingsTest.php
     */
   ...
    namespace Drupal\Tests\Component\Utility;
   

   Oh, we don't move the tests here.

If you are strict you should als move Xss::filter out of components, as external code couples

s. However, the current Settings class fails on all points: (1) It claims to implement the singleton pattern, but isn't, because a true singleton is not possible in Drupal due to simpletests; the Settings class is re-instantiated all over place throughout core.

It would be interesting to know how many places actually require settings() to be a singleton and not being part of the DIC.

Every PHP project has its own concept for application level settings already, so there is no point to expose this utility class in Component.

Do you have examples how other projects solve things like this?, just asking curiosly.

I kinda agree that such a piece of code should be clearly marked as being part of core and not something really generic people should try to reuse.

i also completely disagree with this

It claims to implement the singleton pattern, but isn't, because a true singleton is not possible in Drupal due to simpletests; the Settings class is re-instantiated all over place throughout core.

Well then thats drupal's problem and not the component's?

Every PHP project has its own concept for application level settings already, so there is no point to expose this utility class in Component.

I want to use this component in my custom 100 lines of code website and Settings component sounds like a great candidate for that because it is lightweight. Why only other php projects can only re-use our components? any php codebase should be able to pull this from composer, not just other frameworks

We identified that it would make sense to enhance the class with specific methods in #2207585: Find a new OO home for drupal_get_hash_salt(), which are custom to Drupal.

I dont see whats wrong with settings()->get('hash_shalt')

Also if we are still doing this: moving this to Core namespace would require to move all components that require it to moved too. (DiffEngine and PhpStorage)

Re ParisLiakos:

We identified that it would make sense to enhance the class with specific methods in #2207585: Find a new OO home for drupal_get_hash_salt(), which are custom to Drupal.

I dont see whats wrong with settings()->get('hash_shalt')

It was identified in the quoted issue that we want to add additional validation code to that getter, to throw an exception in case 'hash_salt' is empty.

As I referenced above, though:
A: I completely agree with you (you == ParisLiakos)
B: I don't see why we couldn't add that Settings::getHashSalt() in a CoreSettings extends ComponentSettings class.

ah i see..CoreSettings would work ..
Or check before even creating the settings object against all the required keys in drupal_settings_initialize and throw the exception there..

I'd question why this is RTBC with two people disagreeing with the concept, but that's a bit irrelevant given

moving this to Core namespace would require to move all components that require it to moved too. (DiffEngine and PhpStorage)

would need to be resolved before this is committed.

I don't understand how this class "depends on the site directory from which settings.php is read". drupal_settings_initialize() depends on that, but you could create a new Settings instance with an array from wherever you wanted to (some tests create a Settings instance with their own custom arrays). It's a totally self contained class, so Component seems appropriate.

I'll put my thoughts about hash_salt on that issue.

My 2 pence on this issue:

• The use of Settings by Diff is a best spurious - solved by #1848266: Convert Diff into a proper, PSR-0-compatible PHP component
• I think we should move of PhpStorageFactory to \Drupal\Core too
• whilst there is nothing Drupal specific about the Settings implementation it is our Application's settings singleton so having this in Component is odd too - who is honestly going to want to reuse this.

needs re-roll for #2219009: Improve DX of Settings class

Re-roll because of the following conflict:

diff a/core/includes/session.inc b/core/includes/session.inc	(rejected hunks)
@@ -17,7 +17,7 @@
  */
 
 use Drupal\Component\Utility\Crypt;
-use Drupal\Component\Utility\Settings;
+use Drupal\Core\Site\Settings;
 use Drupal\Core\Session\UserSession;
 use Drupal\Core\Utility\Error;
 

Fixed one occurrence and test annotations.

25: drupal_2208475_25.patch queued for re-testing.

25: drupal_2208475_25.patch queued for re-testing.

+++ b/index.php
@@ -8,6 +8,8 @@
+use Drupal\Core\Site\Settings;
+
 require_once __DIR__ . '/core/vendor/autoload.php';
 require_once __DIR__ . '/core/includes/bootstrap.inc';

Apparently it does, but how does the "use" statement work before the autoloader has been loaded?

The use statement doesn't do anything immediately, it just says "Should you, at some point in the future, need a class called Settings, this is the one I mean." As long as you don't actually attempt to use the Settings class before the autoloader has been defined it should be fine.

BTW my concerns about this change have been addressed, but I'd still like to see the first two bullets of #19 done before this gets in, if only to avoid other Components from copying those two and depending on Core code.

Re #34. Ahh thanks a lot for that explanation. Now I can check the "Learn something new everyday" box for today :-)

Still fine (under the assumption that we actually want the move).

Fixing the phpunit test is fine.

Updating patch with re-roll. Please review.

Needs a small re-roll. Weird that this was ever in Component to begin with.

+++ b/index.php
@@ -8,6 +8,8 @@
+use Drupal\Core\Site\Settings;
+

@@ -16,7 +18,7 @@
-  if (\Drupal\Component\Utility\Settings::get('rebuild_access', FALSE)) {
+  if (Settings::get('rebuild_access', FALSE)) {

That's a bit unfortunate. I'd love to keep index.php lean and mean and copy/pastable without use statements.

+++ b/index.php
@@ -16,7 +18,7 @@
+  if (Settings::get('rebuild_access', FALSE)) {

Mh, looks like this was added without a use for some reason, in https://drupal.org/node/2219009#comment-8605979 - Thanks!

Looks good.

There are change records that will need to be corrected for this:
https://drupal.org/list-changes/published/drupal?keywords_description=Dr...

Please make sure they get updated when this goes in. :)

Needs a reroll.

git ac https://drupal.org/files/issues/settings.site_.46.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 41110  100 41110    0     0  23837      0  0:00:01  0:00:01 --:--:-- 34721
error: patch failed: core/includes/install.core.inc:1
error: core/includes/install.core.inc: patch does not apply

Committed 8a56782 and pushed to 8.x. Thanks!

diff --git a/core/includes/update.inc b/core/includes/update.inc
index a7dabc5..694a8a9 100644
--- a/core/includes/update.inc
+++ b/core/includes/update.inc
@@ -14,7 +14,6 @@
 use Drupal\Core\Config\ConfigException;
 use Drupal\Core\DrupalKernel;
 use Drupal\Core\Page\DefaultHtmlPageRenderer;
-use Drupal\Core\Site\Settings;
 use Drupal\Core\Utility\Error;
 use Drupal\Component\Uuid\Uuid;
 use Drupal\Component\Utility\NestedArray;
diff --git a/core/lib/Drupal/Core/Authentication/Provider/Cookie.php b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
index 9b1f99d..167a0b0 100644
--- a/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
+++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
@@ -9,7 +9,6 @@
 
 use Drupal\Core\Authentication\AuthenticationProviderInterface;
 use Drupal\Core\Session\SessionManagerInterface;
-use Drupal\Core\Site\Settings;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
 
diff --git a/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageTest.php b/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageTest.php
index 8e7d7c2..58cd7ca 100644
--- a/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageTest.php
+++ b/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageTest.php
@@ -7,8 +7,6 @@
 
 namespace Drupal\Tests\Component\PhpStorage;
 
-use Drupal\Core\Site\Settings;
-
 /**
  * Tests the directory mtime based PHP loader implementation.
  */

Remove all these on commit because fixing unused use statements is silly.

Fixed change notices

For the record my objections to this are now moot, as #1848266: Convert Diff into a proper, PSR-0-compatible PHP component and #2210631: Move PhpStorageFactory to the Core namespace have been committed.