Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Steps to reproduce:
- Apply the patch from https://qa.drupal.org/pifr/test/895698
- Go to admin/structure/views/nojs/handler/content/default/filter/status
You will see the following error message:
If you have just changed code (for example deployed a new module or moved an existing one) read http://drupal.org/documentation/rebuild or run the rebuild script
Additional uncaught exception thrown while handling exception.
Original
LogicException: A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break. in drupal_render() (line 3002 of /var/www/d8/core/includes/common.inc).
drupal_render(Array, 1)
drupal_render_root(Array)
Drupal\Core\Page\DefaultHtmlPageRenderer::renderPage('The website has encountered an error. Please try again later.', 'Error')
Drupal\Core\EventSubscriber\DefaultExceptionSubscriber->onHtml(Object)
Drupal\Core\EventSubscriber\DefaultExceptionSubscriber->onException(Object, 'kernel.exception', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.exception', Object)
Symfony\Component\HttpKernel\HttpKernel->handleException(Object, Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)
Additional
LogicException: A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break. in drupal_render() (line 3002 of /var/www/d8/core/includes/common.inc).
drupal_render(Array, 1)
drupal_render_root(Array)
Drupal\Core\Page\DefaultHtmlPageRenderer::renderPage('The website has encountered an error. Please try again later.', 'Error')
_drupal_log_error(Array, 1)
_drupal_exception_handler(Object)
so what basically happens is that the exception rendering fails, this is quite bad, as there is no way to figure that out without going into _drupal_exception_handler()
and what not
Proposed resolution
TODO
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#38 | exception_inception-2364381-29-reuploaded.patch | 7.03 KB | Wim Leers |
Comments
Comment #1
dawehner.
Comment #2
Wim LeersUgh :(
Will fix this, of course.
Comment #3
Wim LeersNote: I encountered this while working on #2352155: Remove HtmlFragment/HtmlPage already, or at least something very similar to this. I solved it there. Perhaps #2361681: drupal_render(): invert second argument ($is_recursive_call -> $is_root_call) => more strict, better DX/TX (the responsible issue/commit) being a child of #2352155 means that I didn't include the solution for that.
If I can't reproduce this in #2352155, then I'll consider that the solution.
Comment #4
dawehnerSlightly related issue: #2376153: Use whoops for exception handling
Comment #5
Wim LeersI've reproduced this problem now that #2352155: Remove HtmlFragment/HtmlPage has landed. I'll fix it.
Comment #6
Wim LeersComment #7
Wim LeersAlso: #4: that issue title sounds hilarious :D
Comment #8
Wim LeersRoot cause: if an exception occurs during rendering, the render stack does not get reset. When we then render a maintenance page with the error message, the error message above occurs, which is logically speaking correct.
AFAICT the best way to fix this is to have the ability to reset the render stack (in case an exception occurs)… and to be able to do that, we need the render stack not to be contained by
drupal_render()
, but for it to be a separate service.I think Fabianx might have some thoughts on this — assigning to him for feedback.
Comment #9
AviiNL CreditAttribution: AviiNL commentedI am getting the same error after generating nodes using devel-generate then visiting my home-page.
edit: figured out it is a problem with the basic theme, once i set bartik default again, the message goes away.
edit2: Okay, so it happens if I add any content to my site when using basic theme,
Not sure if still related to the OP.. (still the same error though)
Comment #10
Fabianx CreditAttribution: Fabianx commentedWhile a render stack is a good idea, it is way less useful once there is a Renderer service that holds the stack state and has a resetStack method.
So postponing briefly on #2346937: Implement a Renderer service; reduces drupal_render / _theme service container calls.
Comment #11
Wim LeersHurray, #1855066: In the "menu" toolbar tray, clicking/tapping white space should show the child level landed,so we can now easily add a
resetStack()
method, hence this issue is now unblocked. Will roll patch, unless somebody beats me to it.Comment #12
Wim LeersOops, that should've been this one.
Now also rolling a patch for this issue.
Comment #13
Wim LeersComment #15
Wim LeersBoth of those failing tests pass locally! :( Re-testing.
Comment #18
Wim LeersI'm clueless. Help?
Comment #22
dawehnerI could reproduce that on my relative install on localhost/d8.
For the config import it set ups a batch process, which itself is processed
using the no_js option. For a simpletest it means that
WebTestBase::checkForMetaRefresh()
is used.If you track the recursive calls in
ConfigImportUiTest::testImport 115
you will see that just the first requesthas that meta tag, which forces the no_js refresh, every following one, no longer has it, aka. no more than step 0 is processed in the simpletest.
You will see similar behaviour when you disable JS, install it on localhost/d8
and import config changes AND extension changes.
Things I tried:
Comment #24
Fabianx CreditAttribution: Fabianx commentedWell, it seems BareHtml Rendering does care about the state, however to fix the bug here, we only need to reset it in the exception rendering case, right?
Lets skip that and move to follow up or won't fix?
Thanks Daniel, great research!
Comment #25
Wim LeersThe inexplicable failure was fixed by dawehner & Fabianx in #2382503: Not possible to render self-contained render array while a render stack is active, which just got committed.
This is actually wrong; there already is a safe guard to reset the stack in case an exception occurs during
#pre_render
. That then only leaves the following function calls insideRenderer::render()
that would need similar safeguards:$elements['#children'] = $this->theme->render($elements['#theme'], $elements);
$elements['#children'] = $this->theme->render($wrapper_hook, $wrapper_elements);
#post_render
callback execution#post_render_cache
callback executionI've attached 3 patches:
I strongly prefer option 2, because it's the least hacky approach. Option 1 does still leave the main stack in its bad state. Option 2 resets it, preventing any and all problems.
Comment #28
Fabianx CreditAttribution: Fabianx commentedI prefer option 2, too, but find it too verbose:
a) Wrap the whole function in a try { catch} after we early return for cache_get.
b) Add a helper tryCall which does the try-catch
c) Leave as is.
My vote is a) or c) as b) is performance problematic.
Besides that: Looks good.
Comment #29
Wim LeersOk, option 2 it is :)
I think that if we're a bit smarter about b) that we can limit the damage? For
/node/2
, this implementation gives me 107,750 (HEAD) vs. 107,802 (patch) function calls. 52/107750 = 0.048% increase. i.e. negligible: a tenth of a half percent.Comment #30
Fabianx CreditAttribution: Fabianx commentedAs discussed in IRC, can add this to renderRoot / renderPlain, behavior of calling render() without not being in a stack / root call context is undefined anyway.
Comment #31
Wim LeersComment #33
Wim LeersI uploaded the interdiff as the patch. D'oh. Again.
Comment #35
Wim LeersWim cannot multitask.
Wim cannot multitask.
Wim cannot multitask.
Comment #37
Wim LeersSo I agreed with
But I always had this feeling we were forgetting something. Now I know what :)
The problem is that we render the main content not with
::renderRoot()
, but with::render()
. For good reason: it's necessary to allow the controller to generate a dynamic title.However, the controller could also choose to throw an exception, like a 404, in case it dynamically decides that it cannot show any content. Views does this:
Here's a stack trace of the view in the failing
NodeIntegrationTest
being rendered:which then hits the following code:
because this happens inside a
::render()
call, the patches since #30 will not catch this exception and hence fail with the same error as the one in the IS.This is absolutely valid, and hence the only possible conclusion is that we must catch exceptions on
::render()
, and hence go back to the patch in #29.Comment #38
Wim LeersStraight reupload of #29. Still applies cleanly.
Comment #39
Fabianx CreditAttribution: Fabianx commentedAs calling render is valid and everything can throw an exception, this is RTBC.
Thanks, Wim!
Comment #40
catchCommitted/pushed to 8.0.x, thanks!