Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Drupal\Core\Settings is a singleton class.
The self::$instance is initialized from within the constructor.
As part of #2354475: [meta] Refactor the installer, (multi)site management, and pre-container bootstrap, I would like to turn all or most of the singletons into well-behaved objects with injected dependencies.
I could start a rant about the singleton here, but this is sufficiently discussed all over the web already.
As a preparation, we should stop setting Settings::$instance from the constructor.
It is a bad idea anyway, and it makes it unnecessarily harder to refactor anything.
This could all be done at once as part of #2354475: [meta] Refactor the installer, (multi)site management, and pre-container bootstrap, but I think it is better to split as much as possible out into preparation tasks.
A follow-up will be #2363353: Refactor / split up \Drupal\Core\Site\Settings singleton, and introduce other multisite-related classes..
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 2363881_10.patch | 19.8 KB | rasikap |
| |||
| #7 | settings_not_write_Self_instance_2363881_7.patch | 19.8 KB | arunkumark |
| |||
| #2 | D8-Settings-singleton-2363881-2.patch | 42.79 KB | donquixote |
Comments
Comment #1
donquixote CreditAttribution: donquixote commentedOh-oh.
There is a number of code places where an instance of Settings is being passed around, and then
$settings->get(..)or$settings->getHashSalt()is being called.Unfortunately this is bogus, because Settings::get() is a static method, and it will look into
self::$instance->storage, not into$this->storage.This is why I think that static and non-static methods should, if possible, not live together in the same class.
Having
SettingsandSettingsInstancebe separate classes would allowSettings::get($key)and$settings->get($key)be two separate methods.("Separation of concerns" should be reason enough to keep them separate, but this might not convince everyone. The motivation above is more practical and direct.)
Comment #2
donquixote CreditAttribution: donquixote commentedRe #1 I should clarify: The "new Settings" happens only in the above mentioned test classes.
However, $settings->get() and similar happens in a number of non-test classes. They all show up in the patch.
Patch:
Introduce Settings::setCreateInstance() to replace the set-instance mechanic in Settings::__construct().
This involves a number of changes in other classes.
Comment #5
marvil07 CreditAttribution: marvil07 as a volunteer commentedThis patch cleans up and make it harder to make mistakes, it looks good!
Sadly, it has been a while, so it does not apply anymore, so making as NW for now.
Comment #6
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedComment #7
arunkumarkHi,
I have re rolled patch for Drupal 8.x- 2.x version.
Comment #8
arunkumarkComment #10
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedWas not able to apply patch. Fixed the error found.
Comment #12
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedComment #13
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #14
donquixote CreditAttribution: donquixote as a volunteer commentedJust saying:
Now that Drupal 8 has been released, we need to be careful if this change causes any BC issues.
This would apply to contrib or custom code that use this low-level settings API.
Comment #25
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Think this needs to be relooked at to see if still a valid task for D10.
Tagging for IS for any remaining tasks
Please do not just reroll