Rendered Cache Metadata created during the main controller request gets lost

Another example that currently does this: Search module.

SearchController::view -> SearchPluginBase->buildResults() -> NodeSearch->execute() -> NodeSearch->prepareResults() -> druapl_render()

#4 sounds excellent. But, it's late here, so perhaps I'm seeing it wrong :)

Rerolled. Now at least the front page and /node/1 can be rendered.

Rerolling this also helped me better understand #4 and its consequences. It basically means:

• renderPlain() now actually means renderInIsolation() (i.e. start its own render stack) — it originally meant "turn this render array into a plain string, and don't care about anything else". But it did that by starting a new stack, and then restoring the old one. So the "getting a plain string" part was only a symptom, what it really is about, is rendering in isolation.
• Per #4, only renderRoot() is now allowed to start a stack. This is what allows us to guarantee that nothing in Drupal does rendering (i.e. calling ::render()) we're not aware of, and therefore whose bubbleable metadata may get lost.
• Combined, this means that ::render() can only be called from inside a ::renderRoot() or ::renderPlain() call. (If that weren't the case, we would still be able to miss some bubbleable metadata, and all in #4 would be flawed.)

Three notable changes in this reroll:

1. HtmlRenderer::renderResponse() no longer calls ::render($something, $is_root_call = TRUE), but instead calls ::renderRoot($something), because only ::renderRoot() sets up a stack, not ::render()
2. BUT! HtmlRenderer::prepare() still needs to be able to render the main content without executing #post_render_cache callbacks. So that means calling ::renderPlain()/::renderInIsolation(), with $is_root_call = FALSE.
3. This uncovered that e.g. rendering /node/1 didn't work, because EntityViewController::view() was calling ::render() without a render stack being set up, because it was calling ::render() too early.

This will still have failures, but less.

Also note: ::renderRoot() and ::renderPlain()/::renderInIsolation() are now roughly equivalent: they both set up a stack. But only the latter restores the prior one again; the former only resets it back to NULL.
We could think about simplifying this, by only keeping one.

Before we continue in this direction, which requires fixing all invalid ::render() calls in Drupal core, I'd like to get sign-off from catch or effulgentsia. Assigning to effulgentsia for feedback.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -91,7 +91,7 @@ public function view(EntityInterface $_entity, $view_mode = 'full', $langcode =
   -        $page['#title'] = $this->renderer->render($build);
   +        $page['#title'] = $this->renderer->renderPlain($build);
   

   +1 to this and fixing all other similar early rendering bugs in core.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -111,16 +111,27 @@ public function __construct(ControllerResolverInterface $controller_resolver, Th
      public function renderRoot(&$elements) {
   -    return $this->render($elements, TRUE);
   +    if (isset(static::$stack)) {
   +      throw new \LogicException('A stray renderRoot() invocation is causing bubbling of attached assets to break.');
   +    }
   +    static::$stack = new \SplStack();
   +    $output = $this->render($elements, TRUE);
   +    $this->resetStack();
   +
   +    return $output;
      }
   

   +1

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -111,16 +111,27 @@ public function __construct(ControllerResolverInterface $controller_resolver, Th
   -  public function renderPlain(&$elements) {
   +  public function renderPlain(&$elements, $is_root_call = TRUE) {
   

   -1 to the addition of this parameter. The code that requires passing FALSE seems broken to me. I see the 2 instances of that in HtmlRenderer in this patch so far, and I think we need to fix that class to not require that.

4. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -171,7 +182,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
        if (!isset(static::$stack)) {
   -      static::$stack = new \SplStack();
   +      throw new \LogicException("Render Stack is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain() / renderRoot() or #pre_render pattern instead.");
        }
   

   +1, but should we move it to the public render() method instead of here?

5. Before we continue in this direction, which requires fixing all invalid ::render() calls in Drupal core, I'd like to get sign-off from catch or effulgentsia.

   Early render() calls are a bug, so +1 to fixing them, and to adding the above exception that requires contrib to fix them too.

Also, +1 to renaming renderPlain() to renderIsolated() or similar, but I think we should retain renderPlain() as a (deprecated for 9.x) wrapper for BC.

3. But we need the ability to render something in isolation without executing #post_render_cache callbacks.

Continuing with this patch then, since it's gotten +1s overall.

First fixed this from #4:

and the stray renderRoot() call thingy is broken atm. in HEAD also when trying to render an exception within the Renderer. (not sure why though)

And here's a bunch of fixes. It's gonna take quite a bit of work to fix all wrong usages in core.

Re: the 2 cases in #20, can we have 2 different methods for them? e.g., renderIsolated() (for rendering an email or whatever) and renderEarly() (for rendering an HTML fragment outside of the proper pipeline, but with the intent to then re-insert the result somewhere into a larger fragment that might be cached). We could at least then document these cases properly, and more easily find any remaining renderEarly() calls to see if we can find a way to fix the use case to get itself back into the proper pipeline.

Or, if renderEarly() doesn't cover it, since maybe there are cases where it's not only about being early, perhaps renderIsolated() and renderIsolatedFragment()?

Wim, Fabianx, and I spent most of DrupalCon sprint day discussing this issue. I am going to attempt to capture something vaguely close to what we concluded:

1) Every time you want to render a render array, you MUST have an active render stack context to do so.

2) For the typical cases, the controller and blocks, core will do that for you.

3) For the controller specifically, we can wrap all _controller the calls (much as we used to do with _content, but simpler) with a callable that will do it for you. However, if the controller result is not an array we throw it away and return the result; at that point you're on your own and we assume you have a view listener that knows what it's doing.

4) For the edge cases where you want to render a completely new context (such as an email page, or creating an HTML page other than the one you're about to return, etc.), you have to explicitly create a new context. This is already happening in, eg, contact module.

5) For the edge cases where you have to return a string and pass the metadata back some other way (these are almost all legacy uses of l() or url(), or translatable strings), there will be a new method on Renderer that takes a callable (almost always a closure). That method will take the callable's result, render it, return the string, and stick the resulting metadata onto the current stack frame in the Renderer.

6) For UrlGenerator, we can create a UrlRenderer service that wraps UrlGenerator and the Renderer. It can take care of the above work so that UrlGenerator shouldn't need to be modified. The same can be done for StringTranslation.

7) This mechanism should, we think, also resolve the OutboundPathProcessor problem. A processor that has cache-context-sensitive logic will need to take the Renderer as a dependency and use the method from point 5 itself. If it doesn't, that's a bug. Please file a patch!

8) As a delightful side-effect, SafeMarkup's cache can move to the Renderer stack. That means it does not persist beyond a given rendering context. We can therefore render the controller result before calling blocks, or even thinking about blocks, which results in throwing away the string index at that point. It could be built up later by other contexts, but those are independent so they don't need to be the same cache. That reduces the memory footprint of SafeMarkup.

When is $token_service->replace() called during controller execution? Isn't it usually called during rendering (e.g., from formatters)?

Discussed this on a call with the other committers, and we decided to defer triaging this as Critical or Major.

There's no question that this would be a great thing to fix, because:

1. It would cleanly solve #2351015: URL generation does not bubble cache contexts.
2. It would solve similar problems as above with contrib-based outbound processors.
3. Per #24.8, it would pave the way for a more efficient SafeMarkup registry: similar or better than #2488538: Add SafeMarkup::remove() to free memory from marked strings when they're printed or #2503447: Use Twig_Markup to help with the SafeMarkup::$safeStrings memory load from Renderer::doRender().
4. It would solve other problems with contrib use cases of early rendering.

However, 1 and 3 above have other, potentially quicker, solutions in the works, and it's not yet clear if there are any contrib conditions under which 2 or 4 would be critical. So, we'll re-evaluate this one's priority once new information about any of that comes in. Or, if it lands before then, then we won't have to :)

Per #2429287-102: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance, taking this on to better be able to assess the criticalness, risk and amount of work required.

First, a reroll that applies to HEAD.

#17 was at 2,090 fail(s), and 372 exception(s).

#30 is at 1,634 fail(s), and 299 exception(s).

Looks like we got a nice reduction "for free" — probably all the other cacheability-fixing patches that went in since helped quite a bit :)

Less failures.

#2505835: Optimize CommentAdminOverview should also help reduce the number of failures.

Less failures.

And one very noteworthy fix:

diff --git a/core/modules/comment/src/Tests/CommentDefaultFormatterCacheTagsTest.php b/core/modules/comment/src/Tests/CommentDefaultFormatterCacheTagsTest.php
index aeb6d95..884bc03 100644
--- a/core/modules/comment/src/Tests/CommentDefaultFormatterCacheTagsTest.php
+++ b/core/modules/comment/src/Tests/CommentDefaultFormatterCacheTagsTest.php
@@ -72,6 +72,11 @@ public function testCacheTags() {
       'entity_test_view',
       'entity_test:'  . $commented_entity->id(),
       'comment_list',
+      'config:core.entity_form_display.comment.comment.default',
+      'config:field.field.comment.comment.comment_body',
+      'config:field.field.entity_test.entity_test.comment',
+      'config:field.storage.comment.comment_body',
+      'config:user.settings',
     );
     sort($expected_cache_tags);
     $this->assertEqual($build['#cache']['tags'], $expected_cache_tags, 'The test entity has the expected cache tags before it has comments.');
@@ -114,6 +119,11 @@ public function testCacheTags() {
       'config:filter.format.plain_text',
       'user_view',
       'user:2',
+      'config:core.entity_form_display.comment.comment.default',
+      'config:field.field.comment.comment.comment_body',
+      'config:field.field.entity_test.entity_test.comment',
+      'config:field.storage.comment.comment_body',
+      'config:user.settings',
     );
     sort($expected_cache_tags);
     $this->assertEqual($build['#cache']['tags'], $expected_cache_tags, 'The test entity has the expected cache tags when it has comments.');

… note that the fact that drupal_render() was used in the test meant that some cache tags were missing! Therefore the test was incomplete!

(I'd been debugging for an embarrassingly long time, unable to figure out was wrong — turns out the test was wrong!)

Less failures.

• A gloriously simplified MessageViewBuilder in the contact module.
• And now you can actually use the /node/add/article form again, thanks to fixed file + image widgets, as well as #type => actions . I bet this will fix a lot of failures. :)

(Note, I tried to get rid of the rendering in Actions, but to do so, I needed to resolve a @todo that was added by sun back in #1751606: Move published status checkbox next to "Save". The technical debt from the dropbutton is still haunting us, and will continue to do so. So, for now, just kept the exact same logic.)

(I'd been debugging for an embarrassingly long time, unable to figure out was wrong — turns out the test was wrong!)

Urgs, cheer up!

+++ b/core/modules/views/src/Tests/ViewElementTest.php
@@ -52,12 +52,13 @@ protected function setUp() {
   public function testViewElement() {
+    $renderer = $this->container->get('renderer');
     $view = Views::getView('test_view_embed');

@@ -118,13 +119,14 @@ public function testViewElement() {
   public function testViewElementEmbed() {
+    $renderer = $this->container->get('renderer');

Minor point: Everytime I do that I add an inline @var statement to make it easier to jump to it, next time you run into it, while debugging

1. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   index a91b14b..76153bd 100644
   --- a/core/lib/Drupal/Core/Render/Renderer.php
   
   --- a/core/lib/Drupal/Core/Render/Renderer.php
   +++ b/core/lib/Drupal/Core/Render/Renderer.php
   

   I'm curious, we chang e the places where we throw an exception, I would assume that we also have to update the documention on Renderer itself?

2. +++ b/core/modules/field/field.module
   @@ -121,7 +121,7 @@ function field_help($route_name, RouteMatchInterface $route_match) {
   -        $output .= drupal_render($item_list);
   +        $output .= \Drupal::service('renderer')->render($item_list);
   

   Mh, I'd expected renderPlain here ...

3. +++ b/core/modules/simpletest/src/Form/SimpletestResultsForm.php
   @@ -94,10 +94,10 @@ protected static function buildStatusImageMap() {
        return array(
   -      'pass' => drupal_render($image_pass),
   -      'fail' => drupal_render($image_fail),
   -      'exception' => drupal_render($image_exception),
   -      'debug' => drupal_render($image_debug),
   +      'pass' => $image_pass,
   +      'fail' => $image_fail,
   +      'exception' => $image_exception,
   +      'debug' => $image_debug,
   

   Nice!

#39: thanks for the early feedback — please note that this patch is still very very very very rough. Yes, the Renderer will need updated docs, but the current changes to that class will not be the final changes.

1. Fixed everywhere. (Will be in the next patch I post.)
2. Oh, I was concerned it might add assets, but I didn't consider where that was being rendered: in hook_help(). Then, yes, absolutely. Done. (Will be in the next patch I post.) Thank you! *Awesome* catch :)
3. Heh :)

Hurray, #38 fixed ~450 failures!

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -89,16 +89,27 @@ public function __construct(ControllerResolverInterface $controller_resolver, Th
   public function renderRoot(&$elements) {
-    return $this->render($elements, TRUE);
+    if (isset(static::$stack)) {
+      throw new \LogicException('A stray renderRoot() invocation is causing bubbling of attached assets to break.');
+    }

is there a way to trow a kind of that exception when something trying to add metadata to stack that does not exists?

#42: no, it doesn't work, I tried. This is dark dropbutton/theme system + render system interaction magic.

#43: Addressing that is the next step, will be in the next patch! :)

So far, what I've done, is fixing things by looking at test failures, debugging them and then fixing the root cause. But this makes for slow progress, and at some point we'll have to deal with the main purpose of this issue: capturing bubbleable metadata that is lost, because the route's controller is calling drupal_render() when no stack exists (=== outside of a render context).

But, before doing so, and what you'll find in this patch, I looked at all remaining drupal_render() calls (roughly 250) and determining which ones are broken by design and won't be fixed by the above. This is mostly in tests. Nearly every exception of those 260 exceptions in #38 is caused by drupal_render() calls in tests. This should fix the majority, and significantly decrease the number of exceptions.

#38: 1,105 fail(s), and 260 exception(s).
#44: 1,018 fail(s), and 166 exception(s).

So 87 fewer fails, 94 fewer exceptions.

Next step described in #44, i.e. do the key part of what this issue aims to do, and that should cause another significant drop in number of test failures. Stay tuned!

Added 2 more issues under new meta #2509534: [Meta] document or refactor calls to drupal_render() but then found old one and checked all related issues from it #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead (they are all already fied)

Rebased.

#2505835: Optimize CommentAdminOverview conflicted (changes here became obsolete, that patch fixed it already) and #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests with this, easy fixes, straight reroll.

I had a 502, bad gateway. That apparently ate the patch. d.o--

This patch then finally does what the issue is all about!:

1. It introduces an EarlyRenderingControllerWrapper event subscriber, that wraps the executed controller, thus ensuring that a render context is available. In doing so, it is able to collect the bubbleable metadata that would otherwise be lost. See the docs on that class for a more detailed explanation.
2. to do (1), introduced a RendererInterface::executeInRenderContext() method. This is intended to be used for the most advanced use cases only. Of which (1) is the prime example. It accepts a render stack (yes, stack, not context, because I wanted to keep changes legible — that'll be fixed in the next step, see below), and makes it the current render stack. Once it's done, it restores whatever the current render stack was.
3. Obviously, Renderer::renderRoot() and Renderer::renderPlain() have been setting up a "render context" all along. But rather than letting them continue to do things directly, now that we have (2), we should unify things, to make things more understandable. So both have been updated to use ::executeInRenderContext().
4. Consequently, a new problem was that ::executeInRenderContext() must not reset the stack automatically, otherwise code still can't access the stack, and (1) would be unfixable. But we also don't want to make ::resetStack() a public API, that'd make things far too fragile. The solution to that lies in the fact that ::resetStack() really only existed as the signal that Renderer::renderRoot() was using to determine whether it was a nested call. By changing that to use a property on the Renderer instead, I was also able to remove ::resetStack().
   This really only makes things much clearer :)

Next step: class \Drupal\Core\Render\RenderContext extends \SplStack {…}, that will greatly clarify things (see point 2 above).

This should cause another significant drop in the number of failures. But it won't be zero, a whole bunch of bad drupal_render() calls still exist, those will need to be fixed too, but they'll be easier to surface. I've chosen to keep this reroll focused just on the above, to make it easier to review.

(Also rebased again, zero conflicts this time.)

#49: 1,019 fail(s), and 166 exception(s)
#51: 263 fail(s), and 77 exception(s)

YAY!

Next step: class \Drupal\Core\Render\RenderContext extends \SplStack {…}, that will greatly clarify things (see point 2 above).

Done.

Next step: getting to zero failures.

The increase of failures is due to the move from a class property to an object property. We need to persist the Renderer service instead.

Exciting! I am at a client onsite this week but will try to find time to review. At least from what I see in the comments here I'm smiling.

Turns out #57 is not the right solution, because… /facepalm. What happens in #57: the renderer is persisted, so when the test runner creates a test environment, the renderer service is already created before the test environment is set up. So when running the test, the same renderer is still used. Which means a bunch of things don't exist as far as it's concerned, because it was created in a different environment…

So, back to a class property (i.e. using static), the same solution as HEAD.

Hopefully that'll bring us back to 263 failures like #51.

P.S.: "Crell smiling commit gate": PASS! :P

Oops, two debug thingies snuck in.

And in the mean time, HEAD changed hugely thanks to #2407195: Move attachment processing to services and per-type response subclasses. Rebased.

Notice that the patch is slightly smaller, thanks to #2407195 :)

And yay, #60 (and hence #64 too, if all is well) is back to 263 failures, just like #51 :)

Now let's fix the remaining failures, most of which should be bad usages of drupal_render().

This should fix a whole lot of the failures.

Just some quick feedback. I really like it!

One question, could we do all the conversions in tests separately?

1. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapper.php
   @@ -0,0 +1,136 @@
   +      if (!$context->isEmpty()) {
   ...
   +        }
   +        else {
   +          throw new \LogicException(sprintf('Early rendering is not permitted for controllers returning anything else than render arrays. Response class: %s.', get_class($response)));
   +        }
   

   So you are not allowed to call drupal_render() during a rest request? That seems quite a hard limitation

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -89,18 +96,31 @@ public function __construct(ControllerResolverInterface $controller_resolver, Th
   +    $this->isRenderingRoot = TRUE;
   ...
   +    $this->isRenderingRoot = FALSE;
   

   Its sad that we need to store state on the renderer. Is there any way we could avoid that? For example, could we look onto the current stack?

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -89,18 +96,31 @@ public function __construct(ControllerResolverInterface $controller_resolver, Th
   +   *
   +   * @todo Rename to ::renderInIsolation()
       */
   

   Can we keep a BC layer?

4. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -458,8 +479,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    $this->context->update($elements);
   

   I'm curious whether naming it $this->context to $this->contextStack would be a bit easier to understand

5. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -316,6 +320,35 @@ public function renderPlain(&$elements);
   +   * @throws \LogicException
   +   */
   

   It would be great to explain when this is thrown

#69: Thanks for the feedback! :) Glad that you're generally happy. Yes, we can move the test conversions into separate issues if we want. I'll address your points later, now still working on reducing the number of failures.

About 50 failures less.

>=29 less failures.

>=10 fewer exceptions. Painful, repetitive changes to Views' tests, but that's due to the nature of Views' very repetitive/verbose tests (no data provider-style testing), and refactoring those tests is out of scope here. Fortunately, very simple changes.

Nevertheless, that means a whopping 55 KB interdiff.

P.S.: So many changes for only ~10 fewer exceptions? Well yes, but that's because those exceptions didn't actually show the actual impact: once you fixed the test code triggering the first exception, there was another function call triggering the same exception, and so on. Hence the use of the word "repetitive" above.

P.S.: So many changes for only ~10 fewer exceptions? Well yes, but that's because those exceptions didn't actually show the actual impact: once you fixed the test code triggering the first exception, there was another function call triggering the same exception, and so on. Hence the use of the word "repetitive" above.

In other words, the fact how we use simpletest assertions is shit. You should fail immediately, and not run the tests in a sort of broken/running state.
I'm glad that with KernelTestBaseNG for example this will happen.

Again fewer exceptions.

Fabianx pointed out that many failures are due to

Early rendering is not permitted for controllers returning anything else than render arrays. Response class: Symfony\Component\HttpFoundation\RedirectResponse

But… when redirecting, the bubbleable metadata is lost anyway. And it's perfectly valid for the controller to do early rendering, but then in an exceptional case, choose to do a redirect instead. Yet prior patches don't allow that. This explains the failures in Views. By simply only throwing an exception for responses that do care about attachments or cacheability, we remain correct, but fix the failures :)

Thanks, Fabianx!

#79 already fixed all remaining failures in Views, this interdiff fixes all remaining exceptions in Views, plus all other exceptions. This should bring us down to zero exceptions!

Except… Drupal\rest\Tests\Views\StyleSerializerTest is locally throwing strange, reproducible PDO errors. I hope that's a local environment problem.

I said "zero exceptions", but I meant "zero yellow test result rows" — there were still 8 exceptions in a test that also has failures.

This brings it down to a single remaining test with failures: Drupal\simpletest\Tests\SimpleTestTest.

Green.

Next:

1. a bit of clean-up + address review points in #69
2. split off all the drupal_render() changes that can happen in HEAD into a separate issue/patch, that can be committed much faster
3. then this patch is significantly smaller to review, and to make it as easy as possible, I'll provide a "logical changes" patch that will fail, because it points out those places that still do early rendering, the entire patch (the one proposed to be committed as part of this issue) fixes those and will pass

Okay, #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache is the party pooper. Committed three minutes before I posted this. No luck :P

Rebased, resolved a conflict. Let's see if we're still going to be green!

I resolved the conflict correctly (otherwise we'd have many more failures), but didn't update the newly added test. Will finally be actually green.

(This does #87.1.)

#13.3: finally fixed that, no more new parameter for ::renderPlain()!

#42: tried that. Does not work, it breaks the dropbutton.

#69:

1. You must use either:
   • Renderer::renderRoot()
   • Renderer::renderRoot()
   • or, if really necessary (only intended/necessary for advanced use cases): Renderer::executeInRenderContext(), if the code rendering a REST response is calling out to something else that may call drupal_render() (this is what Fabian said in #72.a)
2. In HEAD, we use the presence of static::$stack to determine whether we're already rendering something or not. But the way that works in HEAD makes it impossible to fix this issue. This is a simple solution, that actually is also more accurate/semantical! See #51.4 for details.
3. Of course, and in fact, I think doing such a rename is out of scope for this issue. This patch is more than big enough already. Created a follow-up for that: #2511308: Rename RendererInterface::renderPlain() to ::renderInIsolation().
4. Per our IRC discussion, sticking to $this->context.
5. Done.

Also per our IRC discussion: improved the comment explaining why Renderer::$context must be static.

(This does #87.2 and .3.)

Identical to #92, just split up. The "spin-off, do-not-test" patch is also uploaded separately to #2511472: Refactor all usages of drupal_render()/Renderer::render() that break #2450993, which is where it is spun off to. We need to land that ASAP, i.e. before entering reroll hell. Then, the "logical changes" patch here should become green.

IS updated. Next: CR.

#2511472: Refactor all usages of drupal_render()/Renderer::render() that break #2450993 landed, re-uploading #93's logical changes patch. If all is well, zero failures.

#96: Why?

#100:

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -150,16 +174,17 @@ public function render(&$elements, $is_root_call = FALSE) {
     try {
       return $this->doRender($elements, $is_root_call);
     }
     catch (\Exception $e) {
-      // Reset stack and re-throw exception.
-      $this->resetStack();
+      // Mark the ::rootRender() call finished due to this exception & re-throw.
+      $this->isRenderingRoot = FALSE;
       throw $e;
     }

This is the other try/catch you're referring to.

Note how it is NOT resetting the stack anymore. It is only resetting the isRenderingRoot flag. See #51 for an explanation.

Hence it is not necessary to wrap the logic in ::executeInRenderContext() in a try/catch either.

Yes: #2511330: Deprecate RendererInterface::render()'s sole $is_root_call parameter

I think this is a review of #93... It was sitting in my dreditor for a few days while I went through it across 2 states. :-)

1. +++ b/core/core.services.yml
   @@ -1392,6 +1392,11 @@ services:
   +  early_rendering_controller_wrapper:
   +    class: Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapper
   +    arguments: ['@controller_resolver', '@renderer']
   +    tags:
   +      - { name: event_subscriber }
   

   Nit: All of our other subscriber classes have a *Subscriber suffix. Let's follow that pattern.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapper.php
   @@ -0,0 +1,142 @@
   + * When controllers call drupal_render() (RendererInterface::render()), we call
   

   Assuming we don't want anyone using drupal_render() anymore, just the objects/services, let's not refer to the function in the documentation at all.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapper.php
   @@ -0,0 +1,142 @@
   + * If the render context is not empty, then the controller did use
   + * drupal_render(), and bubbleable metadata is lost. But, rather then letting it
   + * go lost, the closure will update the render array returned by the controller
   + * by merging the "lost" bubbleable metadata onto the render array. Disaster
   + * averted.
   

   It's not lost. It *would have been lost* were it not for this subscriber. Important distinction. I think this paragraph needs to be rewritten accordingly.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapper.php
   @@ -0,0 +1,142 @@
   +    $event->setController(function() use ($controller, $arguments) {
   +      $context = new RenderContext();
   

   The contents of this closure are sufficiently non-trivial that they should be in their own method, and a trivial closure just calls that. Or, even better, push it to its own controller service.

5. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapper.php
   @@ -0,0 +1,142 @@
   +      // If early rendering happened, i.e. if code in the controller called
   +      // drupal_render(), then the bubbleable metadata for that is stored in
   +      // the current render context.
   +      if (!$context->isEmpty()) {
   +        // If a render array is returned by the controller, merge the "lost"
   +        // bubbleable metadata.
   +        if (is_array($response)) {
   

   Let's unroll this code block to make the code metrically simpler.

   First, we should call the return from the controller $controller_result, to be consistent with elsewhere since it may not be a Response object.

   Then, check if $controller_result is an object, of any sort. If it is, assume the controller knew what it was doing. (This is where we can also have the AttachmentsInterface check, etc., if appropriate.) Either return $controller_result or throw an exception.

   If it's a string, just return it period. (Or do whatever we do with strings these days; I lost track.)

   Then if it's an array, we can go into the other bits of handling here. That means fewer nested conditionals, which makes the code easier to follow and debug.

6. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapper.php
   @@ -0,0 +1,142 @@
   +        // If a response object is returned, and the response object cares about
   +        // attachments or cacheability, then throw an exception: early rendering
   +        // is not permitted in that case. It is the developer's responsibility
   +        // to not use early rendering.
   

   I'm unclear here on why this is the case. I can't return a Response with attachments? Or anything with attachments? That seems short-sighted to disallow someone to use objects directly and force them into the array form. Why do this? The comment here is unconvincing.

7. +++ b/core/lib/Drupal/Core/Render/RenderContext.php
   @@ -0,0 +1,60 @@
   +   * Updates the stack.
   +   *
   

   Shouldn't this be "updates the current frame of the stack"?

8. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -57,11 +57,24 @@ class Renderer implements RendererInterface {
   +   * This must be static as long as some controllers rebuild the container
   +   * during a request. This causes multiple renderer instances to co-exist
   +   * simultaneously, render state getting lost, and therefore causing pages to
   +   * fail to render correctly. As soon as it is guaranteed that during a request
   +   * the same container is used, it no longer needs to be static.
   

   Bless you for including this.

9. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -89,18 +102,29 @@ public function __construct(ControllerResolverInterface $controller_resolver, Th
      public function renderPlain(&$elements) {
   -    $current_stack = static::$stack;
   -    $this->resetStack();
   -    $output = $this->renderRoot($elements);
   -    static::$stack = $current_stack;
   -    return $output;
   +    return $this->executeInRenderContext(new RenderContext(), function () use (&$elements) {
   +      return $this->render($elements, TRUE);
   +    });
      }
   

   How does this differ from renderRoot() now, other than renderRoot() having semaphore protection? The actual body of the method is now identical. Does that mean they can be merged?

10. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -89,18 +102,29 @@ public function __construct(ControllerResolverInterface $controller_resolver, Th
    @@ -150,16 +174,17 @@ public function render(&$elements, $is_root_call = FALSE) {
    

    If render() no longer should be called outside of renderRoot() or renderPlain(), why not make it protected? That would save the check below in doRender().

11. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -150,16 +174,17 @@ public function render(&$elements, $is_root_call = FALSE) {
    +    // the 404 page to be rendered). That page might also use
    +    // Renderer::renderRoot() but if exceptions aren't caught here, it will be
    +    // impossible to call Renderer::renderRoot() again.
    +    // Hence, catch all exceptions, reset the isRenderingRoot property and
    +    // re-throw exceptions.
         try {
           return $this->doRender($elements, $is_root_call);
         }
         catch (\Exception $e) {
    -      // Reset stack and re-throw exception.
    -      $this->resetStack();
    +      // Mark the ::rootRender() call finished due to this exception & re-throw.
    +      $this->isRenderingRoot = FALSE;
           throw $e;
         }
    

    We haven't moved to 5.5 in code yet, but a finally {} block in renderRoot() seems like the cleaner way to handle this, maybe?

12. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -495,52 +521,24 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +    // @todo Convert to an assertion in https://www.drupal.org/node/2408013
    +    if (static::$context->count() > 1) {
    +      throw new \LogicException('Bubbling failed.');
         }
    

    Hm. Are we certain this could only happen due to code-screwups? There's no way for a data-screwup to cause this to happen? (Rough divider: Assertions are for errors that can only possibly ever occur if a developer screwed up the code. If it's possible for data to screw up, it should be an exception.)

13. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
    @@ -88,8 +95,8 @@ public function renderPlain(&$elements);
    +   * A render context (\Drupal\Core\Render\RenderContext) can be used to perform
    +   * bubbling; it is a stack of \Drupal\Core\Render\BubbleableMetadata objects.
    

    Interesting note: This fact is not self-evident from the RenderContext class itself! That should be made clearer.

14. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
    @@ -316,6 +322,37 @@ public function renderPlain(&$elements);
    +   * Only for very advanced use cases. Prefer to use ::renderRoot() and
    +   * ::renderPlain() instead.
    

    "Prefer using ::renderRoot()..."

    The grammar you have there sounds German, not Belgian. But it's not English either way. :-P

Excellent review, much appreciated! :) Since they're all either nitpicks or code clarity things, I think you're generally happy with it?

1. Fixed.
2. Well, it's only marked deprecated, it will be removed in D9. I think it's important to keep this mentioned, because most early rendering is/will be done by controllers that are still using drupal_render(). Which means that if they grep the code base (or api.d.o) for "drupal_render", they'll actually find this. So I think it's a feature to leave this in :)
3. Good point. Hopefully this is better.
4. Done.

5. First, we should call the return from the controller $controller_result, to be consistent with elsewhere since it may not be a Response object.

   Disagreed, because $response is what HttpKernel::handleRaw() calls it. This is mirrored after that.

   Then, check if $controller_result is an object, of any sort. If it is, assume the controller knew what it was doing.

   I wanted to do that too. And the patch used to do that. But that unfortunately doesn't work (as I found out the hard way in #79), because controllers returning render arrays (particularly for rendering forms) may choose to return a RedirectResponse at any time.

   Therefore, I think the current logic actually makes as much sense as it can. If you still have suggestions after my above explanations for why things are the way they are in the patch, let me know :)

6. Again, see #79. But you're absolutely right that the comment is too vague. I added a bit that makes the reasoning a bit more clear, but I can't currently think of a better way. I'm hoping you see a better way to structure this :)
7. Copy/pasted from where it lived before, but: sure. Done.
8. :)
9. They cannot be merged. They serve different purposes. renderRoot() indeed has a semaphore to protect it from being called while within an existing renderRoot() call. There are many opportunities here for simplification/clean-up (see #8, #103, etc.), but let us please not do that here, and focus on the task at hand: not losing bubbleable metadata when doing early rendering.
10. Eh… I think you mixed things up a bit there :) There's absolutely no way to make render() protected. This is what calling the (deprecated, but still around until D9) drupal_render() calls. It's also what Twig templates when rendering render arrays call.
11. No; note that this is render(), not renderRoot(). I do have plenty of other places where finally would be most helpful though :)
12. Yes, absolutely.
13. Done.
14. There's no such thing as "Belgian" :P Only French (Walloon is the flavor), Dutch (Flemish is the flavor) and German. (Yeah I know "flavor" isn't the right word, but "dialect" isn't either.) Fixed :D

Yes, I'm on board in large strokes.

The legal return values from controllers gets very tricky with this patch, why I'm concerned. Here's the possible values, and my understanding of what we (I) want them to be. Please fill in (copy and paste to a new comment) what you think they should be and we can then make sure the code handles it accordingly. (With tests, ideally.)

Looking that over, it seems to me that, if I understand this patch's intent (which is to ensure that metadata that gets triggered from within the controller call is captured, no matter what weirdness the controller or its subcalls did), then we can/should go all the way with it. That is:

1) If the return is a render array or AttachmentInterface object, take any captured "global" context and merge it, then return the result.
2) If the return is something very basic like a redirect, pass it through normally.
3) If it's anything else, capture the metadata, produce a render array or AttachmentInterface object, and return that.

Wim, is your understanding that far off from that?

First of all: thanks for asking for test coverage, that was glaringly missing! :O #ashamed

Second: I don't think the legal return values for controllers don't become very complex, in fact… they don't change at all. Nevertheless, that table is a good initiative :)

(Found one bug in the process!)

Change record created: https://www.drupal.org/node/2513810.

Well, #108 is not directly comparable to #107 since it has different rows and columns. :-) Nonetheless, this is a good discussion tool and the table should eventually get captured into the code structure and comments.

To make it explicit, can you define "Early rendering" for this situation? We keep using the word but I am not sure anyone but Wim or FabianX fully grok its meaning anymore. :-)

Primitives: Yes, I know that's out of scope here. I mention it for completeness. :-) However, IIRC Symfony fullstack does allow strings; At least I know Silex does, and just tosses them into a Response object.

Render array: Looks like we're all on the same page here. Yay.

Response: Sounds like we're in agreement.

Non-Response Object: #108 didn't cover this one. Do you agree/disagree with #107 (ie, return verbatim the same as Response.)

Object implementing AttachmentInterface: Here's where I think we disagree. It seems entirely reasonable to me for a controller to want to control the whole response, or to return a domain object for a View listener to render, and still opt-in to our caching system. Consider the case of a controller that renders a ViewModel object that depends on 2 nodes. It should be cache tagged with both of those nodes. However, turning that ViewModel into a Response is the job of a View listener. But I don't want to also have to do whatever it is to ensure that the cache tags end up in the Response HTTP headers so that it clears properly in the "page" (response) cache or Varnish. I should still be able to feed it declarative information and have the system take care of that for me.

Response implementing AttachmentInterface: This is a narrower use case, but I can still see it being valid for the same reason as above.

One case to keep in mind is that I am quite sure someone is going to push for adding an OOP Render API during the D8 cycle, possibly even the one that Carl has been working on outside of Drupal. We should allow for such more advanced cases to tie into the cache system, which allows for more contrib flexibility.

Put another way: We don't want to tightly couple advanced caching to render arrays if we don't have to. Looking at the code, I'm not convinced we have to, at least as far as controllers are concerned.

I also see a potential security issue here. Understanding render arrays and cache contexts and such is a non-trivial case. I can easily see someone writing code that happens to step on something that needs a cache context (like a url rewrite, which would affect anyone and everyone silently), yet they're returning a ViewModel object for some very good reason. If that happens... poof, you have a cache poisoning hole. Although I suppose that is more a case for merging in caught metadata for all return values, period, just in case.

Well, #108 is not directly comparable to #107 since it has different rows and columns. :-) Nonetheless, this is a good discussion tool and the table should eventually get captured into the code structure and comments.

Because I forgot about the objects, you didn't sufficiently distinguish between different types of responses, and I felt another column helps clarify things. Sorry :(

To make it explicit, can you define "Early rendering" for this situation? We keep using the word but I am not sure anyone but Wim or FabianX fully grok its meaning anymore. :-)

Rendering before a MainContentRenderer implementation renders it, or more generally speaking: rendering that happens outside of a render context (i.e. nor renderRoot(), nor renderPlain(), nor executeInRenderContext() is a parent on the call stack).

Primitives

Well, Symfony's HttpKernel sure doesn't allow it. Let's leave this for a separate issue.

Response

Indeed.

Non-Response object

Indeed — sorry, overlooked that. (Getting that test coverage to do what it does was a major PITA, I was happy to finally be able to post that patch :P)

(Funny enough, I'm so used to dealing with Response objects at this point, that quite literally, this did not even cross my mind while working on this.)

Now, to answer this. I think we should look at the criticalness of this issue, and realize why it's critical. It's critical, because we still have lots of code (a few in D8 core, but surely many in D8 contrib code, present and future) that calls drupal_render() in controllers (D7's page callbacks). This issue is primarily about resolving that, and the potential security problems there. Yes, it does provide the infrastructure to safeguard other return values of controllers (responses and objects) too. But if your controller is returning not a (silly) render array, but a proper Response object, or a domain object… then we also expect you to be able to generate those Response or domain objects responsibly. If you Do The Right Thing and return a Response or a domain object, then we also expect you to do The Right Thing rendering-wise.

Therefore, I think it makes sense to land the patch in its current state, which indeed punishes (by throwing an exception) Response/domain objects that are returned by controllers that also do early rendering (i.e. call drupal_render()) — which is a pretty strange/crazy combination to be honest. But we can then continue to discuss in a major follow-up issue if we want to bring some of the render array-returning controller behavior over to those object-returning controllers too. I fear that it's quite bikesheddable. But the beauty is that it's a pure API addition, so we can add it at any point in the future. I want to move on to more important issues than this, because what you're asking for is a pure feature addition IMO.

Even more so because we currently only have CacheableResponseInterface, and not CacheableDomainObjectInterface. So we'd need to change/add something there, which is yet more to discuss.

tl;dr: such handling of early rendering for Response/domain objects is 1) a feature request, 2) not even desirable

We don't want to tightly couple advanced caching to render arrays if we don't have to. Looking at the code, I'm not convinced we have to, at least as far as controllers are concerned.

This is not at all about advanced caching. Anything can already do that. Especially since #2407195: Move attachment processing to services and per-type response subclasses sets a great example for other Response subclasses/types to follow.
This is about dealing with the stupidity of render arrays, and ensuring we aren't insecure because of that.

I can easily see someone writing code that happens to step on something that needs a cache context (like a url rewrite, which would affect anyone and everyone silently), yet they're returning a ViewModel object for some very good reason

If you're clever enough to deal with your own domain objects, then you're also smart enough to either mark the corresponding responses as non-cacheable, or to have your domain objects implement CacheableDependencyInterface.

Conclusion: IMHO you're asking for a feature that's out of scope here. Yes, it totally makes sense to bring it up here — it's in scope in that regard — but explicitly designing to have drupal_render() calls performed by a controller that returns Response or domain objects not have their cacheability metadata lost feels like a step in the wrong direction: we want people to stop using early rendering, not for people that HAVE left the render array-centric world view to explicitly rely on such magic behavior.

Let me end by quoting the @todo on \Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber:

* @todo Remove in Drupal 9.0.0, by disallowing early rendering.

So, updated table, where I just replaced "Response" with object:

Here's a few nits - overall looks great and makes me think that we should have a followup meta to try to remove as much early rendering as possible.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php
   @@ -0,0 +1,168 @@
   +use Drupal\Core\Cache\CacheableDependencyInterface;
   

   Not used

2. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php
   @@ -0,0 +1,168 @@
   + * When controllers call drupal_render() (RendererInterface::render()), we call
   + * that "early rendering". Controllers should return only render arrays, but we
   + * cannot prevent controllers from doing early rendering. The problem with early
   + * rendering is that the bubbleable metadata (cacheability & attachments) are
   + * lost.
   + *
   + * This can lead to broken pages (missing assets), stale pages (missing cache
   + * tags causing a page not to be invalidated) or even security problems (missing
   + * cache contexts causing a cached page not to be varied sufficiently).
   + *
   + * This event subscriber wraps all controller executions in a closure that sets
   + * up a render context. Consequently, any early rendering (i.e. drupal_render()
   + * invocations during the actual controller's execution) will have their
   + * bubbleable metadata (assets & cacheability) stored on that render context.
   + *
   + * If the render context is empty, then the controller either did not do any
   + * rendering at all, or used the RendererInterface::renderRoot() or
   + * ::renderPlain() methods. In that case, no bubbleable metadata is lost.
   + *
   + * If the render context is not empty, then the controller did use
   + * drupal_render(), and bubbleable metadata was collected. Without this
   + * subscriber, that bubbleable metadata would have been lost. But since it is
   + * collected by this subscriber, this bubbleable metadata can be merged onto the
   + * render array. Disaster averted.
   + *
   + * In other words: this just exists to ease the transition to Drupal 8: it
   + * allows controllers that return render arrays (the majority) to still do early
   + * rendering. But controllers that return responses are already expected to do
   + * the right thing: if early rendering is detected in such a case, an exception
   + * is thrown.
   

   Seems a bit long and repetitive. Specifically paragraph 3 and 5. Also "Disaster averted." is funny but not consistent with the normal tone of core docs.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php
   @@ -0,0 +1,168 @@
   +  protected function wrapControllerExcecutionInRenderContext($controller, array $arguments) {
   

   Misspelt method name

4. +++ b/core/modules/system/src/Tests/Common/UrlTest.php
   @@ -9,7 +9,9 @@
   +use Drupal\Core\Cache\CacheableMetadata;
   

   Not used

5. +++ b/core/modules/system/tests/modules/early_rendering_controller_test/src/AttachmentsTestResponse.php
   @@ -0,0 +1,19 @@
   +use Drupal\Core\Cache\CacheableResponseTrait;
   

   Not used.

6. +++ b/core/modules/system/tests/modules/early_rendering_controller_test/src/EarlyRenderingTestController.php
   @@ -0,0 +1,100 @@
   +use Drupal\Component\Utility\SafeMarkup;
   ...
   +use Drupal\Core\Url;
   

   Not used

7. +++ b/core/modules/views/src/Tests/Plugin/CacheTest.php
   @@ -282,14 +283,18 @@ function testHeaderStorage() {
   +      return $renderer->render($output);;
   ...
   +      return $renderer->render($output);;
   

   Double ;;

#117: all fixed.

makes me think that we should have a followup meta to try to remove as much early rendering as possible.

We have that already: #2509534: [Meta] document or refactor calls to drupal_render().

This directly affects the request processing system and I still have outstanding concerns. Please don't RTBC it yet. (It's been less than 24 hours since the last response to me.)

Also change record need update for code examples - at least suggested way to use render context

#120: 99.9% of code does not need to change. That's the whole point of this issue: we wrap controllers so that not every controller has to do the perfectly correct thing to still be secure. Only a tiny fraction of code needs to use ::executeInRenderContext(). IMO the developers writing the code that needs that are capable of finding it in D8 core's codebase. An example in the CR is going to cause unnecessary concerns with developers reading it.

I personally would be okay to merge information into (cacheable + attachments) responses, too - if we can - and move the whole do-exception-on-early-render to D9 - if that is what is needed to get this in.

-1

The summary of this patch: We do support the crappy way of doing things for render arrays, and render arrays only. Everybody else has to behave. And render arrays will have to be have in D9 too. We only do this to ease the transition.

The quoted part means that controllers returning Response objects — which did not exist in D7, so there is no actual transition easing there — would be allowed to be equally messy as render arrays. We want to get rid of the messiness of render arrays, not have more of it!

We want to get rid of the messiness of render arrays, not have more of it!

Amen, reverend. :-P

Let me try and articulate why I'm still a bit uncomfortable here.

1) I understand the desire to avoid scope creep. However, prior to this patch controllers returned directly to the kernel, meaning a view listener could catch anything. We only had listeners in place to support some things, but technically any return could get handled by any view listener you wanted. With this patch, we're black listing some return values. That makes it an effective API change, albeit an edge-case one, and it means we need to be very deliberate about what we white/black list as a legal controller return.

2) Early rendering: Any turning of a render array into a string other than in the controller wrapper or (later) the block wrappers or the final view listener that produces the page.

The reason early rendering is dangerous is because we risk losing either assets or cache contexts along the way. When something is stringified, we need to capture all assets or cache contexts in order to ensure that A) No assets get left out if that portion of the page is loaded from cache and B) No cache contexts get missed, leading to inadvertent cache poisoning. A given render array that has neither assets or cache contexts wouldn't cause an issue. (Not that we want to encourage it, but it wouldn't actually break anything.)

So the real issue is capturing all assets and cache contexts that happen during a controller. Early rendering is just one way for those to happen. However, as Wim, effulgentsia, and I were discussing at the DC LA pre-sprint, those can come from all kinds of seriously weird places. We can say "if you're returning a non-render-array then we assume you're not screwing it up", but it could be something other than your code that is screwing it up. Eg, we were discussing outbound path processors that would affect *every* URL generated... even one for a RedirectResponse. That is, if your controller does nothing but return a RedirectResponse, you could still have a relevant cache context needed.

3) Now, we've said previously that as of CacheableResponseInterface, our cache system is opt-in. That allows someone to return a Response without that interface to take complete control and not get screwed up by our intricate caching system. That's good, but this patch explicitly blacklists CacheableResponseInterface as a legal controller result... that is, it makes it impossible for a controller to opt-in to the system we just made opt-in!

Quoting from #114:

But if your controller is returning not a (silly) render array, but a proper Response object, or a domain object… then we also expect you to be able to generate those Response or domain objects responsibly. If you Do The Right Thing and return a Response or a domain object, then we also expect you to do The Right Thing rendering-wise.

With this patch in its current state, if you're returning not-a-render-array, I couldn't even tell you how I would Do The Right Thing(tm). The options appear to be:

A) Do nothing and opt-out of caching entirely (frequently undesirable)
B) Set all of the appropriate cache headers yourself on a Response, but that would still not include any cache tags unless you did it all manually. (Tip: No one is going to do that.)
C) Early-render your ViewModel to a render array so that it gets past the checks, but now you're doing exactly what we told you not to do. Also, render arrays are *only* for HTML output. If you want to return anything else, you have to return your own response or a ViewModel, and of the two the ViewModel is the superior approach in most cases (IMO).

There is no option for "return a ViewModel that opts-in to caching". To give a concrete example, suppose I've a controller that renders a HAL response using not the Serializer but the Nocarrier/Hal library (a fairly good PHP library for HAL handling that I've used before and quite like). There are plenty of reasons to do that, and return an instance of a Hal object. Of course, the data that's put into that Hal object could come from 2 nodes and a user, and thus needs appropriate cache tags to ensure it gets cleared appropriately. How can I opt-in to the cache system? As written, I can neither render Hal to a Response myself (CacheableResponseInterface is blacklisted) nor subclass Hal to include tags. Too, we explicitly discard any uncaught cache information that may have leaked out, so I can't even rely on the system to help me. Basically, such a module is SOL.

tl;dr:

1) The patch assumes that "leaked metadata" only happens when someone is returning a render array *and* early-rendered something. I do not believe that is a safe assumption, which may lead to security issues.

2) The patch gives no way to opt-in to tag-based caching other than returning a render array, which is only for HTML pages. You can't even opt-in by returning a Response object with the appropriate interface, which you could before. That makes it a regression.

3) As Wim and I have both noted above, render arrays are messy and we want to move to something more structured in the future. Contrib can and should be able to experiment with that, either with generic ViewModels (eg, Carl's OOP Render API) or domain-specific ViewModels (eg, Nocarrier/Hal, HtmlFragment TNG, or whatever else). As written, this patch disallows such experimentation, or at least give such experiments no apparent way to produce safely-cacheable responses. That too is a regression, albeit a more implicit one.

As a bare minimum, I would remove the blacklisting of AttachmentInterface/CacheableResponseInterface. I believe that is counter-productive and a regression. We should not blacklist any controller result options unless we're certain that they are a security hole.

Second, we should check for uncaught metadata on all controller results, not just arrays. That said, I am not sure how to generically handle any metadata we find on arbitrary domain objects. That at least is not a regression, so solving that here is not a blocker, but at the very least we should document that fact and open a (non-critical) follow-up to look into it. Responses, though, we should try to handle if at all possible as that's a known quantity.

+++ b/core/lib/Drupal/Core/Render/RendererInterface.php
@@ -316,6 +322,37 @@ public function renderPlain(&$elements);
+   * All rendering must happen within a render context: within a render context,
+   * all bubbleable metadata is bubbled and hence tracked, without a render
+   * context, it would be lost. This could lead to missing assets, incorrect

Grammar problem. I think the comma in "hence tracked, without a render..." is supposed to be a period. Otherwise it's a run-on sentence.

This patch looks really solid. Thanks Wim for answering my questions on irc.
My main observation is this marks one of the few instances where we've consciously decided to babysit broken code.

First off: thank you very much for your very thorough review! :)

1) I understand the desire to avoid scope creep. However, prior to this patch controllers returned directly to the kernel, meaning a view listener could catch anything. We only had listeners in place to support some things, but technically any return could get handled by any view listener you wanted. With this patch, we're black listing some return values. That makes it an effective API change, albeit an edge-case one, and it means we need to be very deliberate about what we white/black list as a legal controller return.

I completely agree we need to be very careful.

But I think we are, because the blacklisting only activates if early rendering happened. If early rendering happened, it is guaranteed that that was in fact masking a bug.

We can say "if you're returning a non-render-array then we assume you're not screwing it up", but it could be something other than your code that is screwing it up.

(Emphasis in the quote is mine.)
We don't assume you're not screwing up, we disallow (and hence prevent) you from screwing up. We detect early rendering for all controllers (returning anything), but we only fix things automatically if the return value was a render array.

Eg, we were discussing outbound path processors that would affect *every* URL generated...

This is why we're making sure in #2351015: URL generation does not bubble cache contexts that we're capturing all cacheability metadata of URL generation — even that of early rendering.

… even one for a RedirectResponse.

Indeed. But a RedirectResponse does not implement CacheableResponseInterface, which means we can conclude this Response will not be cacheable. If/once RedirectResponse implemented CacheableResponseInterface (or we'd add a CacheableRedirectResponse class), then we'd throw an exception in that case too.

3) Now, we've said previously that as of CacheableResponseInterface, our cache system is opt-in. That allows someone to return a Response without that interface to take complete control and not get screwed up by our intricate caching system. That's good, but this patch explicitly blacklists CacheableResponseInterface as a legal controller result... that is, it makes it impossible for a controller to opt-in to the system we just made opt-in!

This is incorrect.
This patch does not blacklist CacheableResponseInterface at all. All it does, is not allow ("blacklist") a CacheableResponseInterface value to be returned if early rendering happened. Because if early rendering happened, then we know for a fact that the cacheability metadata on this response is in fact wrong (i.e. incomplete). The controller returning the CacheableResponseInterface value should either stop doing early rendering, which would ensure the cacheability metadata is complete, or it should wrap its early rendering in a RendererInterface::executeInRenderContext() call. Then it would have caught the cacheability metadata of its own early rendering, and things would be safe again, and if (!$context->isEmpty()) { in our wrapping subscriber would evaluate to FALSE.

Suspected misunderstanding :)

At this point, I suspect you maybe misread the definition of "early rendering": early rendering is the calling of drupal_render()/RendererInterface::render() when no render context is active. For clarity:

1. If your controller calls RendererInterface::render(Root|Plain)(), you're good. (This is already the case in HEAD.)
2. If your controller calls drupal_render()/RendererInterface::render() WITH somewhere up in the PHP call stack there being a RendererInterface::executeInRenderContext() call, you're good. Because rendering is happening INSIDE of a render context.
3. If your controller calls drupal_render()/RendererInterface::render() WITHOUT somewhere up in the PHP call stack there being a RendererInterface::executeInRenderContext() call, you're NOT good. Because rendering is happening OUTSIDE of a render context.

If that's indeed the cause for the misunderstanding, let's document that more clearly. I've already made clarifications in this reroll.
(This is why all of the wrapping subscriber's blacklisting logic is wrapped in that if (!$context->isEmpty()) { call.)

With this patch in its current state, if you're returning not-a-render-array, I couldn't even tell you how I would Do The Right Thing(tm). The options appear to be:

A) Do nothing and opt-out of caching entirely (frequently undesirable)
B) Set all of the appropriate cache headers yourself on a Response, but that would still not include any cache tags unless you did it all manually. (Tip: No one is going to do that.)
C) Early-render your ViewModel to a render array so that it gets past the checks, but now you're doing exactly what we told you not to do. Also, render arrays are *only* for HTML output. If you want to return anything else, you have to return your own response or a ViewModel, and of the two the ViewModel is the superior approach in most cases (IMO).

(First, for clarity, repeating your definition of "ViewModel" from #113: Consider the case of a controller that renders a ViewModel object that depends on 2 nodes. It should be cache tagged with both of those nodes.)

A) Indeed.
B) Indeed.
C) No. There are no checks to get past. You clearly want your ViewModel object to carry along the associated cacheability metadata. So: class ViewModel implements CacheableDependencyInterface. It'd then be up to your view subscriber to take that ViewModel object and turn it into a response (and that would likely be a CacheableResponseInterface response, but not necessarily.) But I think your key concern is that as part of generating that ViewModel object, a URL is generated (e.g. \Drupal::url('some_route')) and/or some rendering happens (e.g. drupal_render(entity_view(Node::load(5), 'full'))), and that because of that early rendering, the ViewModel object would be blacklisted by the wrapping subscriber. The answer is simple: if you're doing that, then wrap it in a render context, and you'll be fine. Our wrapping subscriber merely helps prevent such bugs!

As written, I can neither render Hal to a Response myself (CacheableResponseInterface is blacklisted)

This is not true, either use renderRoot()/renderPlain(), which render things in a render context automatically, or wrap it in a executeInRenderContext() call.
(This further convinces me that the "Suspected misunderstanding" section is indeed the misunderstanding.)

1) The patch assumes that "leaked metadata" only happens when someone is returning a render array *and* early-rendered something. I do not believe that is a safe assumption, which may lead to security issues.

This is wrong twice: A) we always check for leaked metadata, regardless of the controller's return value, B) leaked metadata does only happening when doing early rendering. We can detect it with complete certainty. (When combined with #2351015: URL generation does not bubble cache contexts, which also captures URL/link cacheability metadata that is being leaked.)
I completely agree that if those assumptions were in fact the case, that we'd have security issues.

2) The patch gives no way to opt-in to tag-based caching other than returning a render array, which is only for HTML pages. You can't even opt-in by returning a Response object with the appropriate interface, which you could before. That makes it a regression.

This is also wrong twice:
A) this patch does not change anything about opting in to tag-based caching. Both before and after this patch, it is FinishResponseSubscriber that checks if the response is an instance of CacheableResponseInterface and if so, generates the headers for you based on the cacheability metadata in the Response object
B) your controller can return either a CacheableResponseInterface Response directly (which would opt in to tag-based caching) or any domain object (e.g. ViewModel), and it's then up to the corresponding view subscriber to turn the domain object into a CacheableResponseInterface Response. The domain object may or may not implement CacheableDependencyInterface to send along cacheability metadata. Perhaps it's in the view subscriber that all the cacheability metadata comes into play — the domain object could very well be sufficiently independent that it doesn't need cacheability metadata. In other words: you have full freedom; you're not at all restricted to render arrays
C) Therefore, there is no regression, because frankly, none of this statement made sense.

I think Crell actually wanted to say "the patch gives no way in to opt in controller return values other than render arrays to have early rendering metadata merged in automatically". That statement would've made sense. So I'll answer that too. But in doing so, I have to basically repeat everything I said in #114.

1. We expect controllers returning Responses and domain objects to behave. I.e. to not take over the irresponsible early rendering that controllers returning render arrays have.
2. You're asking for responses/objects implementing AttachmentsInterface or CacheableResponseInterface to automatically have the leaked bubbleable metadata merged as well. That conflicts with the above.
3. While it is technically possible for AttachmentsInterface or CacheableResponseInterface since they have setters, it's impossible for CacheableDependencyInterface, which your domain objects (e.g. ViewModel) would be using.
4. So the "regression" you talk about is literally just an exception pointing at previously masked bugs in the controllers' code. Not throwing that exception would violate point 1.
5. This is why I asked in #114 to defer this discussion to a follow-up: it means actively babysitting broken code in what is supposed to be clean D8 code, rather than just babysitting broken code in render arrays, which we specifically choose to do for easing the transition from D7.

As written, this patch disallows such experimentation, or at least give such experiments no apparent way to produce safely-cacheable responses. That too is a regression, albeit a more implicit one.

Per all of the above, this is incorrect.

As a bare minimum, I would remove the blacklisting of AttachmentInterface/CacheableResponseInterface. I believe that is counter-productive and a regression. We should not blacklist any controller result options unless we're certain that they are a security hole.

While possible, it comes with caveats (see the ordered list with 5 points just a bit higher). You're literally asking for a non-explicit, automagical solution to babysit broken code that is newly written for Drupal 8! Color me confused :)

Second, we should check for uncaught metadata on all controller results, not just arrays.

Again, this is what we already do. See if (!$context->isEmpty()) {.

In this reroll:

1. EarlyRenderingControllerWrapperSubscriber class documentation clarified.
2. EarlyRenderingControllerWrapperSubscriber::wrapControllerExecutionInRenderContext() documentation clarified further, added an explicit is_object() check and now also checking for CacheableDependencyInterface (which may be implemented by domain objects).
3. Fixed #125.
4. Expanded test coverage to also demonstrate all cases for domain objects, and prove that they in fact are allowed to be turned into cacheable responses.

#126: Thanks for the review, @larowlan!

Indeed: My main observation is this marks one of the few instances where we've consciously decided to babysit broken code. — and my concern is that by doing what Crell is asking for, we're choosing to babysit even more broken code.

The current patch is IMO reasonably babysitting: controllers returning render arrays are encouraged to not do any rendering. That's taken care of for them. If they do early rendering anyway, that causes problems. Historically, we have a lot of that code. So this patch makes the transition easier to D8.
But if we also do what Crell is asking for, then we go beyond what's IMHO reasonable: there is no reason why controllers returning Response or domain objects should be doing early rendering — and note again that if they do renderRoot() or renderPlain(), they are NOT doing early rendering.

This makes me wonder if this is just a naming problem: what if we rename "early rendering" to "contextless rendering"? (Then: EarlyRenderingControllerWrapperSubscriber -> ContextlessRenderingControllerWrapperSubscriber.)

The is_object is not needed.

Oops. Fixed.

#130: I disagree with all of that. Again, this means literally babysitting broken code, and violating the "Response objects contain everything to send a HTTP response" principle.

Even for CacheableDependencyInterface we could still think about it.
No, this we absolutely cannot do. It's designed to have getters only. It explicitly cannot have setters. Otherwise anybody can go and overwrite the cacheability of all sorts of things. Then all the cacheability work we've done is effectively a house of cards.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php
   @@ -0,0 +1,166 @@
   +      elseif ($response instanceof AttachmentsInterface || $response instanceof CacheableResponseInterface || $response instanceof CacheableDependencyInterface) {
   +        throw new \LogicException(sprintf('Early rendering is not permitted for controllers returning anything else than render arrays. Returned object class: %s.', get_class($response)));
   +      }
   

   Yeah I agree with crell, this discourages a better future.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -58,11 +58,24 @@ class Renderer implements RendererInterface {
   +   */
   +  protected static $context;
   +
   

   It would be great to have a todo to get rid of the static ...

3. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -263,7 +310,9 @@ public function execute() {
   +    $build['#markup'] = $this->renderer->executeInRenderContext(new RenderContext(), function() {
   +      return $this->view->style_plugin->render();
   +    });
   

   I try to understand that change. The style plugin is the serializer, which does not expose any kind of bubbleable metadata ... so this change is kinda odd.

4. +++ b/core/modules/system/src/Tests/Common/EarlyRenderingControllerTest.php
   @@ -0,0 +1,92 @@
   +class EarlyRenderingControllerTest extends WebTestBase {
   ...
   +  function testEarlyRendering() {
   

   All this test code looks pretty much like it could live happily in a kernel test, which would actually add an additional level of test coverage. It ensures that the http kernel + renderer works for itself, and don't produce any state which is not reset.

1. The opposite is true. We can choose to be less restrictive in the future. See my above comments…
2. If somebody can send me the link to the issue that is fixing the container rebuilding craziness, then I'd be happy to link to it. I haven't been able to find it. At least now the reason is fully explained/documented.
3. Because the serializer style plugin calls things that do perform early rendering. I made this change in #82, you can compare with the failures prior to that comment/patch.
4. I don't see how; we need to be able to perform requests.

Wim talked me through the details of the wrapper and rendering on Skype the other day. To clarify for anyone else that had similar concerns (or has them in the future and finds this issue):

1) All of our fancy caching is opt-in, via one of a couple of interfaces. (CacheableResponseInterface and CacheableDependencyInterface, specifically.)

2) If you don't use one of those, it doesn't matter if any cache metadata leaks because it won't get used anyway.

3) If you do use one of those, it means you are providing all of the necessary metadata yourself. If something still leaked, then you have a bug.

4) If you only ever use renderRoot() or renderPlain(), nothing should leak *except* for OutboundUrlProcessors. However, that already has a critical: #2351015: URL generation does not bubble cache contexts

5) According to Wim, there is no other way to leak data beyond that existing critical or using render() yourself. If you do the latter, you're wrong and you should fix your code. This is the part that I was most nervous about. I am going to take Wim's word for it that there is no other possible way to leak metadata. If it turns out there is, that's probably a critical in its own right as well.

6) If you opt-in with one of those interfaces, and nothing leaks, everything works fine and you can return anything.

7) Leaky metadata with a render array return is allowed, because everyone's doing it.

(I'm sure someone is going to say "I just said that in an earlier comment!" Well, it's a complicated issue and the documentation is imperfect, because it's a complicated issue. Ask Wim how many times it took for him to explain it before I finally caught on. :-P)

Therefore, I'm now on board with this issue. My only remaining pushback would be to improve the LogicException message, as even knowing the above now the message is still not helpful. I would suggest instead "The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early."

That provides better guidance on what the developer is doing wrong and what to do about it, and documents for future stubborn-minded folk (like me) the limited case in which that exception would actually happen. :-)

Modulo that fix, this has my +1.

Wim++

Regarding #131 I opened up a follow up: #2525200: Move EarlyRenderingControllerTest to a KernelTest in order to ensure no state

#133 in other words, this can be done by wim now again ...

#133: Thank you — for writing that overview and your great constructive criticism :)

So, per #133, I updated the exception message. But in doing so, I noticed that the assertResponse() assertions in EarlyRenderingControllerTest were wrong. I thought the second parameter was asserting the presence of content, but it was just a message to associate with the assertion. Oops. Split them all up into assertReponse() + assertRaw() assertions. Which made me see that one "Hello world" was not showing due to a small oversight.

+++ b/core/modules/system/src/Tests/Common/EarlyRenderingControllerTest.php
@@ -34,55 +34,69 @@ class EarlyRenderingControllerTest extends WebTestBase {
-    $this->assertResponse(200, 'Hello world!');
+    $this->assertResponse(200);
+    $this->assertRaw('Hello world!');
     $this->assertCacheTag('foo');

Nice!!

So, this is nice, #2309215: HTML double-escaping in revision messages introduced a new early rendering occurrence, and testbot just detected that for us with an exception. :) Fixing it now.

This fixes it, but the underlying problem is that Twig's {% trans %} doesn't support render arrays. We should fix that at #2334319-7: {% trans %} does not support render array and MarkupInterface valued expressions, then all this ugliness goes away :)

It is great to find those failures now!

I first uploaded a wrong patch, then fixed it. The patch in #141 is correct. I wonder if testbot somehow got the previously uploaded patch? Re-uploading the same patch.

Found it. The early rendering was not happening a few lines later. It was happening in a test.

RTBC +1

1. This patch is a major jump forward for locking down the render system before we are rendering. Once we achieve this for reals our lives will be much easier - but this is an awesome place for Drupal 8 to get to.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -188,17 +188,20 @@ public function revisionOverview(NodeInterface $node) {
          $row = [];
   -      $row[] = [
   +      $column = [
            'data' => [
              '#type' => 'inline_template',
              '#template' => '{% trans %}{{ date }} by {{ username }}{% endtrans %}{% if message %}<p class="revision-log">{{ message }}</p>{% endif %}',
              '#context' => [
                'date' => $link,
   -            'username' => $this->renderer->render($username),
   +            'username' => $this->renderer->renderPlain($username),
                'message' => Xss::filter($revision->revision_log->value),
              ],
            ],
          ];
   +      // @todo Simplify once https://www.drupal.org/node/2334319 lands.
   +      $this->renderer->addCacheableDependency($column['data'], $username);
   +      $row[] = $column;
   

   Why are we creating the $column variable?

3. +++ b/core/modules/system/tests/modules/early_rendering_controller_test/src/EarlyRenderingTestController.php
   @@ -0,0 +1,129 @@
   +/**
   + * Controller routines for early_rendering_test routes.
   + */
   +class EarlyRenderingTestController extends ControllerBase  {
   

   Lets add an @see to the relevant test.

4. +++ b/core/modules/system/tests/modules/early_rendering_controller_test/src/EarlyRenderingTestController.php
   @@ -0,0 +1,129 @@
   +  protected function earlyRenderContent() {
   ...
   +  public function renderArray() {
   ...
   +  public function renderArrayEarly() {
   ...
   +  public function response() {
   ...
   +  public function responseEarly() {
   ...
   +  public function responseWithAttachments() {
   ...
   +  public function responseWithAttachmentsEarly() {
   ...
   +  public function cacheableResponse() {
   ...
   +  public function cacheableResponseEarly() {
   ...
   +  public function domainObject() {
   ...
   +  public function domainObjectEarly() {
   ...
   +  public function domainObjectWithAttachments() {
   ...
   +  public function domainObjectWithAttachmentsEarly() {
   ...
   +  public function cacheableDomainObject() {
   ...
   +  public function cacheableDomainObjectEarly() {
   

   Our docs standards say that this methods should have docs.

1. :)
2. To be able to do
   

   +      // @todo Simplify once https://www.drupal.org/node/2334319 lands.
   +      $this->renderer->addCacheableDependency($column['data'], $username);
    

   Because previously the render array is assigned to $row[], i.e. appended to the $row, we're not able to add a cacheable dependency to it without first calculating the index of the last added column. This seemed a clearer solution.

3. Done.
4. Done (and discussed in person).

Thanks for adding the beta evaluation to the issue summary. Committed ebb21d2 and pushed to 8.0.x. Thanks!

this change record still a draft.

@Wim Leers made it in #109
and
@andypost and @Wim Leers discussed in #120 and #121 if additional examples were needed.

can we publish it?

This paragraph in the CR is a bit confusing :
"Note that in HEAD, early rendering's bubbleable metadata is lost, which can lead to security problems (due to missing cache tags/contexts on the response)."

I guess that means "in HEAD before the patch was committed", but that is misleading in a change record that is here to stay :-)
Do we really need that paragraph ? It seems like it was meaningful in the context of the issue / issue summary, but in the CR now that it's fixed ?