Error handler throws exception when service container is not (fully) available yet [#2194111]

Comment	File	Size	Author
#10	drupal8.error-handler-container.10.patch	1.91 KB	sun

#6	request.png	17.76 KB	dawehner
	drupal8.error-handler-container.0.patch	1.2 KB	sun

Comment #1

sun

German

Karlsruhe

CreditAttribution: sun commented 12 February 2014 at 03:22

Log in or register to post comments

Comment #2

12 February 2014 at 04:39

Status:

Needs review

» Needs work

The last submitted patch, drupal8.error-handler-container.0.patch, failed testing.

Log in or register to post comments

Comment #3

sun

German

Karlsruhe

CreditAttribution: sun commented 12 February 2014 at 12:15

Status:

Needs work

» Needs review

drupal8.error-handler-container.0.patch queued for re-testing.

Log in or register to post comments

Comment #4

dawehner

German

CreditAttribution: dawehner commented 13 February 2014 at 22:41

#2160655: Improve error handling of \Drupal class seems also to be an related.

In general hasService('request') seems to also return TRUE, if the request is not set yet onto the container, see Container::has() and getRequestService() on the container. Maybe we should fill an upstream bug report?
At least for request we might actually want to use IntrospectableContainerInterface::initialized() instead?

Log in or register to post comments

Comment #5

sun

German

Karlsruhe

CreditAttribution: sun commented 15 February 2014 at 04:16

we might actually want to use IntrospectableContainerInterface::initialized() instead?

I'm not sure how that would look code-wise in this function? — bear in mind that we can't make any assumptions or rely on anything in the error handler.

For now, I'd recommend to go ahead with this patch; it certainly resolves the last remaining WTFs in our procedural error handler.

In the end, this entire code will (hopefully) be replaced by completely new error/exception handler code in (or attached to) DrupalKernel.

Log in or register to post comments

Comment #6

dawehner

German

CreditAttribution: dawehner commented 15 February 2014 at 11:56

File	Size
request.png	17.76 KB

Well, there is a semantic difference between has and initialized. While it does not matter for most services it does matter for the request one, see the attached screenshot.

so if we fail during booting or don't have a request at all, we might not fix those issues.

For now, I'd recommend to go ahead with this patch; it certainly resolves the last remaining WTFs in our procedural error handler.

Given that has('request') will always return TRUE we could at least try to check for IntrospectableContainerInterface.

Log in or register to post comments

Comment #7

dawehner

German

CreditAttribution: dawehner commented 15 February 2014 at 11:56

For now, I'd recommend to go ahead with this patch; it certainly resolves the last remaining WTFs in our procedural error handler.

In the end, this entire code will (hopefully) be replaced by completely new error/exception handler code in (or attached to) DrupalKernel.

You cannot argue against that, sure.

Log in or register to post comments

Comment #8

sun

German

Karlsruhe

CreditAttribution: sun commented 15 February 2014 at 15:35

If I get you right, then we'd have to amend that condition to be

if (\Drupal::hasService('request') && \Drupal::getContainer()->isScopeActive('request') && ...)

?

Log in or register to post comments

Comment #9

dawehner

German

CreditAttribution: dawehner commented 15 February 2014 at 15:46

We do we not simply use something like:

if (\Drupal::hasService('request') &&
  // Or introduce a new method on \Drupal
  ($container = \Drupal::getContainer();) && $container->initialized('request')) {