Settings::getInstance() should throw an exception if called before being initialized

Settings has zero conceptual reason for being a singleton:

(1) Settings depend on the website (via the configuration path), which depends on the request.
(2) Symfony supports executing multiple requests in the same PHP environment.
(3) As a consequence, there could be different settings in the same PHP environment.

It is only a singleton by laziness, because we still haven't come up with a bootstrap process that makes sense.

I agree with the (main) problem statement of this issue:

Settings::get() should blow up with an exception upon too early access.

But the proposed patch doesn't really make sense to me.

In any case, I disagree with @Damien in #1, and here is why:

1. Yes, Settings indeed depends on the site directory, which depends on the [master] request.
2. Yes, HttpKernel allows us to execute sub-requests in the same PHP environment.
3. However, sub-requests are always to be treated internally to the Drupal site environment of the master request. It is not possible to change the site directory within a single request.
4. The site directory defines the filesystem location from which settings.php is read, and the one and only purpose of Settings is to be a read-only reincarnation of $settings variables from settings.php.

5. The site directory also defines the list of available extensions. It is not possible to change that list mid-request. Doing so would require to boot a new kernel with a completely new module list.

   In turn, a HttpKernel would have to reboot a new instance of its parent Kernel. I am very confident we do not want to get there; the current DrupalKernel reboot situation upon module installation is painful enough already.

6. As a consequence, there can only be one Settings instance in the entire lifetime of a PHP request, regardless of how many sub-requests may be issued. The Settings singleton represents the settings.php of the master request. Only the request changes, no other parts of Drupal's kernel are rebooted.

I do not see why we need a wrapper, instead of doing it directly on the method.

In any case \Drupal\Component\Utility\Settings is now \Drupal\Core\Site\Settings.

Proposed resolution: do it directly on the method (BTWgetSingleton() is now getInstance())

Added exception directly in the methods. Also removed global variables that were not used.

Using exception only on getInstance.

+1 for doing that, though we should also have a test

This improves developer experience a bit

Needs work as test are needed still

Simple unit test cases added.

Thanks for your work!

1. +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -48,9 +48,13 @@ public function __construct(array $settings) {
   +   * @throws \BadMethodCallException
       * @return \Drupal\Core\Site\Settings
   

   @thows should be after @return. There should be also additional line break between them.

2. +++ b/core/tests/Drupal/Tests/Core/Site/SettingsGetInstanceTest.php
   @@ -0,0 +1,36 @@
   +class SettingsGetInstanceTest extends UnitTestCase {
   

   Should we also test the scenario where the exception is being thrown?

Updated patch.
1) @thows should be after @return. There should be also additional line break between them.

Changed.

2) Should we also test the scenario where the exception is being thrown?

Actually, testGetInstanceWithMissingInstance() test case is testing exception thrown.

Sorry I was blind and didn't see that :) RTBC for me!

IMHO we should extend the existing test in \Drupal\Tests\Core\Site\SettingsTest

@dawehner Settings class is singleton and settings are initialized in SettingsTest::SetUp() - How would you test exception thrown then?

@dawehner Settings class is singleton and settings are initialized in SettingsTest::SetUp() - How would you test exception thrown then?

You could unset it first, can't you, at least using reflection :)

I'm working on this issue for the SpringWeekend2016 in Milan with fusillicode!!

Working on it with Ema.jar directly from Milan SprintWeekend2016 ;)

We've created a test based on reflection, as dawehner said, and added it in \Drupal\Tests\Core\Site\SettingsTest

Cleaning up drupalcampfi tags.

Great DX improvement in situations like unit tests

8.1.x has entered the RC phase and 8.0.x has entered it's last RC. As silly as it might seem something mught be relying on the NULL behaviour - however there is a DX improvement here. Therefore we should move this to 8.2.x. It is also only a task,

The alternative would be to trigger and error and maybe return a non singleton instance?

1. +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -49,8 +49,13 @@ public function __construct(array $settings) {
   +   * @throws \BadMethodCallException
   

   An @throws should document under what circumstances the exception is thrown.

2. +++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
   @@ -131,4 +131,19 @@ public function testGetApcuPrefix() {
   +    $settings = new Settings(array());
   

   So this will change the singleton for all other tests - doesn't break anything atm and if it did the other tests would be written wrong.

@throws does not need to have a comment if the exception is that obvious, see https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... .

1. +++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
   @@ -131,4 +131,19 @@ public function testGetApcuPrefix() {
   +   * @expectedException \BadMethodCallException
   

   Testing best practices say that the exception annotation should not be used and we should call $this->setExpectedException() right before the last line in the test instead.

2. +++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
   @@ -131,4 +131,19 @@ public function testGetApcuPrefix() {
   +    $class = new \ReflectionClass('Drupal\Core\Site\Settings');
   

   this should use the Settings::class instead of the class name in a string.

3. +++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
   @@ -131,4 +131,19 @@ public function testGetApcuPrefix() {
   +    $singleton = $settings->getInstance();
   

   $singleton is never used and can be removed.

Updated patch based on review comments by @klausi. Thanks for the review and guidance.

Correct patch file attached.

+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -44,8 +44,13 @@ public function __construct(array $settings) {
+      throw new \BadMethodCallException('Settings::$instance is not initialized yet.');

What about telling people what they should do instead? So probably call new Settings or Settings::initialize?

Agreed, so the message could be 'Settings::$instance is not initialized yet. Call Settings::initialize() first.'

+++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
@@ -126,4 +126,21 @@ public function testGetApcuPrefix() {
+    $this->setExpectedException('BadMethodCallException');

this should use \BadMethodCallException::class and not as string.

Thanks for the reviews and comments. Patch updated.

Well, if people run into this problem then they are trying to access settings too early, probably outside of the usual Drupal request flow. The only solution to that is to initialize the settings if they really need settings. They probably cannot wait for later?

1. +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -44,8 +44,14 @@ public function __construct(array $settings) {
   +   * @throws \BadMethodCallException
   +   *  Thrown when the settings instance called before being initialized.
   

   That sentence is a bit broken, did you mean "Thrown when the settings instance has not been initialized yet."?

2. +++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
   @@ -126,4 +126,21 @@ public function testGetApcuPrefix() {
   +  /**
   +   * Tests Settings::testGetInstanceReflection().
   +   *
   

   That method does not exist on the Settings class. Should be "Tests that an excpetion is thrown when settings are not initialized yet."

Updated patch.

Looks good!

+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -44,8 +44,14 @@ public function __construct(array $settings) {
+      throw new \BadMethodCallException('Settings::$instance is not initialized yet. Call Settings::initialize() first.');

I think that rather than telling them what to do an explanation of why this occurs might be more useful. I'm sympathetic to @donquixote's point that just telling a developer to call initialize() without also making them aware of why might lead to blindly calling Settings::initialize() without considering your code's architecture.

Patch updated.

+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -50,7 +50,7 @@
+      throw new \BadMethodCallException('Whatever you are trying to do, it might be too early for that. You could call Settings::initialize(), but it is probably better to wait until it is called in the regular way. Also check for recursions.');

I think we should keep the first sentence "Settings::$instance is not initialized yet." and then add the rest. We should mention the reason why we have to throw the BadMethodCallException.

Looks good, thanks!

Tried to think of a better comment because the current one seems a bit cryptic, but that's because hitting the error in the first place is strange and couldn't improve on it.

Committed/pushed to 8.3.x, thanks!