This is the last remaining puzzle piece after

#2176105: Installer catches exceptions and manually re-prints them; does not use error/exception handler

+ will most likely help us to fix #1908310: Drupal without DB but with site config throws uninformative error

CommentFileSizeAuthor
#10 drupal8.error-handler-container.10.patch1.91 KBsun
#6 request.png17.76 KBdawehner
drupal8.error-handler-container.0.patch1.2 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Related issues: +#2096741: Installation failure: Missing configuration files

Status: Needs review » Needs work

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

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

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

dawehner’s picture

dawehner
Primary language German
commented

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

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

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.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new request.png17.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.

dawehner’s picture

dawehner
Primary language German
commented

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.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

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

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

?

dawehner’s picture

dawehner
Primary language German
commented

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')) {
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.error-handler-container.10.patch1.91 KB

OK, a helper method on \Drupal makes sense to me. Let's simply do both for now? We can always adjust it later. :-)

dawehner’s picture

dawehner
Primary language German
commented

+1 for now.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

+1 for now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed e71d749 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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