Problem/Motivation

When a service is called without an initialized container, you get an unhelpful message like Fatal error: Call to a member function foo() on a non-object
Additionally, \Drupal::getContainer() can be incorrectly used instead of \Drupal::hasContainer() by checking if it returns NULL.

Proposed resolution

Throw an exception when the container is not initialized.

Remaining tasks

N/A

User interface changes

N/A

API changes

If the container is not initialized, \Drupal::getContainer() (and anything that calls it like \Drupal::service()) will throw \Drupal\Core\DependencyInjection\ContainerNotInitializedException.
Additionally, \Drupal::setContainer() will no longer take NULL, use \Drupal::unsetContainer()

Original report

We should throw exceptions if the container is not initialized yet.
This applies to

  • Drupal::service()
  • Drupal::config()
  • Drupal::moduleHandler()
  • etc, everything that uses Drupal::$container.

Though valuable of its own, this issue is also a preparation for #2354475: [meta] Refactor the installer, (multi)site management, and pre-container bootstrap

Solution 1: Explicit check.

   public static function service($id) {
+    if (!isset(static::$container)) {
+      throw new ContainerNotInitializedException("Container not initialized yet.");
+    }
     return static::$container->get($id);
   }

Solution 2: PlaceholderContainer.

Alternatively we could initialize Drupal::$container with a placeholder container object, that will throw an exception every time we call Drupal::$container->get(). This will also have performance benefits.

If we do this, we need to be careful with Drupal::getContainer(), because we don't want other objects to silently take a reference to the placeholder.

Suggested commit message

Issue #2363341 by tim.plunkett, donquixote, adci_contributor, dawehner, msonnabaum, chx, jibran: Throw exception in Drupal::service() and friends, if container not initialized yet.

Files: 
CommentFileSizeAuthor
#94 interdiff.txt696 bytestim.plunkett
#94 throw_exception_in-2363341-95.patch18.92 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,223 pass(es). View
#92 throw_exception_in-2363341-92.patch19.18 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,227 pass(es). View
#92 interdiff.txt4.6 KBtim.plunkett
#84 interdiff.txt2.05 KBtim.plunkett
#84 2363341-container-84.patch21.69 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,276 pass(es). View

Comments

donquixote’s picture

Status: Active » Needs review
FileSize
6.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Following the approach in Solution 2 with PlaceholderContainer.

https://github.com/donquixote/drupal/compare/D8-PlaceholderContainer-236...

donquixote’s picture

Status: Needs work » Needs review
FileSize
6.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,474 pass(es). View

https://github.com/donquixote/drupal/compare/D8-PlaceholderContainer-236...

The \Drupal::$container needs to be filled with a PlaceholderContainer on \Drupal::setContainer(NULL).
Otherwise, tests will fail.
https://github.com/donquixote/drupal/commit/023312800f6a01661119870507a5...

donquixote’s picture

Note that PlaceholderContainer->has($id) always returns FALSE instead of throwing an exception.
This is a convenient trick to simplify and speed up calls to \Drupal::hasService($id) and \Drupal::hasRequest(), which can now save one part of the &&.

dawehner’s picture

  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?

dawehner’s picture

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.

chx’s picture

>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__);

donquixote’s picture

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

\Drupal::setContainer(NULL); in the same file is the only way to make sure this is initialized as soon as the class becomes available. It does not look pretty, but it does the job.
(maybe others have an opinion on this too)

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.

I want to avoid the PlaceholderContainer to silently leak out of \Drupal into other objects.
I also want to avoid changing the behavior of \Drupal::getContainer() in the scope of this issue.
The current behavior of \Drupal::getContainer() is to return NULL if not initialized. Some functionality in core depends on that, e.g. in drupal_bootstrap():

  return \Drupal::getContainer() ? DRUPAL_BOOTSTRAP_CODE : DRUPAL_BOOTSTRAP_CONFIGURATION;

Other functionality depends on the container in Drupal being initialized, without checking.

This asks for a follow-up for \Drupal::getContainer().
For this issue we should focus on the non-controversial stuff.
E.g. \Drupal::service('foo') would currently just break with fatal or sth like that. The patch changes that to throw an exception instead.

Let's keep this changes out

Fine with me.
It is already split into separate commits, so we can easily split it to a separate issue.
I just hope this won't idle around forever.

Did you considered to extend from RuntimeException?

Ok, why not.
(php.net: "Exception thrown if an error which can only be found on runtime occurs.")

Can we provide any more context, help message?

I am open to suggestions.
I would prefer a message that is not just redundant with the exception class name.
And maybe we could make a default message in the exception class, so it does not have to be repeated all over the place?

donquixote’s picture

@chx: Sorry, I completely missed your comment while writing #8.

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

Yes, I am walking through fire.
Technically I don't think it is a problem. But it raises doubts.
Not sure what to do about it. Either we all bite this apple, or we need to find another solution.

This being said, I am personally more concerned about the overuse of global state and static access, than this.

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

Not sure if __METHOD__ should become the exception message, or if we should wrap that in something else, by overriding the parent exception constructor.

donquixote’s picture

And this information is already in the stack trace..

donquixote’s picture

I *knew* I saw this somewhere!
http://stackoverflow.com/a/693799/246724

PHP can't parse non-trivial expressions in initializers.

I prefer to work around this by adding code right after definition of the class:

Fabianx’s picture

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.

donquixote’s picture

I have a new idea for the exception message and the ::init() !

a) the $containerOrNull is while completely accurate, naming that feels like a work around.

Good idea, doing that.

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.

We can do that. But we still need to keep ::setContainer(). So it might be redundant.
(or it would be, if not for the new idea I am adding)

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.

This would mean to trade predictability for superficial sanity.

Calling ::init() from outside will trigger the class loader and include the file before it is used, or when it is not used at all. And on the other hand, if the class file is included from somewhere else, bypassing the class loader, then the ::init() would not be called. Or in some exotic case it could happen that ::init() is called twice.

Calling ::init() or ::setContainer() or ::initStaticProperties(), however we name it, from within the class file, is the most predictable we can get. It just doesn't look pretty, but that is all.

a fatal is thrown in both cases

Only because we don't catch the exception anywhere. We might leave it at that, or change this in the future.

So if you want to re-use the class, you will need to call the "static constructor" yourself, which is okay if its documented.

But why should we have to..
I prefer technical predictability over documented convention.

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?

I agree.
However, I would prefer to do this in a follow-up, because it involves code changes in wider places, and it means an API change.

donquixote’s picture

FileSize
7.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,471 pass(es). View
donquixote’s picture

Btw we can use the same mechanic for DrupalKernel::$container.
But first some cleanup:
#2363519: Docblock / cleanup in DrupalKernel
#2363523: Docblock / cleanup in \Drupal

donquixote’s picture

FileSize
17.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,652 pass(es). View

Let's see if this works for DrupalKernel.
We can do this in a follow-up, but I would like to post this here as a preview.

https://github.com/donquixote/drupal/compare/D8-PlaceholderContainer-236...
Too bad that github does not show the branches.

donquixote’s picture

@chx (#7)

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

I think the essential thing here is that the thing we call from outside a function must not have any side effects (or none that we have to worry about). It must be "pure", or limited to a local scope (not sure if "pure" is really the right word here).

E.g. if we (indirectly) call stuff like t() from outside a function/method, we likely get into trouble.

But if we include a file with class X extends Y {} then this will have a "side effect", because it triggers the class loader for
Y. However, this is one of the non-lethal side effect that we can live with.

I think in this case we are safe, because nothing fishy happens in the PlaceholderContainer constructor. And 169 people on stackoverflow agree.

EDIT: And even then, this should not become an everyday pattern.

Fabianx’s picture

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!

donquixote’s picture

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

Ok if we do this in a follow-up, considering it is an API change?

Lets improve documentation some however

How about this?

/**
 * Initialize \Drupal::$container with a placeholder object.
 * See https://www.drupal.org/node/2363341
 * 
 * This technique is the most reliable way to initialize static properties with
 * non-trivial expressions. It should NOT be used for anything else. Also, the
 * code being called MUST NOT have any side effects other than initializing the
 * static properties.
 *
 * In general (e.g. in PSR-1), a PHP file should either declare symbols OR have
 * side-effects, but not both. This specific case is ok only because the side
 * effect applies to nothing else but the class declared in the same file, and
 * it happens immediately after the class is being declared. A version of the
 * class without this initialization applied is never available to the outside
 * world.
 * 
 * Note: PHP does not care whether this is called before or after the class
 * declaration. It is called before only for better visibility. 
 */
\Drupal::initStaticProperties(NULL);
donquixote’s picture

If we want to use the same technique for the database manager, maybe we should externalize some of this flavor text?

donquixote’s picture

FileSize
13.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,801 pass(es). View

Adding unit tests.
https://github.com/donquixote/drupal/compare/D8-PlaceholderContainer-236...
The DrupalKernel preview stuff is out for now. I think we should do this in a follow-up.

donquixote’s picture

FileSize
13.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,650 pass(es). View

Use \PHPUnit_Framework_TestCase instead of Drupal UnitTestCase.

https://github.com/donquixote/drupal/compare/D8-PlaceholderContainer-236...

donquixote’s picture

Note to self: @chx on IRC reminded me to use this in the unit test:

    /**
     * @expectedException        MyException
     * @expectedExceptionMessage Some Message
     */
Fabianx’s picture

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

dcrocks’s picture

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.

donquixote’s picture

FileSize
2.25 KB
13.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch D8-PlaceholderContainer-2363341-26.patch. Unable to apply patch. See the log in the details link for more information. View

Here is a fresh version with reroll and chx' suggestion as in #23.

I still would like #2363523: Docblock / cleanup in \Drupal to go in first, though.

donquixote’s picture

Fabianx’s picture

Category: Task » Bug report

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.

dcrocks’s picture

moshe weitzman’s picture

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?

dcrocks’s picture

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

moshe weitzman’s picture

Failed tests :(

adci_contributor’s picture

Status: Needs work » Needs review
FileSize
11.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,649 pass(es). View

Trying to reroll

tim.plunkett’s picture

FileSize
12.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,037 pass(es). View

Rerolled without whitespace errors.

tim.plunkett’s picture

FileSize
13.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,040 pass(es). View
2.53 KB

Fixing one of the @todos.

tim.plunkett’s picture

Issue tags: +DX (Developer Experience)
FileSize
6 KB
14.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,047 pass(es). View

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.

dcrocks’s picture

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.

tim.plunkett’s picture

FileSize
22.3 KB
20.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
20.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
13.89 KB

Working around Drush more with an optional parameter.

dcrocks’s picture

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?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
20.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,212 pass(es). View
1.8 KB

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

tim.plunkett’s picture

FileSize
20.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,328 pass(es). View
1.92 KB

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

jibran’s picture

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

dcrocks’s picture

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.

tim.plunkett’s picture

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.

jibran’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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?

dawehner’s picture

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()

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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 :)

tim.plunkett’s picture

Status: Needs work » Needs review

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.

tim.plunkett’s picture

Status: Needs review » Postponed

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.

tim.plunkett’s picture

FileSize
19.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh. View

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

kim.pepper’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
alexpott’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,171 pass(es), 2 fail(s), and 3 exception(s). View

Reuploading

Status: Needs work » Needs review
dcrocks’s picture

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.

Status: Needs work » Needs review

Status: Needs work » Needs review
dcrocks’s picture

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.

dcrocks’s picture

Status: Needs work » Needs review

donquixote’s picture

re #40 (timplunkett):

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

Just some comments on the new approach without the placeholder.

The placeholder approach has a theoretical micro performance advantage (which may or may not be significant, and would yet have to be measured). And it allows to show different error messages showing why the container is not set - either because it was never set, or because it was unset after being set. It would even remember when it was unset. It also makes the $container variable more consistent, as it always contains an object.

This being said, I can see how it is unorthodox, unfamiliar, possibly confusing. The class PlaceholderContainer only exists for this particular mechanic, and the in-file initialization raises some bad feelings.

I don't see it being technically fragile, but I could be missing something. But even if it is totally solid and reliable, everyone who looks at it will spend some time thinking about what this is, and what could go wrong.

I still love it, though :)

-----------------

If we go with this simpler approach, I am wondering if we should directly call static::service() instead of static::getContainer()->get() in the methods like currentUser() etc. We could inline the getContainer() call into service() for a bit of micro-optimization.

  public static function service($id) {
    if (static::$container === NULL) {
      throw new ContainerNotInitializedException('\Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.');
    }
    return static::$container->get($id);
  }
[..]
  public static function currentUser() {
    return static::service('current_user');
  }
dcrocks’s picture

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.

tim.plunkett’s picture

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

dcrocks’s picture

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

dcrocks’s picture

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.

tim.plunkett’s picture

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

dcrocks’s picture

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.

tim.plunkett’s picture

Status: Needs work » Postponed

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

Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 62: 2363341-container-55.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
21.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,276 pass(es). View
2.05 KB

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

Berdir’s picture

+++ 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?

tim.plunkett’s picture

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

dcrocks’s picture

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.

DamienMcKenna’s picture

Issue summary: View changes

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

daffie’s picture

Status: Needs review » Needs work

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.

daffie’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.6 KB
19.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,227 pass(es). View

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.

daffie’s picture

Status: Needs review » Needs work
  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?
tim.plunkett’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
18.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,223 pass(es). View
696 bytes

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.

kim.pepper’s picture

RTBC +1

jibran’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed df44266 on 8.0.x
    Issue #2363341 by tim.plunkett, donquixote, adci_contributor: Throw...
dcrocks’s picture

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.

tim.plunkett’s picture

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.

a.milkovsky’s picture

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.

daffie’s picture

@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.

a.milkovsky’s picture

VladimirAus’s picture

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]
Berdir’s picture

You need to update drush.

VladimirAus’s picture

drush 7 is at latest version

daffie’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.