Throw exception in Drupal::service() and friends, if container not initialized yet.

1. +++ b/core/lib/Drupal.php
   @@ -5,10 +5,17 @@
    /**
   + * Initialize \Drupal::$container with a placeholder object.
   + * See https://www.drupal.org/node/2363341
   + */
   +\Drupal::setContainer(NULL);
   +
   

   Is there no way around that? Can we initialize maybe really early in the DrupalKernel?

2. +++ b/core/lib/Drupal.php
   @@ -108,7 +122,10 @@ class Drupal {
   +    static::$containerOrNull = $container;
      }
   
   @@ -117,10 +134,10 @@ public static function setContainer(ContainerInterface $container = NULL) {
      public static function getContainer() {
   ...
   +    return static::$containerOrNull;
      }
   

   I don't see why we should not return $container directly here ... the exception thrown by that later would be also helpful. On other words I don't see why we need to store both.

3. +++ b/core/lib/Drupal.php
   @@ -469,6 +487,17 @@ public static function urlGenerator() {
   +   * @param string $route_name
   +   *   The name of the route
   +   * @param array $route_parameters
   +   *   An associative array of parameter names and values.
   +   * @param array $options
   +   *   (optional) An associative array of additional options,
   +   *
   +   * @return string
   +   *   The generated URL for the given route.
   +   *
   +   * @see \Drupal\Core\Routing\UrlGeneratorInterface::generateFromRoute()
       * @see \Drupal\Core\Url
       * @see \Drupal\Core\Url::fromRoute()
       * @see \Drupal\Core\Url::fromUri()
   @@ -493,6 +522,12 @@ public static function linkGenerator() {
   
   @@ -493,6 +522,12 @@ public static function linkGenerator() {
       * generate() method. For detailed documentation, see
       * \Drupal\Core\Routing\LinkGeneratorInterface::generate().
       *
   +   * @param string $text
   +   * @param \Drupal\Core\Url $url
   +   *
   +   * @return string
   +   *   An HTML string containing a link to the given route and parameters.
   +   *
       * @see \Drupal\Core\Utility\LinkGeneratorInterface::generate()
   

   Let's keep this changes out

4. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerNotInitializedException.php
   @@ -0,0 +1,15 @@
   +class ContainerNotInitializedException extends \Exception {
   +
   

   Did you considered to extend from RuntimeException?

5. +++ b/core/lib/Drupal/Core/DependencyInjection/PlaceholderContainer.php
   @@ -0,0 +1,94 @@
   +    throw new ContainerNotInitializedException();
   ...
   +    throw new ContainerNotInitializedException();
   ...
   +    throw new ContainerNotInitializedException();
   ...
   +    throw new ContainerNotInitializedException();
   ...
   +    throw new ContainerNotInitializedException();
   ...
   +    throw new ContainerNotInitializedException();
   ...
   +    throw new ContainerNotInitializedException();
   ...
   +    throw new ContainerNotInitializedException();
   ...
   +  public function hasScope($name) {
   +    throw new ContainerNotInitializedException();
   ...
   +    throw new ContainerNotInitializedException();
   

   Can we provide any more context, help message?

Note: This is the foundation to not fail completly in some cases ... its quite hard to debug errors in the bootstrap which calls out to watchdog_exception for example. Maybe throwing an exception allows to harden stuff.

>Is there no way around that? Can we initialize maybe really early in the DrupalKernel?

Agreed -- usually we burn ourselves when something is done outside of functions/methods.

> Can we provide any more context, help message?

Speaking of functions/methods, throw new ContainerNotInitializedException(__METHOD__);

I ran into the same initialization problem just recently for my D7 RenderCache::init() class.

I also researched and thought about the implications of putting things into the class itself (and init the real container) and in the end decided against putting the include in the file.

However I wanted to lazily initialize the real container and not a placeholder container - and I found use cases where I need the static class to be initialized differently - in D7 that would be within hook_boot().

I think however we should do two things:

a) the $containerOrNull is while completely accurate, naming that feels like a work around. My proposal would be to instead check: if static::$container instance of PlaceholderContainerInterface or PlaceholderContainer. We can still return NULL if the container is not set, but instanceof being an opcode is as fast as isset(), so should not be a performance concern.

b) Having a setContainer is nice, but I prefer a more generic ::init() method - that is personal preference however and I am not opposed to setContainer.

c) We know this will or should always only be available after the classloader is loaded, so the static class initialization should be close to the classloader initialization and then it can be done without having to have the call within the same class file.

And c) should be perfectly okay as this just increases the DX a fatal is thrown in both cases - except for the has() case. So if you want to re-use the class, you will need to call the "static constructor" yourself, which is okay if its documented.

d) Code that uses getContainer() to judge if things are initialized for Drupal are using a work-around IMHO - unless that is really a public API and specified that it should be used in that way. I would expect a \Drupal::isInitialized() or even if it needs to be container bound \Drupal::hasContainer(), but maybe that is for a case where the legacy code will be removed anyway?

That are my thoughts on this issue.

It looks clean to me, with the initStaticProperties this is much less a concern now (for me).

I don't think we need to support setContainer(NULL) anymore, but use unsetContainer for that. is there still something that calls setContainer(NULL)?

I agree that as long as the static initialization keeps within the scope of the class and is _merely_ and this needs to be cleanly documented for the case that PHP does not support dynamic initializations for static class properties, then this is okay.

Lets improve documentation some however, that this MUST not be used for anything outside of the "scope" of the current class.

Thanks!

Looks good to me, once you are happy, please leave a tell in IRC or here and we can review and possibly RTBC.

I tried this patch on a Git clone of D8.0 I created today. It applied ok but I think it may need a re-roll. In any case It did solve a problem I have seen, #2377631: Early Install error: PHP Fatal error: Call to a member function get() on a non-object, for a long time. I've looked at a number of issues for clues and was finally lead to this one and decided to try it. Trivial testing on the new install showed no problems. This may save a number of issues with similar symptoms.

RE=classifying as a bug as the test coverage shows that there was wrong behavior before, which made it impossible to use the container in the case it is not initialized, yet.

Throwing an exception is fixing this and helping with debugging.

This is improving error messages for me. The code looks OK but I'm not 100% familiar with all that it does. Anyone else want to review this?

#2363523: Docblock / cleanup in \Drupal is in. Can this get another look?

Failed tests :(

Trying to reroll

Rerolled without whitespace errors.

Fixing one of the @todos.

Okay, fixing everything except the parts that are drush's fault.
I think this is ready, and important for DX to figure out why your container is missing.

Applied patch cleanly to new clone of D8.0, installed without problems, did trivial testing without any errors being reported, either by drupal or apache. Again, made the errors messages reported in #2377631: Early Install error: PHP Fatal error: Call to a member function get() on a non-object go away.

After discussing with @dawehner, I decided to try option 1 from the IS. Using a PlaceholderContainer seems like a hack.

Working around Drush more with an optional parameter.

I tried this patch on a new git clone of D8.0. The error I have been seeing does get caught with the new exception message at the same place it always has. But now install fails at the last step with multiple occurrences of the new exception message. Is this a problem with the patch or has D8 been suffering from unnoticed silent fails for years now?

@dcrocks the last patch hasn't passed automated tests yet, it's not quite done.

Okay, I think this new patch should be the final one.

Can we include #2160655: Improve error handling of \Drupal class here and close it as a duplicate?

Installed cleanly on new D8.0, D8 install went ok, initial operations are ok. The error I've been seeing doesn't show up anymore, which is a surprise, but I didn't really understand why it was happening in the 1st place.

I think the only change from there that is missing here is the change to PluginBase, which is now a part of DependencySerializationTrait, and doesn't seem relevant anymore.

Thanks for clearing that up @tim.plunkett. Marked #2160655: Improve error handling of \Drupal class as duplicate and added suggested commit message in IS. Can we update IS with current fix and perhaps BE?

I'm wondering whether we have to profile whether the additional overhead of the function calls matter. Not sure how many functions calls do we have on \Drupal::$foo()

+++ b/core/lib/Drupal.php
@@ -103,27 +104,53 @@ class Drupal {
+  public static function getContainer($exception_on_invalid = FALSE) {
+    if (static::$container === NULL && $exception_on_invalid) {

I think the default should be the other way around. The special case is calling \Drupal::getContainer() and being happy to not get a container :)

We can't change the default without fixing Drush first. My hope was to just remove the parameter and swap the logic once Drush is fixed.

Instead of introducing a new parameter we intend on removing anyway, splitting this into two issues.
This is now blocked on #2421005: Add \Drupal::hasContainer() instead of checking if \Drupal::getContainer() === NULL and getting an updated drush on testbots.

Here's the patch, which won't apply until the other issue goes in.

#2421005: Add \Drupal::hasContainer() instead of checking if \Drupal::getContainer() === NULL is in. Lets test it!

Well now it's postponed on https://github.com/drush-ops/drush/pull/1153 and #2404923: Upgrade Drush on qa.d.o

Test it's no longer have a dependency on drush!

Reuploading

Tried this patch on a new Git clone of D8.0. Patch applied cleanly, installed cleanly, no errors with trivial use. Neither drupal log nor apache error log showed any problems.

This looks like the same failure I have been seeing for a long time in apache's error log(#2377631: Early Install error: PHP Fatal error: Call to a member function get() on a non-object). Versions of this fix prior to #55 weren't showing the error, mainly because of the code change instigated by comment in #52. This error has been occuring in the code in drupal.php to get the theme service(~line 614) for more than a year now. It only shows in the apache error log at the beginning of install and 'seems' to have no lasting affect. I ignored it until recently but I thought since D8 is getting closer to release status it shouldn't be hanging around in people's error logs, so I opened the aforementioned issue.
At this time I would say that this patch is now successful because it has caught a long standing and unnoticed silent fail.

Since #2421005: Add \Drupal::hasContainer() instead of checking if \Drupal::getContainer() === NULL went in, retrying patch.

I don't know which solution is the best but I do believe an error has been occurring very early in D8 install for a very long time that has simply not been noticed, possibly because you only see it in the apache error log. And it does appear to be a 'ContainerNotInitializedException' error getting the theme service. And it also looks like when drupal tries to log it, since logging appears to need a theme, it recurs in exception handling, which causes the drush? failure.
Of course I may be completely off on this but I think the failure of the current patch is not due to the patch but because it is finally catching a previously uncaught error in D8's install.

@dcrocks ContainerNotInitializedException is added by these patches, it does not occur in HEAD at all.

What I am saying is that is the error that has been happening, This patch gives it a name.

Before I get some sleep I have to try to clarify.
1) An uncaught fatal error has been occurring on D8 install for more than a year.
2) This patch catches this error.
3) This error is what causes the patch to fail testing.
4) With or without the patch D8 swallows the error and keeps on ticking. It is only the testing system that chokes on it.

Thanks @dcrocks. I think we have concluded that both approaches solve your problem, now it's just a matter of getting it to pass.

This is what I think is happening.
(1) A theme service is being called early in install without an initialized container. Either before or simultaneous with display of the first install page. A php fatal error is shown the apache error log. Install proceeds without incident.
(2) If sol 2 is applied the placeholder container must be available and the exception doesn't occur and install continues.
(3) If sol 1 is applied it catches the error but then the exception repeats in exception handling. Install continues without issue unless it is in the test process. There the test process fails.

If I am wrong please correct me. I don't think sol 1 will work without finding the the original error and sol 2 might be hiding a fail.

@dcrocks both solutions work fine. This is currently failing because Drush has not been updated on the testbots.

Okay, Drush was updated. Here are the remaining fixes.

+++ b/core/modules/simpletest/src/Tests/KernelTestBaseTest.php
@@ -27,7 +27,7 @@ class KernelTestBaseTest extends KernelTestBase {
   protected function setUp() {
-    $original_container = \Drupal::getContainer();
+    $original_container = $this->originalContainer;
     parent::setUp();
     $this->assertNotIdentical(\Drupal::getContainer(), $original_container, 'KernelTestBase test creates a new container.');

Uff, does this make sense? $this->originalContainer is always NULL at this point, no?

How can there not be a container at this point?

TestBase::prepareEnvironment() is called right before setUp().

In it, it does $this->originalContainer = clone \Drupal::getContainer();
and then calls \Drupal::unsetContainer()

So this is more correct than the old code used to be, in HEAD it's testing that a fully built container is not the same as NULL

Applied patch in #84 to new git clone of D8.0. D8 installed cleanly with no error msgs in Apache error log or drupal log.

(Added a period to the end of the suggested commit message.)

Some remarks of the DrupalContainerNotInitializedTest. The rest of the patch looks good!

1. +++ b/core/tests/Drupal/Tests/Core/DrupalContainerNotInitializedTest.php
   @@ -0,0 +1,112 @@
   +  public function testContainerNotInitializedException($method, $args = array()) {
   ...
   +  public function testUnsetContainerException($method, $args = array()) {
   ...
   +  public function testGetContainerException() {
   

   Can we add the @covers documentation.

2. +++ b/core/tests/Drupal/Tests/Core/DrupalContainerNotInitializedTest.php
   @@ -0,0 +1,112 @@
   +    call_user_func_array(['Drupal', $method], $args);
   ...
   +    call_user_func_array(['Drupal', $method], $args);
   ...
   +    \Drupal::getContainer(TRUE);
   

   In #2057905: [policy, no patch] Discuss the standards for phpunit based tests there is the best practice that is saying that every test must have an assertion.

3. +++ b/core/tests/Drupal/Tests/Core/DrupalContainerNotInitializedTest.php
   @@ -0,0 +1,112 @@
   +  public function methodsWithExceptionProvider() {
   

   Can this dataprovider also return the variable $expected. And can you also add a not existent service that is expected to fail.

4. +++ b/core/tests/Drupal/Tests/Core/DrupalContainerNotInitializedTest.php
   @@ -0,0 +1,112 @@
   +   * @param string $method
   +   *   The static method to call on \Drupal::
   +   * @param array $args
   +   *   Arguments to pass into \Drupal::$method(..)
   ...
   +   * @param string $method
   +   *   The static method to call on \Drupal::
   +   * @param array $args
   +   *   Arguments to pass into \Drupal::$method(..)
   

   Can we move the documentation of the parameters to the dataprovider docblock. No double code and no double documentation.

And one more remark of the DrupalContainerNotInitializedTest.

+++ b/core/tests/Drupal/Tests/Core/DrupalContainerNotInitializedTest.php
@@ -0,0 +1,112 @@
+class DrupalContainerNotInitializedTest extends \PHPUnit_Framework_TestCase {

Is it possible to add @coversDefaultClass to the docblock of the class. I am not sure because Drupal.php does not have a namespace.

That test is pretty useless. It's a bit weird and cruft-prone to manually call all of the methods of a class in one test method.
Moving the relevant part to \Drupal\Tests\Core\DrupalTest.

1. +++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
   @@ -43,6 +47,16 @@ public function testSetContainer() {
   +    \Drupal::getContainer();
   

   In #2057905: [policy, no patch] Discuss the standards for phpunit based tests there is the best practice that is saying that every test must have an assertion. Can there be a dataprovider with some initialized and some not initialized services. Maybe some none existent services as well.

2. I am not sure but do you think this issue needs a change record?

That issue is still under discussion. Also, the @expectedException is an assertion, so this method has one.
The other test methods in this class test the container when it *is* initialized, we're only testing the new exception, which is correct.

I added https://www.drupal.org/node/2431401 for the CR.

Updated the patch because some of the docs were stale.

RTBC +1

+++ b/core/lib/Drupal.php
@@ -103,23 +104,30 @@ class Drupal {
-   * @deprecated This method is only useful for the testing environment. It
-   * should not be used otherwise.
+   * @throws \Drupal\Core\DependencyInjection\ContainerNotInitializedException

Nice.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed df44266 and pushed to 8.0.x. Thanks!

I am going to close the issue I opened, #2377631: Early Install error: PHP Fatal error: Call to a member function get() on a non-object, not because it was fixed but because this patch now swallows it. Drupal was, and still is I am sure, getting a PHP exception at the beginning of install but this patch catches it now and there is no record of it ever happening. The error has been around for a very long time but never seemed to have any impact, so it might be just as well to forget it. I still feel that you can't leave a tack on the floor without someone stepping on it eventually but the issue never generated a lot of interest.

As I said on the other issue, you can replace the thrown exception with die; and it will install fine. This patch doesn't hide anything.

After I updated to 8.0.0-beta7 I get the ContainerNotInitializedException every time on "drush cim -y". If it is not only me who receives it, I can create a new issue.

@a.milkovsky: This patch is in 8.0.0-beta7. So the bug that you are describing is related to this. Please create a new issue for it, if there is none.

Created new issue ContainerNotInitializedException on configuration import

This patch stops me from using Drush.
I checked against beta6 and beta7. It works with beta6 but not with beta7.
drush en, drush cim aren't working. Got the following error

drush en config -y
<h1>Additional uncaught exception thrown while handling exception.</h1><h2>Original</h2><p><em class="placeholder">Drupal\Core\DependencyInjection\ContainerNotInitializedException</em>: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container. in <em class="placeholder">Drupal::getContainer()</em> (line <em class="placeholder">129</em> of <em class="placeholder">/Applications/MAMP/htdocs/_SITES/DrupalLMS/docroot/core/lib/Drupal.php</em>).<pre>Drupal::getContainer()
drupal8_bootstrap(0)
_drush_bootstrap_drupal_configuration()
drush_bootstrap(3, 6)
drush_bootstrap_to_phase(6)
Drush\Boot\DrupalBoot->bootstrap_and_dispatch()
drush_main()
</pre></p><h2>Additional</h2><p><em class="placeholder">Drupal\Core\DependencyInjection\ContainerNotInitializedException</em>: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container. in <em class="placeholder">Drupal::getContainer()</em> (line <em class="placeholder">129</em> of <em class="placeholder">/Applications/MAMP/htdocs/_SITES/DrupalLMS/docroot/core/lib/Drupal.php</em>).<pre>Drupal::getContainer()
Drupal::theme()
_drupal_log_error(Array, 1)
_drupal_exception_handler(Object)
</pre></p><hr />Drush command terminated abnormally due to an unrecoverable error.   [error]

You need to update drush.

drush 7 is at latest version

@VladimirAus: You need to update to the HEAD version of Drush.

Closed #2110921: Provide better feedback if the service container could not be rebuilt as a duplicate, adding credit.