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

Just ran into this exception and found the @todo via 📌 Try to replace path alias preloading with lazy generation Active and 📌 Create placeholders for more things Active - if you combine the two MRs from those issues and don't have bigpipe installed, you get the exception on various pages.

Converted the patch to an MR and dealt with conflicts/cs issues.

Lots of failures. Might be worth moving Wim's patch to a second MR and see where we land with that - looks like bc could be added quite straightforwardly to Wim's approach.

OK after fixing the unit tests in Wim's approach I realised the bug in Fabian's approach, so we've now got green MRs for both.

However, this is an important improvement from Fabian's:

   $child_element = &$elements[$key];
        if (isset($child_element['#cache']['keys'])) {
          $new_context = new RenderContext();
          $elements['#children'] .= $this->executeInRenderContext($new_context, function () use (&$child_element, $new_context) {
            return $this->doRender($child_element, $new_context);
          });
          // @todo This should not be necessary.
          if (!$new_context->isEmpty()) {
            $frame = $context->pop()->merge($new_context->pop());
            $context->push($frame);
          }
        }
        else {
          $elements['#children'] .= $this->doRender($elements[$key], $context);
        }
      }

e.g. when rendering children, we no longer rely on the global render context state but or even a class property, but pass the context into the method.

I think that this will either solve, or approach solving, the issues I'm running into on 📌 Try to replace path alias preloading with lazy generation Active (where we enter and leave different rendering contexts).

catch → changed the visibility of the branch 2511330-alternate-approach to hidden.

OK #28 was premature but now it's green.

I think the next step here is 📌 Pass RenderContext around in the Renderer Active but that will require interface changes to add the parameter to a couple of methods and maybe more things, so one step at a time.

✨ Improve query and cache API so that render() doesn't have to be called to add query cache metadata Active is closely related too.

Moving this to a bug report, it wasn't a bug as such when it was originally added to core, more of a 'limitation', but now we're using fibers for placeholder rendering, if code actually fiber suspends a decent amount, which is done in 📌 Try to replace path alias preloading with lazy generation Active , everything explodes on cache misses.

The problem was (completely) missed in 📌 Add PHP Fibers support to BigPipe RTBC , but this pre-existing issue solves it, alongside 📌 Pass RenderContext around in the Renderer Active which I just opened this week as a follow-up to this issue. The combination of the two allows that path alias issue to pass nearly all tests, but anything that uses fiber suspend in multiple placeholders will trigger the same exception at the moment.

There's very good existing test coverage of this, as shown by the several commits on both MRs here tracking down and fixing various test failures. I don't think we need explicit test coverage of the fiber suspend issue yet, it could possibly be added in 📌 Pass RenderContext around in the Renderer Active but not here because as soon as you fix this issue you run into that one, keeping separate for ease of review since neither are simple and they don't conflict.

As I get it just need deprecation test

I don't think this needs a deprecation test - the deprecation path is only the trigger_error() and one line method call, no actual bc layer to test. Moving back to needs review.

Some comments on MR 10795.

Thanks for looking, think I resolved all of those.

My mistake about the use Drupal\Core\Render\RenderContext; suggestion. It's in the same namespace, so it was unnecessary and flagged by PHPCS.

Also looks like phpstan baseline needs regenerating.

Added a couple comments on the typehints too.

Argh I spotted the RenderContext thing after commit and before push, but failed to commit the change.

Pushed a commit for that and the other couple of comments. I think the phpstan complaint was real. Should be back to green.

lgtm

Left some questions on the MR

Putting back to RTBC because all the changes from my last review where I changed the status were either trivial or reverted.
I manually confirmed we still have the bubbling check from the other point.

Given the changes here would have been fine for catch to self RTBC I think it is OK for me to commit this still.

larowlan → closed merge request !10795

Committed to 11.x - thanks!
Published the change notice.

Thanks!

Next up for anyone following along is 📌 Pass RenderContext around in the Renderer Active .

I noticed the docs in doRender() were not adjusted:

     // Set the bubbleable rendering metadata that has configurable defaults, if:
     // - this is the root call, to ensure that the final render array definitely
     //   has these configurable defaults, even when no subtree is render cached.
     // - this is a render cacheable subtree, to ensure that the cached data has
     //   the configurable defaults (which may affect the ID and invalidation).
-    if ($is_root_call || isset($elements['#cache']['keys'])) {
+    if (isset($elements['#cache']['keys'])) {

They still mention that doRender() can be the root call, and from the MR it seems that doRender() should no longer be concerned with any of that. Tackle this in a tiny follow-up?

Yes please to tiny follow up 🙏

Working on it here: 🐛 Renderer::doRender() contains some outdated information Active

Thanks

The array type added here cases a fatal error for us, caused by template_preprocess_node():

$variables['date'] = \Drupal::service('renderer')->render($variables['elements']['created']);

created is null, and while it shouldn't call it then, this seems like a BC break that shouldn't be done like this or then the render () method should guard against that?

Follow-up to remove that?

Created 🐛 Fatal error when passing NULL to Renderer::render() Active

Next step here for unblocking async rendering is 📌 Pass RenderContext around in the Renderer Active .