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.
Comment | File | Size | Author |
---|---|---|---|
#94 | interdiff.txt | 696 bytes | tim.plunkett |
#94 | throw_exception_in-2363341-95.patch | 18.92 KB | tim.plunkett |
#92 | throw_exception_in-2363341-92.patch | 19.18 KB | tim.plunkett |
#92 | interdiff.txt | 4.6 KB | tim.plunkett |
#84 | interdiff.txt | 2.05 KB | tim.plunkett |
Comments
Comment #1
donquixote CreditAttribution: donquixote commentedFollowing the approach in Solution 2 with PlaceholderContainer.
https://github.com/donquixote/drupal/compare/D8-PlaceholderContainer-236...
Comment #3
donquixote CreditAttribution: donquixote commentedhttps://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...
Comment #4
donquixote CreditAttribution: donquixote commentedNote 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 &&.Comment #5
dawehnerIs there no way around that? Can we initialize maybe really early in the DrupalKernel?
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.
Let's keep this changes out
Did you considered to extend from RuntimeException?
Can we provide any more context, help message?
Comment #6
dawehnerNote: 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.
Comment #7
chx CreditAttribution: chx commented>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__);
Comment #8
donquixote CreditAttribution: donquixote commented\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 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():
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.
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.
Ok, why not.
(php.net: "Exception thrown if an error which can only be found on runtime occurs.")
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?
Comment #9
donquixote CreditAttribution: donquixote commented@chx: Sorry, I completely missed your comment while writing #8.
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.
Not sure if __METHOD__ should become the exception message, or if we should wrap that in something else, by overriding the parent exception constructor.
Comment #10
donquixote CreditAttribution: donquixote commentedAnd this information is already in the stack trace..
Comment #11
donquixote CreditAttribution: donquixote commentedI *knew* I saw this somewhere!
http://stackoverflow.com/a/693799/246724
Comment #12
Fabianx CreditAttribution: Fabianx commentedI 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.
Comment #13
donquixote CreditAttribution: donquixote commentedI have a new idea for the exception message and the ::init() !
Good idea, doing that.
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)
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.
Only because we don't catch the exception anywhere. We might leave it at that, or change this in the future.
But why should we have to..
I prefer technical predictability over documented convention.
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.
Comment #14
donquixote CreditAttribution: donquixote commentedhttps://github.com/donquixote/drupal/compare/D8-PlaceholderContainer-236...
EDIT:
Interdiff: https://github.com/donquixote/drupal/commit/0749bb843bef693509754d1717f1...
Comment #15
donquixote CreditAttribution: donquixote commentedBtw we can use the same mechanic for DrupalKernel::$container.
But first some cleanup:
#2363519: Docblock / cleanup in DrupalKernel
#2363523: Docblock / cleanup in \Drupal
Comment #16
donquixote CreditAttribution: donquixote commentedLet'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.
Comment #17
donquixote CreditAttribution: donquixote commented@chx (#7)
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 forY
. 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.
Comment #18
Fabianx CreditAttribution: Fabianx commentedIt 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!
Comment #19
donquixote CreditAttribution: donquixote commentedOk if we do this in a follow-up, considering it is an API change?
How about this?
Comment #20
donquixote CreditAttribution: donquixote commentedIf we want to use the same technique for the database manager, maybe we should externalize some of this flavor text?
Comment #21
donquixote CreditAttribution: donquixote commentedAdding 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.
Comment #22
donquixote CreditAttribution: donquixote commentedUse \PHPUnit_Framework_TestCase instead of Drupal UnitTestCase.
https://github.com/donquixote/drupal/compare/D8-PlaceholderContainer-236...
Comment #23
donquixote CreditAttribution: donquixote commentedNote to self: @chx on IRC reminded me to use this in the unit test:
Comment #24
Fabianx CreditAttribution: Fabianx commentedLooks good to me, once you are happy, please leave a tell in IRC or here and we can review and possibly RTBC.
Comment #25
dcrocks CreditAttribution: dcrocks commentedI 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.
Comment #26
donquixote CreditAttribution: donquixote commentedHere 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.
Comment #27
donquixote CreditAttribution: donquixote commentedForgot the github link.
https://github.com/donquixote/drupal/compare/D8-PlaceholderContainer-236...
Comment #28
Fabianx CreditAttribution: Fabianx commentedRE=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.
Comment #29
dcrocks CreditAttribution: dcrocks commentedComment #30
moshe weitzman CreditAttribution: moshe weitzman commentedThis 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?
Comment #31
dcrocks CreditAttribution: dcrocks commented#2363523: Docblock / cleanup in \Drupal is in. Can this get another look?
Comment #34
moshe weitzman CreditAttribution: moshe weitzman commentedFailed tests :(
Comment #35
adci_contributor CreditAttribution: adci_contributor commentedTrying to reroll
Comment #36
tim.plunkettRerolled without whitespace errors.
Comment #37
tim.plunkettFixing one of the @todos.
Comment #38
tim.plunkettOkay, 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.
Comment #39
dcrocks CreditAttribution: dcrocks commentedApplied 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.
Comment #40
tim.plunkettAfter discussing with @dawehner, I decided to try option 1 from the IS. Using a PlaceholderContainer seems like a hack.
Comment #42
tim.plunkettWorking around Drush more with an optional parameter.
Comment #44
dcrocks CreditAttribution: dcrocks commentedI 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?
Comment #45
tim.plunkett@dcrocks the last patch hasn't passed automated tests yet, it's not quite done.
Comment #46
tim.plunkettOkay, I think this new patch should be the final one.
Comment #47
jibranCan we include #2160655: Improve error handling of \Drupal class here and close it as a duplicate?
Comment #48
dcrocks CreditAttribution: dcrocks commentedInstalled 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.
Comment #49
tim.plunkettI 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.
Comment #50
jibranThanks 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?
Comment #51
dawehnerI'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()
Comment #52
alexpottI think the default should be the other way around. The special case is calling \Drupal::getContainer() and being happy to not get a container :)
Comment #53
tim.plunkettWe 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.
Comment #54
tim.plunkettInstead 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.
Comment #55
tim.plunkettHere's the patch, which won't apply until the other issue goes in.
Comment #56
kim.pepper#2421005: Add \Drupal::hasContainer() instead of checking if \Drupal::getContainer() === NULL is in. Lets test it!
Comment #58
tim.plunkettWell now it's postponed on https://github.com/drush-ops/drush/pull/1153 and #2404923: Upgrade Drush on qa.d.o
Comment #61
alexpottTest it's no longer have a dependency on drush!
Comment #62
tim.plunkettReuploading
Comment #66
dcrocks CreditAttribution: dcrocks commentedTried 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.
Comment #70
dcrocks CreditAttribution: dcrocks commentedThis 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.
Comment #71
dcrocks CreditAttribution: dcrocks commentedSince #2421005: Add \Drupal::hasContainer() instead of checking if \Drupal::getContainer() === NULL went in, retrying patch.
Comment #74
donquixote CreditAttribution: donquixote commentedre #40 (timplunkett):
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 ofstatic::getContainer()->get()
in the methods like currentUser() etc. We could inline the getContainer() call into service() for a bit of micro-optimization.Comment #75
dcrocks CreditAttribution: dcrocks commentedI 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.
Comment #76
tim.plunkett@dcrocks ContainerNotInitializedException is added by these patches, it does not occur in HEAD at all.
Comment #77
dcrocks CreditAttribution: dcrocks commentedWhat I am saying is that is the error that has been happening, This patch gives it a name.
Comment #78
dcrocks CreditAttribution: dcrocks commentedBefore 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.
Comment #79
tim.plunkettThanks @dcrocks. I think we have concluded that both approaches solve your problem, now it's just a matter of getting it to pass.
Comment #80
dcrocks CreditAttribution: dcrocks commentedThis 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.
Comment #81
tim.plunkett@dcrocks both solutions work fine. This is currently failing because Drush has not been updated on the testbots.
Comment #84
tim.plunkettOkay, Drush was updated. Here are the remaining fixes.
Comment #85
BerdirUff, does this make sense? $this->originalContainer is always NULL at this point, no?
How can there not be a container at this point?
Comment #86
tim.plunkettTestBase::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
Comment #87
dcrocks CreditAttribution: dcrocks commentedApplied patch in #84 to new git clone of D8.0. D8 installed cleanly with no error msgs in Apache error log or drupal log.
Comment #88
DamienMcKenna(Added a period to the end of the suggested commit message.)
Comment #90
daffie CreditAttribution: daffie commentedSome remarks of the DrupalContainerNotInitializedTest. The rest of the patch looks good!
Can we add the @covers documentation.
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 this dataprovider also return the variable $expected. And can you also add a not existent service that is expected to fail.
Can we move the documentation of the parameters to the dataprovider docblock. No double code and no double documentation.
Comment #91
daffie CreditAttribution: daffie commentedAnd one more remark of the DrupalContainerNotInitializedTest.
Is it possible to add @coversDefaultClass to the docblock of the class. I am not sure because Drupal.php does not have a namespace.
Comment #92
tim.plunkettThat 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.
Comment #93
daffie CreditAttribution: daffie commentedIn #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.
Comment #94
tim.plunkettThat 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.
Comment #95
kim.pepperRTBC +1
Comment #96
jibranNice.
Comment #97
alexpottThis 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!
Comment #99
dcrocks CreditAttribution: dcrocks commentedI 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.
Comment #100
tim.plunkettAs 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.Comment #101
a.milkovskyAfter 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.
Comment #102
daffie CreditAttribution: daffie commented@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.
Comment #103
a.milkovskyCreated new issue ContainerNotInitializedException on configuration import
Comment #104
VladimirAusThis 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 errorComment #105
BerdirYou need to update drush.
Comment #106
VladimirAusdrush 7 is at latest version
Comment #107
daffie CreditAttribution: daffie commented@VladimirAus: You need to update to the HEAD version of Drush.
Comment #109
quietone CreditAttribution: quietone at PreviousNext commentedClosed #2110921: Provide better feedback if the service container could not be rebuilt as a duplicate, adding credit.