Render blocks later, so they can be placed individually in a region template

Patch attached. I was surprised how little code I needed to change to make this work :-)

I'll look into that test failure tomorrow. At first sight I don't understand what the batch test has to do with this patch, but I guess there's some kind of logic to it.

Thanks for testing, Joyce.

Meanwhile I did find a somewhat related issue: #2714509: Remove usages of #theme_wrappers

This patch should fix the failing test. The previous patch could accidentally mess up render elements when a render array returned by a controller has '#type' => 'page' (as \Drupal\system\Controller\BatchController::batchPage does).

Thanks Sutharsan, fair point. I used $block because the keys are usually (but not even necessarily) block identifiers. Indeed $key is more correct. New patch attached.

Oops, made a silly mistake when I did the fix in #7.

Correcting another silly mistake.

This should fix the test failures. (Again, BatchController::batchPage does things differently than normal pages. Yay for automated tests.)

Thanks Karl :-)

If you find any problems caused by this patch, please post them here. Likewise, if you do not encounter issues, that's valuable feedback too!

Thanks for the historical research, Casey. I think the advantage of having a region template still applies today, and indeed this proposal does not change that. In fact the patch doesn't change anything about the way the region wrapper is rendered, it's all about the rendering of the region content (blocks).

I also found that the idea to early-render elements that do not have a #theme property, goes way back to at least 2009. This is before the release of Drupal 7: Before the introduction of render arrays and the paradigm that we add flexibility by rendering them at the last possible moment.

Regarding the function nesting level and xDebug, I put a breakpoint in a block Twig template to test this. Drupal seems to add 9 function calls to the stack between the rendering of the region template and the rendering of the block template. The total depth of the call stack was 60 at that point, so that's still well within the default value of 256.

All this leads me to believe that it was never a deliberate choice to pass pre-rendered HTML to the region template, but rather the unintentional effect of evolving code, never updated to reflect the new approach. I guess it's finally time to fix that.

KarlShea has already reported that this change works without any issues and my organization is running this patch in multiple production environments. I don't want to set my own patch to RTBC, but if someone else would, we might attract the attention of core maintainers.

Thanks for the review, Alex.

I wasn't quite sure if it is expected and acceptable to both overwrite and wrap $page[$region] in a single statement, but I made that change now. The new patch has also been checked/rerolled against 9.3.x (not that it made any difference).

Good point about tests too. I'm setting this to needs review now, so the new patch can be tested. After that it can go back to needs work for the tests.

Oops, missed a comma.

New patch with tests, otherwise unchanged.

Yet another patch... Only changes a single comment that I copy-pasted and forgot to edit.

To be sure, I did some manual testing regarding cacheability. As far as I can see, everything looks good.

During these manual tests I was not logged in. The page_cache module was not installed because it does not support cache contexts for anonymous users, but dynamic_page_cache was enabled.

To test the cacheability of blocks, I took the following steps:

• Create a block plugin which prints a timestamp and the value of date_default_timezone_get().
• Implement getCacheContexts() to add the timezone context.
• Implement getCacheTags() to add a cache tag based on the timezone.
• Render the front page multiple times, sometimes overriding the timezone in settings.php.
• Verify that the block output is cached (timestamp remains the same when the page is reloaded).
• Verify that the block always shows the correct timezone string (cache varies by timezone context).
• Verify that the cache tag can be invalidated.

The above works as it should, both with and without the patch.

To test the cacheability of regions, I took the following steps:

• Implement hook_preprocess_region to do the following:
  • Add a template variable containing the timezone string and a timestamp.
  • Add the 'timezone' cache context to $variables['#cache']['contexts'].
  • Add a boolean template variable which is TRUE when the timezone is 'UTC'.
• Add a region--footer.html.twig template and adjust it to do the following:
  • Render the timezone/timestamp string.
  • Wrap the "powered by Drupal" block in a div if the timezone is UTC.
• Render the front page multiple times, sometimes overriding the timezone in settings.php.
• Verify that the region output is cached (timestamp remains the same when the page is reloaded).
• Verify that the region always shows the correct timezone string (cache varies by timezone context).
• Verify that the powered-by block is wrapped in a div only if the timezone is UTC.

The above works as it should when the patch is applied. This could not work without the patch, because you would not have a way to manipulate the region content and wrap a single block in a div.

As you can see, I had to add the 'timezone' cache context in the region preprocessor to make it work. However that is not a shortcoming of this change; as always it is the developer's responsibility to add cacheability metadata when the rendered output is made dependent on a variable such as the timezone.

@andypost I'm sorry, I don't see how those issues are related. Can you elaborate?

Please note that this issue doesn't change anything about what is cached and how it is cached. It merely delays the point at which region content (usually blocks) is rendered. The only new way this relates to the caching system, is the situation where the new possibilities are used to A) vary the output of the region or B) change the output using some value from the database. Case A would need a cache context added to the region, while case B would require a cache tag. That said, adding cache contexts and/or cache tags to a region is already supported in Drupal core. My manual testing in #30 just shows that the patch doesn't break it and that it plays nice with the new possibilities.

As @lendude asked me in Slack:

I’m just wondering about possible BC implications. If somebody has a hook_preprocess_region somewhere could this lead to a break since the passed $variables might be differently build up now? And if yes, should we care?

In templates, I consider the variable names to be an API (as developer you can assume they don't change) and I expect the output of those variables to be stable. If such a variable is documented to support drilling down (eg. {{ content.field_foo }} in a node template) I consider that part of the API too.

As you can see in template_preprocess_region, we don't change the names of the available variables, only what's inside $variables['content']. Also, the output of {{ content }} is identical after the change. The change does not effect any other variables that may have been added to the template. In other words: we are not breaking any API.

The whole point of this issue is that currently content is a pre-rendered string of HTML. I can only imagine the patch breaking someone's preprocessor if they rely on str_replace, regex etc. to manipulate the HTML. But that's not something Drupal core can support.

When I wrote "documented to support drilling down" in #34 I realized that we should probably document this in region(-*).html.twig files. The new patch is identical to #28, except that it changes this:

 * - content: The content for this region, typically blocks.

...to this, similar to what is found in node templates:

 * - content: The content for this region, typically blocks. Use {{ content }}
 *   to print them all, or print a subset such as {{ content.page_title }}.
 *   Use {{ content|without('page_title') }} to temporarily suppress the
 *   printing of a given child element.

...in the two dozen places where it occurs.

I never knew we also test file hashes of Classy templates. Fixed the test to match the documentation change introduced in #35.

Test failure in #37 was intentional.

Thank you Kristen for your review. The extra div turns out to be a logical effect of the change, but you are right this has not explicitly been mentioned before and I was not aware of this. This only happens in case the content of a region renders to an empty string.

To clarify (because I initially misread your comment): in the case of the Umami demo it is only a single div, the same 'tabs' region, which appears on 77 different URLs.

Analysis

The Umami demo site places one block in the tabs region: block ID 'umami_local_tasks', an instance of the LocalTasksBlock block plugin. For anonymous users there are no local tasks, so the block renders to an empty string. Note that this block is rendered with a #lazy_builder callback, as is the case with most blocks (except instances of MainContentBlockPluginInterface or TitleBlockPluginInterface, see \Drupal\block\BlockViewBuilder::viewMultiple).

Current Drupal core

Currently, the region content is flattened to HTML rather early (the lazy builder isn't as lazy as it could be!). The region.html.twig template then skips rendering the div.region wrapper if that string of HTML is empty ({% if content %}).

After the patch

The essence of this patch is that the region content (ie. blocks) is not rendered before the region template is being processed. Therefore, in the new situation the {% if content %} check in region.html.twig returns TRUE if there is a block to be rendered, regardless of whether that block actually returns HTML or an empty string. This causes a div.region wrapper to be rendered while it used to be skipped.

Possible solution

If this empty div is a problem, it is possible to avoid by changing {% if content %} to {% if content|render|striptags|trim is not empty %}. The Umami theme itself uses this pattern a lot in its page.html.twig, and Olivero does it too. However this method has downsides, see #953034: [meta] Themes improperly check renderable arrays when determining visibility. We could add this to the core region.html.twig templates by default, but I don't think we should be introducing sub-optimal code all over the place. That said, it might make sense to do it in the stable/stable9 themes, since unchanging output is what they promise.

Discussed with @lauriii on Slack. His thoughts:

I don’t think we have to solve the empty region element problem here, it’s a pre-existing problem
what I am little concerned about is changing the behavior for existing sites
what could work is instead of replacing content with the render array, create new variable for the render array
then replace the usage of the content variable everywhere except stable and stable9
[...]
I think we could and should mark it [the content variable] as deprecated

Back to needs work.

Re. #45: Thanks for bringing up performance. Yes, you understand the suggestion correctly. There would be two variables in a region template: the old pre-rendered $content and the new render array variable (let's call it $elements for now). Indeed this approach comes with a performance hit because of double rendering. We should test how noticeable this is.

Actually, the more I think about the $content + $elements approach, the more downsides I see. It feels like we're not solving the problem, but we're trading it for another.

The thing is: at some point in the future we would have to get rid of the deprecated $content variable in favor of $elements. So what happens then? Do we simply remove $content, breaking all templates that still use it? Do we rename $elements to $content, breaking all templates that meanwhile switched to $elements? It seems to me that more sites will be affected at that point, and a removed template variable will probably cause more problems than an extra empty div would.

Of course one could argue that breaking things is more acceptable at the release of a new major version. If that is the consensus, let's just get this ready for Drupal 10, which should be less than 6 months away.

All right, let's make a decision before it's too late. The current situation:

• We have a working patch that has proven itself in production environments, with test coverage.
• In a very specific situation, the patch adds an empty div to the HTML markup.
• Our core change policies say about changing markup:
  • "The following types of changes are allowed for minor releases [...] CSS, markup, or template changes (except for stable base themes)"
  • Changing the markup is allowed in alphas of major versions, or in betas "if the impact outweighs the disruption".
• Avoiding the empty-div situation in the stable base themes is hard, because of the difficulties in determining visibility (#953034: [meta] Themes improperly check renderable arrays when determining visibility).
• Other proposed workarounds are suboptimal for reasons of performance and DX further down the road.
• If we wait too long figuring out how to fix this in 9.x, we may miss the opportunity to ship this with 10.x-alpha.

My conclusion: Let's target 10.x, preferably alpha, and get this in core.

Re #49: Thanks for your input @effulgentsia.

I agree, it would have been nicer if we didn't have to change the structure of the render array. The wrapping region_content was introduced in patch #7 to fix the test failure in #2. The wrapper is not even needed for "normal" pages, only for a very specific case (quoting comment #7):

when a render array returned by a controller has '#type' => 'page' (as \Drupal\system\Controller\BatchController::batchPage does)

So keeping the render array structure unchanged, as patch #2 did, is definitely an option if we find another way to solve the '#type' => 'page' bug.

Trying out an alternative patch in an attempt to address #49. Tests pass locally so I'm curious if testbot agrees.

[edit] Please ignore this patch. It was a renewed attempt at the approach previously used in #2. Although the tests came back green, things actually break. [/edit]

Thanks for looking into it, Alex. I agree, especially for cases such as uikit, mentioned in #49, a change record is needed. (Not sure if the issue status should be "needs work" then.)

Anyway, I had a closer look at the uikit code to understand why it needs access to $variables['elements']. I found that uikit_preprocess_region aims to place different blocks in three distinct divs within the navbar region template. So that is exactly what this issue enables you to do (albeit without preprocessors and in a way that is consistent with other templates). With this patch applied, the uikit preprocessor could have less code and be more efficient, because currently it needs to load and render blocks that have already been loaded and rendered.

So while I understand that this change may require maintainers to adapt their code to the new render array, it may actually be a change for the better.

Here's a new patch, addressing the BC concerns raised in #49. I managed to "find another way to solve the '#type' => 'page' bug" as suggested in #50.

First let me explain where this '#type' => 'page' bug comes from. Most pages are built using the block system. This happens in \Drupal\block\Plugin\DisplayVariant\BlockPageVariant::build and it results in a nicely structured $page array, where every region element is an array of block render arrays, keyed by block ID ($page[region_name][block_id] = [block_render_array]). However, in some rare cases, controllers take full control by returning a render array that already has '#type' => 'page'. One example is \Drupal\system\Controller\BatchController::batchPage. Batch controller does not follow the $page[region_name][block_id] structure, but instead places a render array with '#type' => 'page_title' directly in $page['header']. As a result, we cannot safely set $page['header']['#theme'] = 'region' because it would break the page title render array.

This new patch detects the edge cases (like the batch page) by checking if the $page[region_name] element is a render array with a #theme or #type property. If so, it is wrapped in an array with key "region_content". In all other cases the structure is not altered: blocks remain the direct children of $variables['elements'].

So, does this mean that with this new patch $variables['elements'] is 100% identical? No. After all, the point of this patch is to render the blocks as late as possible. A block with a lazy builder will not yet be built when it passes through the region preprocessor (see image).

I can't reproduce the test failure locally. Rerunning the test.

[update] It seems that the test fail in #56 is unrelated: #3210432: [random test failure] LayoutBuilderQuickEditTest::testQuickEditIgnoresDuplicateFields() Also, it passed on the second try.