Make the Settings class prevent serialization of actual settings

The Settings class needs a major overhaul, which was already on my todo list.

You uncovered some very interesting test failures with this patch — it looks like some code is trying to store the entire Settings singleton in the database. :-(

Re-title, and bump priority

BlockFormController is serialized as a whole because of when theme is not passed via url form adds ajax call back to update region list when theme changes.
The Settings is serialized as part of core/lib/Drupal/Core/StringTranslation/Translator/CustomStrings.php so mostly all of LanguageManager have this circular dependency and other services too.
Grep '@settings' in core.service.yml.

At least this fixes failed tests, but there's other "manager" services

+++ b/core/lib/Drupal/Component/Plugin/PluginBag.php
@@ -7,10 +7,12 @@
+abstract class PluginBag extends DependencySerialization implements \Iterator, \Countable {

Well PluginBag is in Drupal\Component, and has no intrinsic knowledge of the container or services, and DependencySerialization is in Drupal\Core.

settings_as_real_singleton.patch queued for re-testing.

Yep, what Tim said. Sorry. We are already trying to un-tangle the implicit dependency mess we have between Core and Component.

So let's simply remove instantiated plugins on __sleep()

missed to return vars

I wonder whether we shouldn't simply remove the settings service from core.services.yml?

That would resolve all of the serialization problems, because it is no longer injected and stored as a class member anywhere.

The vast majority of code uses Settings::getSingleton() either way already; those injected instances are just some outliers.

Fixed failures, my bad.

@sun probably it makes sense, but anyway we need to fix uncovered troubles with plugin bag serialization,
and implement real singleton to prevent serialization of settings in future

Closes related #2246079: User error: The container was serialized. in Drupal\Core\DependencyInjection\Container->__sleep() and linked #2210521: Settings::getInstance() should throw an exception if called before being initialized

Why not do this similar to the AccountProxy, where the service is just a light-weight facade for the actual settings object?

The vast majority of code uses Settings::getSingleton() either way already;

I don't think this is something to be proud of..

And imo, a singleton or the need for it is the sign that we are doing something wrong: We don't have a solid architecture for low-level pre-container stuff. #2272129: [meta] Introduce a low-level DIC to organize low-level services would fix this.

Patch no longer applied, so reroll...

including conversion of class UpdateManager to use DependancySerialzationTrait.

__wakup() now public

makes ./core/modules/block/src/Tests/BlockTest.php pass.

Marked #2251795: Injected Settings may be serialized + unserialized later, not reflecting current settings.php values as duplicate.

I'm not sure if this is really critical, but bumping for now and the patch looks like it was pretty close?

Its been a while.

Straight reroll.

My hunch about the failing test before is killing $this->pluginInstances rather than just removing them from the export keys.

So, the __wakeup() call isn't actually doing anything. There's no way to change the identity of an object while it's being unserialized.

We could copy Settings::$storage from the singleton on __wakeup(), but that defeats the purpose of a singleton.

Or, we could create a Proxy object as per #18.

Testing to see where settings actually get serialized.

grrr I didn't mean that kind of exploding.

It seems that DependencySerializationTrait is doing its job.

Checking to see if the changes to LazyPluginCollection are necessary for this issue.

Added a simple test since the __sleep() method won't get called in normal code and I already had a bug in the one line that exists.

The stuff with serializing plugin collections could be moved to a separate issue, or perhaps it's not an issue, I'm not sure.

Is this a valid approach? I think the exception is more developer friendly than just making the __sleep() method private.

Thinking about edge cases. Are there cases when a module dev wouldn't be able to prevent Settings from being serialized? I can't think of any. Also, this doesn't prevent people from storing settings values elsewhere, and re-using the old values, but that's not something we can prevent.

Can someone explain why Stop injecting settings as a service. Just access the singleton via the Settings class if it's needed. is not implemented?
I would be happy if we could drop that change.

I agree that making the Settings singleton injectable is mostly lying, but not allowing Settings to be serialized is somewhat orthogonal to that. It would become less of an issue, but it would still be possible to store the settings in the database, which I think is a security issue.

Also, wouldn't removing the Settings service be an API change?

I would be happy if we could drop that change.

What change are you referring to?

Whichever way we go is trivial. Will await on an outcome.

If we agree that it should be completely impossible to inject the Settings service, then I'm fine with the API change. However not allowing the serialization seems like it's probably enough.

Storing an individual setting in the database isn't something we can stop, and I'm not really concerned about that - as dawehner pointed out quite a few things (especially things that affect the container) have been moved to container parameters, so there's less in Settings than there was previously, and additionally nothing stops people storing container parameters individually in the db either.

I agree that making the Settings singleton injectable is mostly lying,

If we agree that it should be completely impossible to inject the Settings service

There are two separate bad things about globals / singleton:
1. Global state in itself.
2. The explicit dependency of components on it.

If we can't get rid of 1., this does not mean we can't get rid of 2.
All we need is an injectable object that wraps the dependency to global state.
(or is it a Facade or something else?)

class SettingsWrapper implements NonStaticSettingsInterface {
  function get($key) {
    return Settings::get($key);
  }
}
class MockSettingsWrapper implements NonStaticSettingsInterface {
  private $mockSettings;
  function get($key) {
    return $this->mockSettings[$key];
  }
}

Components that depend on this always get the up-to-date settings, but they can be tested with a mock version.
This reduces the need to mess with the singleton in unit tests.

And it makes it easier to change this architecture in the future.
E.g. if we finally manage to get rid of the global state / singleton altogether.

As for the current design of the Settings class .. ouch!
We have

1. A constructor that sets the static instance. Meh!
   Yes, a lot of singletons on the planet are written like this, but it is bad nevertheless. If anything, it should be the getInstance() method that sets the static $instance variable.
2. There is no check in __construct() that would prevent the old settings being replaced by new ones.
3. We have a private non-static $storage property, but then all the methods are static and access the static::$instance->storage variable directly. This design is just pointless.
4. Since all relevant methods are static, there is really no point in passing this object around.

The proposed patch is only fighting symptoms of this.
I do not necessarily disagree with it, but it would be great to have a more far-sighted idea where this is going.
And others are also thinking beyond just the serialization problem, so this is why I am bringing this up.

Background: To not let this become a general debate about singletons, my thoughts are summarized here: http://programmers.stackexchange.com/a/247340

re-roll

Looks good.

This prevents major headaches with using the current settings singleton - we can use the other related issues to explore a better solution.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed f01bb09 and pushed to 8.0.x. Thanks!

Note: The language manager still has it injected and many more places, is that really what we want?

Added a follow up, as we are still injecting the settings service, see #2443351: Ensure that settings can't be serialized by not injecting them / replace stuff with container parameters.