Problem/Motivation

There are several things like token processing, CSRF links, and potentially other things, where metadata gets lost due to Drupal's usage of strings.

If the user is within a 'rendering context' (e.g. the renderer is active and there is a stack frame) then this meta data gets nicely bubbled up and stored.

However, there are cases, where there is no such stack and then this data gets lost, which is a potential security issue. (i.e. if cacheability metadata is lost, then cache contexts are lost, which means that Drupal may end up caching without taking those cache contexts into account, leading to information disclosure.)

Solving this would also solve #2351015: URL generation does not bubble cache contexts.

Proposed resolution

  • Enforce a rendering context as mandatory. ::renderRoot() and ::renderPlain() do this automatically, so 99.9% of code doesn't need to change.
  • Provide controllers with a default rendering context, that is used and merged when they return a render array. (See EarlyRenderingControllerWrapper.)

Remaining tasks

Review.

User interface changes

None.

API changes

  1. All rendering must happen within a render context. Most code is unaffected. Affected code was insecure. (See beginning of issue summary.)
  2. Added RendererInterface::executeInRenderContext(RenderContext $context, calllable $callable).

Note: 99% of code does not need to change at all.. The only 3 things (besides tests, which often are testing things that only are ever executed within a render context, so those tests need to do so as well) using ::executeInRenderContext():

  1. EarlyRenderingControllerWrapper (newly added)
  2. HtmlRenderer (which is doing something very advanced)
  3. RestExport (which is also doing something advanced/crazy: using the render system to generate JSON/JSON-LD/HAL/… responses)

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because functionality is broken and metadata is lost.
Issue priority Critical because of security implications of lost cacheability metadata.
Prioritized changes The main goal of this issue is security to help with being able to make more things cacheable.
Disruption Potentially disruptive as e.g. calling \Drupal::l() during some event will throw an Exception - if there is no rendering context. This might be especially happening during cron().

Original report

Problem/Motivation

The Render Stack works in the following way:

Anything rendered while the stack is active (e.g. while we are in a root call ) is put on the stack, then merged later, when coming back.

That means any Controller or PageDisplayVariant renderable that uses #pre_render is safe.

However a Controller that uses drupal_render to render something early, which can be as simple as calling \Drupal:l() once its cache context aware, won't have their meta data stored on the stack.

This gives two problems:

a) It makes cacheable CSRF for urls or links more difficult (the same for cache contexts)
b) It leads to a different path when #pre_render is used and when its not used
c) It makes it difficult to automatically send Cache-Tags and Contexts for JSON responses.

Here is a test-case, which I need to make into a real test:


function render_it(&$build, $tag_name = 'late') {

  $some_build = [
    '#cache' => [
      'tags' => [$tag_name],
    ],
    '#markup' => 'M:' . $tag_name,
  ];

  $build['#markup'] = drupal_render($some_build);
}

function c1() {
  $build = [];
  render_it($build, 'early');
  return $build;
}

function c2() {
  return [ '#pre_render' => ['render_it'] ];
}

$page['region']['early'] = c1();
$page['region']['late'] = c2();

drupal_render_root($page);

// This is only returning 'late'
print_r($page['#cache']['tags']);

Note: Controllers in general should avoid early rendering. If someone needs to render something early outside of the request, they should use $renderer->renderPlain(), which runs #post_render_cache and has an independent stack, etc.

Proposed resolution

- Introduce a scope into the Renderer service that is defined in two stages:

a) REQUEST: Collect rendered meta data into the $request->getCurrentRequest()->renderMetaData object / array

This is the default and start mode.

b) RENDER: Second, merge back this render data into the main request at the top, so that its processed first - as if it was already on the stack, set the Renderer to be in RENDER mode (mainly to save performance as merging is costly - can be omitted first).

In case for a JSON response we could get the Cache-Tags and Contexts from the $request->renderMetaData and set the appropriate HTTP headers, but that would be follow-up and needs broader discussion.

---

To explain from a different angle, a render stack is active as long as it is called recursively, therefore for the HtmlRender, we have the following Call Stack == Render Stack:

We have the MainContent returned from the Controller.

HtmlRenderer -> Renderer -> render() -> MainContent

Then the meta data and markup is extracted from Main Content and put into Rendered Main Content, which is then placed in the page.

HtmlRenderer -> Renderer -> render() -> Page -> Block 1, Block 2, Block 3, Renderered Main Content

And because the BlockViewBuilder enforces the #pre_render pattern, this is safe.

But for the controller this is different and we end up with N stacks that are created and destroyed:

1. Controller -> Renderer-> render()
2. Controller -> Renderer-> render()
3. Controller -> Renderer-> render()
...

Then we have finally get back the main content, which misses this data:

HtmlRenderer -> Renderer -> render() -> MainContent

However if we store this data from the N stacks and populate it into MainContent before rendering the main content, then the result is the exact same as if the controller was using the #pre_render pattern (which we can't enforce and which would be a DX problem).

And therefore we have consistency again.

And the request object is the perfect place for that, because the controller is tied to the request as well.

And then while sending the response we could get the cache tags and cache contexts from the request as well :).

Remaining tasks

- Create proof of concept

User interface changes

API changes

CommentFileSizeAuthor
#152 interdiff.txt1.2 KBWim Leers
#152 rendered_cache_metadata-2450993-152.patch115.4 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,438 pass(es). View
#148 interdiff.txt709 bytesWim Leers
#148 rendered_cache_metadata-2450993-148.patch115.02 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,402 pass(es). View
#145 rendered_cache_metadata-2450993-145.patch114.37 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,372 pass(es), 0 fail(s), and 1 exception(s). View
#141 interdiff.txt1.27 KBWim Leers
#141 rendered_cache_metadata-2450993-141.patch114.37 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,364 pass(es), 0 fail(s), and 1 exception(s). View
#136 interdiff.txt8.14 KBWim Leers
#136 rendered_cache_metadata-2450993-136.patch113.15 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,366 pass(es), 0 fail(s), and 1 exception(s). View
#130 interdiff.txt1.35 KBWim Leers
#130 rendered_cache_metadata-2450993-130.patch112.47 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,379 pass(es). View
#127 interdiff.txt18.4 KBWim Leers
#127 rendered_cache_metadata-2450993-126.patch112.49 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,362 pass(es). View
#118 interdiff.txt6.23 KBWim Leers
#118 rendered_cache_metadata-2450993-118.patch102.97 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,377 pass(es). View
#108 rendered_cache_metadata-2450993-108.patch103.39 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,315 pass(es). View
#108 interdiff.txt12.9 KBWim Leers
#108 rendered_cache_metadata-2450993-108-test-only.patch11 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,272 pass(es), 3 fail(s), and 1 exception(s). View
#105 interdiff.txt8.76 KBWim Leers
#105 rendered_cache_metadata-2450993-105.patch91.97 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,238 pass(es). View
#98 rendered_cache_metadata-2450993-98.patch91.01 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,301 pass(es). View
#93 rendered_cache_metadata-2450993-93-spinoff-do-not-test.patch168.69 KBWim Leers
#93 rendered_cache_metadata-2450993-93-logical_changes.patch91.28 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,969 pass(es), 136 fail(s), and 187 exception(s). View
#92 interdiff.txt9.31 KBWim Leers
#92 rendered_cache_metadata-2450993-92.patch265.36 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,292 pass(es). View
#91 interdiff.txt3.41 KBWim Leers
#91 rendered_cache_metadata-2450993-91.patch263.46 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,228 pass(es). View
#89 interdiff.txt1.47 KBWim Leers
#89 rendered_cache_metadata-2450993-89.patch260.11 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,205 pass(es), 7 fail(s), and 0 exception(s). View
#87 interdiff.txt1.47 KBWim Leers
#87 rendered_cache_metadata-2450993-87.patch259.29 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch rendered_cache_metadata-2450993-87.patch. Unable to apply patch. See the log in the details link for more information. View
#84 interdiff.txt745 bytesWim Leers
#84 rendered_cache_metadata-2450993-84.patch258.23 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,208 pass(es), 8 fail(s), and 0 exception(s). View
#82 interdiff.txt23.98 KBWim Leers
#82 rendered_cache_metadata-2450993-82.patch257.54 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,196 pass(es), 35 fail(s), and 8 exception(s). View
#79 interdiff.txt1.65 KBWim Leers
#79 rendered_cache_metadata-2450993-79.patch233.66 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,006 pass(es), 35 fail(s), and 23 exception(s). View
#78 interdiff.txt5.91 KBWim Leers
#78 rendered_cache_metadata-2450993-78.patch233.13 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,967 pass(es), 96 fail(s), and 23 exception(s). View
#75 interdiff.txt53.77 KBWim Leers
#75 rendered_cache_metadata-2450993-75.patch227.65 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,777 pass(es), 103 fail(s), and 33 exception(s). View
#73 interdiff.txt3.27 KBWim Leers
#73 rendered_cache_metadata-2450993-72.patch176.17 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,618 pass(es), 110 fail(s), and 45 exception(s). View
#70 interdiff.txt6.09 KBWim Leers
#70 rendered_cache_metadata-2450993-70.patch172.98 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,609 pass(es), 138 fail(s), and 45 exception(s). View
#66 interdiff.txt14.83 KBWim Leers
#66 rendered_cache_metadata-2450993-66.patch167.32 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,580 pass(es), 190 fail(s), and 45 exception(s). View
#64 interdiff.txt1.25 KBWim Leers
#64 rendered_cache_metadata-2450993-64.patch152.56 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,863 pass(es), 253 fail(s), and 77 exception(s). View
#61 interdiff.txt1.25 KBWim Leers
#61 rendered_cache_metadata-2450993-61.patch154.38 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch rendered_cache_metadata-2450993-61.patch. Unable to apply patch. See the log in the details link for more information. View
#60 interdiff.txt5.1 KBWim Leers
#60 rendered_cache_metadata-2450993-60.patch154.53 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,860 pass(es), 263 fail(s), and 78 exception(s). View
#57 interdiff.txt631 bytesWim Leers
#57 rendered_cache_metadata-2450993-57.patch154.28 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,671 pass(es), 312 fail(s), and 81 exception(s). View
#54 interdiff.txt16.47 KBWim Leers
#54 rendered_cache_metadata-2450993-54.patch154.24 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,354 pass(es), 377 fail(s), and 85 exception(s). View
#51 interdiff.txt13.77 KBWim Leers
#51 rendered_cache_metadata-2450993-51.patch146.77 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,873 pass(es), 263 fail(s), and 77 exception(s). View
#49 rendered_cache_metadata-2450993-48.patch135.12 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,784 pass(es), 1,019 fail(s), and 166 exception(s). View
#5 rendered_cache_metadata-2450993-5.patch8.99 KBFabianx
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#8 rendered_cache_metadata-2450993-8.patch12.33 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,789 pass(es), 3,590 fail(s), and 1,023 exception(s). View
#8 interdiff.txt3.49 KBWim Leers
#16 rendered_cache_metadata-2450993-16.patch13.29 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,530 pass(es), 2,198 fail(s), and 407 exception(s). View
#16 interdiff.txt1.01 KBWim Leers
#17 rendered_cache_metadata-2450993-17.patch27.77 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,622 pass(es), 2,090 fail(s), and 372 exception(s). View
#17 interdiff.txt14.56 KBWim Leers
#30 rendered_cache_metadata-2450993-30.patch26.97 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,554 pass(es), 1,634 fail(s), and 299 exception(s). View
#33 rendered_cache_metadata-2450993-32.patch36.51 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,302 pass(es), 1,587 fail(s), and 265 exception(s). View
#33 interdiff.txt9.63 KBWim Leers
#36 rendered_cache_metadata-2450993-36.patch40.6 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,345 pass(es), 1,564 fail(s), and 263 exception(s). View
#36 interdiff.txt5.38 KBWim Leers
#38 rendered_cache_metadata-2450993-38.patch44 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,168 pass(es), 1,105 fail(s), and 260 exception(s). View
#38 interdiff.txt3.5 KBWim Leers
#44 rendered_cache_metadata-2450993-44.patch136.22 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,751 pass(es), 1,018 fail(s), and 166 exception(s). View
#44 interdiff.txt96.02 KBWim Leers
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Fabianx’s picture

Assigned: Unassigned » Fabianx

Notes from IRC for myself:

$request→something = $request→something→merge(BM::createFromRenderArray(…))

Wim Leers’s picture

Another example that currently does this: Search module.

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

Fabianx’s picture

I have a new and much simpler idea to solve this:

Have:

- renderRoot() construct the static::$stack
- if render() is called without being in a render context(), throw a helpful Exception - as there is _no_ stack to render on.
- Fix core

Contrib needs to fix itself, but that is good, because currently if you render early without using renderPlain() that is plain unsupported anyway.

--

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)

Fabianx’s picture

Status: Active » Needs review
FileSize
8.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Here is a patch implementing #4.

This is fully compatible with the approach to return a value object from renderPlain() and renderRoot().

We could even think if we want to retire renderPlain() again (after it returns a value object) as now renderPlain() == renderRoot() except for the Exception ...

Lets see how much of core tests break, the unit tests for the Render group are already fixed :).

Status: Needs review » Needs work

The last submitted patch, 5: rendered_cache_metadata-2450993-5.patch, failed testing.

Wim Leers’s picture

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
12.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,789 pass(es), 3,590 fail(s), and 1,023 exception(s). View
3.49 KB

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.

Wim Leers’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 8: rendered_cache_metadata-2450993-8.patch, failed testing.

Wim Leers’s picture

Assigned: Fabianx » effulgentsia

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.

Fabianx’s picture

I agree, I will also ping catch. :)

effulgentsia’s picture

  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.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned

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.

Wim Leers’s picture

  1. 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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
13.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,530 pass(es), 2,198 fail(s), and 407 exception(s). View
1.01 KB

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)

Wim Leers’s picture

FileSize
27.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,622 pass(es), 2,090 fail(s), and 372 exception(s). View
14.56 KB

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

The last submitted patch, 16: rendered_cache_metadata-2450993-16.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 17: rendered_cache_metadata-2450993-17.patch, failed testing.

Fabianx’s picture

#15.3:

There is 2 cases where a renderPlain() / renderIsolated() is needed:

- I need a string + some assets for rendering something out-of-bounds, then e.g. sending a remote RPC call, or anything ... I don't care about cacheability, so run all #post_render_cache callbacks.

- I need to render something early due to architecture, but that does not yet mean it will be actually displayed (views fields is such a case, which use $field->theme()).

There it would be helpful if renderInIsolation() would actually return a cacheable render array directly, so that the markup and metadata are preserved and no #post_render_cache callbacks have been run, yet.

Yes, it would be nicer to not render at all, but that is sometimes difficult and that is the use case that $is_root_false = FALSE.

--

Overall +1 to fixing core in this way:

It makes it absolutely simple to get to the next step of allowing cache tags on e.g. CacheableJsonResponse by just grepping for renderRoot(), similar renderPlain() points to early rendering and we can then on a case-by-case basis decide if we need to call #post_render_cache or not.

Which is way way way better than before, where you had to check every single ->render() call.

Fabianx’s picture

Initial review:

  1. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
    @@ -197,15 +197,15 @@ protected function ajaxRender(Request $request) {
    -      $resource_commands[] = new AddCssCommand($renderer->render($css_render_array));
    +      $resource_commands[] = new AddCssCommand($renderer->renderPlain($css_render_array));
    
    +++ 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);
    

    When we renderPlain we also need to take into account any metadata that is in $build and add a dependency.

    We can do that as a follow-up however, too.

    e.g.

    $page['#title'] = $this-
    >renderer->renderPlain($build);
    // @todo Should live in the #title property instead.
    $this->renderer->addDependency($page, BubbleableMetadata::createFromRenderArray($build));
    
  2. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRenderer.php
    @@ -59,12 +59,12 @@ public function renderBarePage(array $content, $title, $page_theme_property, arr
         // Add the bare minimum of attachments from the system module and the
         // current maintenance theme.
         system_page_attachments($html['page']);
    -    return $this->renderer->render($html);
    +    return $this->renderer->renderPlain($html);
    

    Is that not renderRoot as well?

    Else we loose the attachments, don't we?

effulgentsia’s picture

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.

effulgentsia’s picture

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

Crell’s picture

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.

Fabianx’s picture

Priority: Major » Critical
Issue summary: View changes
Fabianx’s picture

Issue tags: +security

Promoted this one to critical and demoted #2351015: URL generation does not bubble cache contexts instead.

Fabianx’s picture

The main justification for this being critical is token handling.

I see no way to support token_replace without having a rendering context / stack active.

effulgentsia’s picture

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

effulgentsia’s picture

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 :)

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
FileSize
26.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,554 pass(es), 1,634 fail(s), and 299 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 30: rendered_cache_metadata-2450993-30.patch, failed testing.

Wim Leers’s picture

#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 :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
36.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,302 pass(es), 1,587 fail(s), and 265 exception(s). View
9.63 KB

Less failures.

Status: Needs review » Needs work

The last submitted patch, 33: rendered_cache_metadata-2450993-32.patch, failed testing.

Wim Leers’s picture

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
40.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,345 pass(es), 1,564 fail(s), and 263 exception(s). View
5.38 KB

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!)

Status: Needs review » Needs work

The last submitted patch, 36: rendered_cache_metadata-2450993-36.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,168 pass(es), 1,105 fail(s), and 260 exception(s). View
3.5 KB

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.)

dawehner’s picture

(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!

Status: Needs review » Needs work

The last submitted patch, 38: rendered_cache_metadata-2450993-38.patch, failed testing.

Wim Leers’s picture

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

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Render/Element/Actions.php
@@ -94,7 +94,7 @@ public static function preRenderActionsDropbutton(&$element, FormStateInterface
-        $button = drupal_render($element[$key]);
+        $button = \Drupal::service('renderer')->renderPlain($element[$key]);

IIRC, title can now also contain a render array, so possibly using $element['key'] might just work (tm).

andypost’s picture

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
136.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,751 pass(es), 1,018 fail(s), and 166 exception(s). View
96.02 KB

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

Status: Needs review » Needs work

The last submitted patch, 44: rendered_cache_metadata-2450993-44.patch, failed testing.

Wim Leers’s picture

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

Wim Leers’s picture

Status: Needs work » Needs review

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.

Wim Leers’s picture

FileSize
135.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,784 pass(es), 1,019 fail(s), and 166 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 49: rendered_cache_metadata-2450993-48.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
146.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,873 pass(es), 263 fail(s), and 77 exception(s). View
13.77 KB

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.)

Status: Needs review » Needs work

The last submitted patch, 51: rendered_cache_metadata-2450993-51.patch, failed testing.

Wim Leers’s picture

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

YAY!

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
154.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,354 pass(es), 377 fail(s), and 85 exception(s). View
16.47 KB

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.

Fabianx’s picture

#54 looks like sweet tasty ice-cream :DDD.

Status: Needs review » Needs work

The last submitted patch, 54: rendered_cache_metadata-2450993-54.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
154.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,671 pass(es), 312 fail(s), and 81 exception(s). View
631 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 57: rendered_cache_metadata-2450993-57.patch, failed testing.

Crell’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
154.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,860 pass(es), 263 fail(s), and 78 exception(s). View
5.1 KB

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

Wim Leers’s picture

FileSize
154.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch rendered_cache_metadata-2450993-61.patch. Unable to apply patch. See the log in the details link for more information. View
1.25 KB

Oops, two debug thingies snuck in.

Status: Needs review » Needs work

The last submitted patch, 61: rendered_cache_metadata-2450993-61.patch, failed testing.

The last submitted patch, 60: rendered_cache_metadata-2450993-60.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
152.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,863 pass(es), 253 fail(s), and 77 exception(s). View
1.25 KB

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 :)

Wim Leers’s picture

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().

Wim Leers’s picture

FileSize
167.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,580 pass(es), 190 fail(s), and 45 exception(s). View
14.83 KB

This should fix a whole lot of the failures.

The last submitted patch, 64: rendered_cache_metadata-2450993-64.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 66: rendered_cache_metadata-2450993-66.patch, failed testing.

dawehner’s picture

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
172.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,609 pass(es), 138 fail(s), and 45 exception(s). View
6.09 KB

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

Status: Needs review » Needs work

The last submitted patch, 70: rendered_cache_metadata-2450993-70.patch, failed testing.

Fabianx’s picture

#69.1:

There is two mitigating factors:

a) You can easily setup a renderInContext callback yourself.
b) In the future we could opt-out normal Responses from that check - because it is mainly there for Cacheable and Html and Ajax responses, which all have metadata.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
176.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,618 pass(es), 110 fail(s), and 45 exception(s). View
3.27 KB

>=29 less failures.

Status: Needs review » Needs work

The last submitted patch, 73: rendered_cache_metadata-2450993-72.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
227.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,777 pass(es), 103 fail(s), and 33 exception(s). View
53.77 KB

>=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.

dawehner’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 75: rendered_cache_metadata-2450993-75.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
233.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,967 pass(es), 96 fail(s), and 23 exception(s). View
5.91 KB

Again fewer exceptions.

Wim Leers’s picture

FileSize
233.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,006 pass(es), 35 fail(s), and 23 exception(s). View
1.65 KB

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!

The last submitted patch, 78: rendered_cache_metadata-2450993-78.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 79: rendered_cache_metadata-2450993-79.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
257.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,196 pass(es), 35 fail(s), and 8 exception(s). View
23.98 KB

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

Status: Needs review » Needs work

The last submitted patch, 82: rendered_cache_metadata-2450993-82.patch, failed testing.

Wim Leers’s picture

FileSize
258.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,208 pass(es), 8 fail(s), and 0 exception(s). View
745 bytes

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.

Wim Leers’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 84: rendered_cache_metadata-2450993-84.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
259.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch rendered_cache_metadata-2450993-87.patch. Unable to apply patch. See the log in the details link for more information. View
1.47 KB

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

Status: Needs review » Needs work

The last submitted patch, 87: rendered_cache_metadata-2450993-87.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
260.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,205 pass(es), 7 fail(s), and 0 exception(s). View
1.47 KB

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!

Status: Needs review » Needs work

The last submitted patch, 89: rendered_cache_metadata-2450993-89.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
263.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,228 pass(es). View
3.41 KB

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.

Wim Leers’s picture

FileSize
265.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,292 pass(es). View
9.31 KB

(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: Consider renaming 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.

Wim Leers’s picture

Related issues: +#2511472: Refactor all usages of drupal_render()/Renderer::render() that break #2450993
FileSize
91.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,969 pass(es), 136 fail(s), and 187 exception(s). View
168.69 KB

(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.

Status: Needs review » Needs work

The last submitted patch, 93: rendered_cache_metadata-2450993-93-logical_changes.patch, failed testing.

Wim Leers’s picture

IS updated. Next: CR.

Fabianx’s picture

Little thing:

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -495,52 +521,24 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+    static::$context = $context;
+    $result = $callable();
+    // @todo Convert to an assertion in https://www.drupal.org/node/2408013
...
+    // Restore the original render context.
+    static::$context = $current_context;

Should this not be wrapped in a try {} catch {} as well to restore the render context when exception bubbles up?

  • alexpott committed 68d4f0b on 8.0.x
    Issue #2511472 by Wim Leers, Fabianx, dawehner: Refactor all usages of...
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
91.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,301 pass(es). View

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

Wim Leers’s picture

#96: Why?

Fabianx’s picture

#99: Because when the Exception is caught the old render context is active, while in the ideal case it is caught so far outside that no rendering context is active anymore.

It made sense to me that instead of resetStack we just catch all Exceptions, reset our state to the old valid one and re-throw.

I think it could be follow-up though.

Wim Leers’s picture

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

Fabianx’s picture

I am giving a tentative RTBC + 1. This patch looks really sweet.

We do have a follow-up already to remove $is_root_call from render(), right? (and then also for doRender).

Wim Leers’s picture

Crell’s picture

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

Wim Leers’s picture

FileSize
91.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,238 pass(es). View
8.76 KB

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
Fabianx’s picture

Quick reminder on #100 :).

Crell’s picture

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.)

Return Logic Reason
string (or any other primitive) Disallowed I think we should allow this, frankly; and with this patch it probably becomes viable to do
Render array Grab metadata from context, merge, render, return rendered result array Catch additional metadata from odd corners; render immediately to reduce mem usage
HttpFoundation\Response (or subclass) Legal, return unmodified For when the user wants all the controls; we cannot disallow this
Object implementing AttachmentInterface ? (current code disallows, which I do not understand) ?
Object not implementing AttachmentInterface Legal, return unmodified Assume a view listener knows what to do

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?

Wim Leers’s picture

FileSize
11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,272 pass(es), 3 fail(s), and 1 exception(s). View
12.9 KB
103.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,315 pass(es). View

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 :)

Return Early rendering supported? Logic Reason
string (or any other primitive) N/A unsupported HEAD doesn't handle this. Symfony also doesn't consider this a valid return value. Symfony only allows Response objects. Drupal does too, and also allows render arrays. Supporting strings — which surely is possible, and more possible than before thanks to this patch — is very much out of scope for this issue. If you want to introduce that, please file a separate issue for that.
Render array Yes Grab metadata from context, merge, render, return rendered result array Catch additional metadata from odd corners; render immediately to reduce mem usage
HttpFoundation\Response or subclass, which does not care about bubbleable metadata No Legal, return unmodified For when the user wants all the controls; we cannot disallow this. Response objects not implementing neither AttachmentsInterface nor CacheableDependencyInterface don't care about attachments nor cacheability. Hence it's safe to ignore bubbleable metadata from early rendering that is lost.
HttpFoundation\Response or subclass, which does care about bubbleable metadata No If any early rendering happened, throw an exception Any controller returning a Response object wants to strictly control what is sent to the user. That's why they return a Response in the first place: tight, complete, final control over what is sent. We don't want to automatically modify these objects. If they want control, then they should also be responsible and render things correctly, and not rely on early rendering being fixed automagically.

(Found one bug in the process!)

Wim Leers’s picture

Issue tags: -Needs change record

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

The last submitted patch, 108: rendered_cache_metadata-2450993-108-test-only.patch, failed testing.

Fabianx’s picture

+1000 to #108, that is exactly what we want.

While theoretically we could indeed go to the lengths and merge the data for those responses also - as we have the means to add the data, we want to slowly transition to disallowing early rendering all together, which would be a way too big BC break at this point.

However for the newly introduced interfaces we can do so - so this is a step into a better future ... (*majestic music playing*)

Fabianx’s picture

Still RTBC from my side.

Crell’s picture

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.

Wim Leers’s picture

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:

Return Early rendering supported? Logic Reason
string (or any other primitive) N/A unsupported HEAD doesn't handle this. Symfony also doesn't consider this a valid return value. Symfony only allows Response objects. Drupal does too, and also allows render arrays. Supporting strings — which surely is possible, and more possible than before thanks to this patch — is very much out of scope for this issue. If you want to introduce that, please file a separate issue for that.
Render array Yes Grab metadata from context, merge, render, return rendered result array Catch additional metadata from odd corners; render immediately to reduce mem usage
HttpFoundation\Response (or subclass) or domain object, which does not care about bubbleable metadata No Legal, return unmodified For when the user wants all the controls; we cannot disallow this. Response/domain objects not implementing neither AttachmentsInterface nor CacheableDependencyInterface don't care about attachments nor cacheability. Hence it's safe to ignore bubbleable metadata from early rendering that is lost.
HttpFoundation\Response (or subclass) or domain object, which does care about bubbleable metadata No If any early rendering happened, throw an exception Any controller returning a Response or domain object wants to strictly control what is sent to the user. That's why they return a Response or domain object in the first place: tight, complete, final control over what is sent. We don't want to automatically modify these objects. If they want control, then they should also be responsible and render things correctly, and not rely on early rendering being fixed automagically.
Fabianx’s picture

#113: You miss a tiny detail: We only throw an Exception (for cacheable or attachments included Responses) when the renderContext is not-empty. Any such advanced controller can easily do:

  $render_context = new RenderContext();
  $this->renderer->executeInRenderContext($render_context, function() {
     // do something
  });

  // Deal with the render context, e.g. add it to a render array or to a Response or ...

or use #pre_render or use renderRoot() or use ...

So getting that Exception is not the end of the world, it just means you need to "catch" the render context yourself.

We also allow all responses or domain objects unchanged as we say:

- You returned a Response, you know what you are doing. In another follow-up we even (as discussed) want to opt-in only CacheableResponseInterface Responses to automatically gets Drupal's caching headers.

Then we also say, if you return a pure Response, you are also responsible for making it cacheable by setting the right HTTP headers.

But as Wim said: That is still a follow-up.

Fabianx’s picture

Assigned: Wim Leers » Unassigned
Status: Needs review » Reviewed & tested by the community

I am tentatively setting to RTBC. What Crell is asking for is clearly on the roadmap, but out-of-scope for this issue.

We can also re-visit allowing Cacheable or AttachmentsInterface responses in the future without an API / BC break, while the opposite is not possible.

Therefore and after very careful re-review, setting to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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 ;;

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
102.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,377 pass(es). View
6.23 KB

#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().

Crell’s picture

Status: Reviewed & tested by the community » Needs review

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.)

andypost’s picture

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

Wim Leers’s picture

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

Fabianx’s picture

Assigned: Unassigned » Crell
Issue tags: +Needs subsystem maintainer review, +blocker

Officially tagging and assigning to Crell for sign-off.

It looked like feedback had been addressed. We now have at least the alexpott round behind us :).

Also tagging blocker as this blocks #2351015: URL generation does not bubble cache contexts (that issue can base on this issue, but sooner or later it will be blocked on this one).

@Crell:

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.

Wim Leers’s picture

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!

Crell’s picture

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.

Crell’s picture

+++ 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.

larowlan’s picture

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.

Wim Leers’s picture

FileSize
112.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,362 pass(es). View
18.4 KB

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.
Wim Leers’s picture

#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.)

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php
@@ -135,17 +137,17 @@ protected function wrapControllerExecutionInRenderContext($controller, array $ar
+      elseif (is_object($response) && ($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)));
       }

The is_object is not needed.

An Exception is too much here, after carefully re-reading Crell's comment I agree that disallowing a case that was allowed before is a regression.

Because at least for AttachmentsInterface and CacheableResponseInterface we can safely merge in the data.

Even for CacheableDependencyInterface we could still think about it.

I think a trigger_error() with WARNING level would be the right thing to do:

- The developers gets to fix the problem. => good!
- On production no error is shown suddenly, because some obscure code path runs into early rendering.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
112.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,379 pass(es). View
1.35 KB

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.

dawehner’s picture

  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.

Wim Leers’s picture

  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.
Crell’s picture

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++

dawehner’s picture

dawehner’s picture

Assigned: Crell » Wim Leers

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

Wim Leers’s picture

FileSize
113.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,366 pass(es), 0 fail(s), and 1 exception(s). View
8.14 KB

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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!!

plach’s picture

Issue tags: +D8 Accelerate London

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 136: rendered_cache_metadata-2450993-136.patch, failed testing.

Wim Leers’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
Related issues: +#2334319: {% trans %} does not support render array and MarkupInterface valued placeholders
FileSize
114.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,364 pass(es), 0 fail(s), and 1 exception(s). View
1.27 KB

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 placeholders, then all this ugliness goes away :)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

It is great to find those failures now!

Fabianx’s picture

RTBC + 1, changes make sense.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 141: rendered_cache_metadata-2450993-141.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
114.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,372 pass(es), 0 fail(s), and 1 exception(s). View

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 145: rendered_cache_metadata-2450993-145.patch, failed testing.

Fabianx’s picture

#145: I think that is a different early rendering happening a few lines later :).

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
115.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,402 pass(es). View
709 bytes

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

plach’s picture

RTBC +1

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

Fabianx’s picture

#150.2: Because we want to add cacheable metadata to the column via the Renderer, in this case the username.

The rest should indeed be addressed.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
115.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,438 pass(es). View
1.2 KB
  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).
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed ebb21d2 on 8.0.x
    Issue #2450993 by Wim Leers, Fabianx, Crell, dawehner, effulgentsia:...

Status: Fixed » Closed (fixed)

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

YesCT’s picture

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?

Fabianx’s picture

#156: Thanks for catching that.

I checked and published the change record.

Do we have other lingering draft change records that have a 'fixed' or 'closed fix' issue?

Could someone run a query?

Maybe we should make it a policy that the committer should publish the CRs on fixing the issue?

Or maybe we should make someone else responsible?

yched’s picture

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 ?

Fabianx’s picture

#158: Indeed, thanks for catching that:

Fixed:

https://www.drupal.org/node/2513810/revisions/view/8716728/8717156

Also added an example how to setup a render context, even though it should only in seldom cases be needed.