Find a new OO home for drupal_get_hash_salt()

I fixed some docs in the test.

I also think this may not be the right place, or at least not for the code in its current form, as Settings is a component and should therefore be useful and make sense outside the context of Drupal. Adding a method for a very specific setting is questionable, but referring to Drupal-specific use cases (settings.php) is not helpful to outsiders at all.

Wasn't @sun working on a site service? Maybe this would fit in there.

Moar changes.

Sorry, but I don't really see how we can add that code given that Settings currently lives in Component. What we could do is pass a list of required properties in __construct() and have Settings::get() throw an exception in case a required property is missing. That would make it generic enough for people to re-use outside of Drupal.

Actually that would be kind of pointless as well, as you obviously know all of the settings' keys when calling Settings::__construct(), so making Settings throw an exception on a missing key would be weird.

Instead we should throw an exception as soon as we initialize Settings if 'hash_salt' is missing.

1. Settings is completely misplaced in Component — it's a 100% Drupalism and we need to move that into Core ASAP. My plan was to move it into Drupal\Core\Site\Settings, which is also where the new Site singleton will be located, because Settings are always loaded from a particular site directory.

2. Adding dedicated methods for settings that are required to exist makes perfect sense to me. We should consider to add more of them later on. This approach would allow us to "namespace" settings in settings.php and provide dedicated methods to retrieve them, automatically applying defaults; e.g., Settings::getTwig(), etc.

In short, the proposed patch looks good and makes sense to me.

The only part that I don't like is that the method is not static — but to change that, we first have to convert Settings into a proper singleton, because the current code and usage is a total mess. Pretty much following the code of the new Site singleton in #1757536: Move settings.php to /settings directory, fold sites.php into settings.php → this may or may not happen in #2199795: Make the Settings class prevent serialization of actual settings

So for now, the approach of the patch is in line with the current "architecture" of the Settings class, and thus fine.

I'd like to see a few replacements as part of this patch though... At minimum, drupal_get_hash_salt() itself should call the method. :)

I really wonder whether we could special case the logic to initialize the APC classloader.
For all the other usecases it could be worth to try out container parameters for that.

Created #2208475: Move Settings into Drupal\Core\Site\Settings

Thanks — looks great to me! :)

i feel that this should be postponed to #2208475: Move Settings into Drupal\Core\Site\Settings
if we end up moving it to core, we should actually move it first, and then add this method.
if not, well there are some alternatives posted in #2208475: Move Settings into Drupal\Core\Site\Settings

This approach would still allow people to call settings()->get('hash_salt') without triggering the exception and is confusing DX: How do people know when to call ->getSettingName() and when to call get('setting_name')?

Actually that would be kind of pointless as well, as you obviously know all of the settings' keys when calling Settings::__construct(), so making Settings throw an exception on a missing key would be weird.

Instead we should throw an exception as soon as we initialize Settings if 'hash_salt' is missing.

Erroring on construction is an interesting idea, but it assumes that the setting is required for the site to work at all. That's probably true for hash_salt, as it's required for fundamental things such as form handling, but might not be true in all cases. If we go down this route, we should ensure that it's possible to add an 'error on use' option if we need it in the future.

we can move on here now

yes indeed we have duplicate methods..the only difference is the fact of being static..
so there are two paths
Settings::get()
and
$this->settings->getSetting()

I find it very confusing tbh..I believe that (i didnt test it) $this->settings->get() would be the exact same thing of calling $this->settings->getSetting()

+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -70,6 +70,26 @@ public static function getAll() {
+    $hash_salt = static::$instance->get('hash_salt');

hmm the whole other class uses self:: and i think it makes sense because this class is final ( :( )
But anyway lets keep the consistency and use self here too

Just a silly thought: (to be ignored)

One potential way to address that concern would be:

  public function __call($name, $args) {
    // Check whether the method exists literally.
    if (method_exists($this, $name)) {
      return $this->$name($args[0]);
    }
    // Check whether the method name maps to a setting.
    elseif (isset($this->storage[$setting = substr(Container::underscore($name), 3)])) {
      return $this->storage[$setting];
    }
    // Otherwise, fall back to a literal get().
    else {
      return $this->get(substr($name, 3));
    }
  }

Dunno why I'm posting this, but yeah, the idea is flawed, so please ignore this, unless you can make something more fruitful out of it :-)

looks good to good, besides one thing which i also mentioned in #19

+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -70,6 +70,26 @@ public static function getAll() {
+    $hash_salt = static::$instance->get('hash_salt');

hmm the whole other class uses self:: and i think it makes sense because this class is final ( :( )
But anyway lets keep the consistency and use self here too

no problem:) thanks for your patience!

Hm. I assume/fear that the injected Settings will be serialized much more often with this change, since the CsrfTokenManager gets injected into every form...

#2199795: Make the Settings class prevent serialization of actual settings

It looks like #2201919: Replace drupal_get_hash_salt() with direct Settings call in CsrfTokenGenerator was merged into this issue?

Wondering whether it is necessary to perform that change here? Can we make the constructor call Settings::getHashSalt() instead?

The problem with serialized settings is that they are typically unserialized in a completely different request/context. settings.php might have been changed, but the unserialized Settings class still holds the former settings.

Thanks.

That appears to be the only safe/secure approach for now. We'll have to study and investigate this whole situation of serialized settings some more - which could use a critical release-blocking issue, because unserialization and usage of possibly stale/outdated settings is a security issue.

No longer applies.

Also shouldn't we mark drupal_get_hash_salt() deprecated here?

Part of me thinks twice about this because it further cements that anything with a dependency on Settings has to stay in \Drupal\Core and can't be a component, however:

1. OOP code should not be calling out to procedural code. If we'd stuck to that rule I think we'd have saved ourselves quite a lot of pain this release cycle with hidden dependencies.

2. Like the Cache system, the settings dependency can always be restricted to a factory, then only the factory has to be in \Drupal\Core not the entire subsystem.

Committed/pushed to 8.x, thanks!