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

https://gist.github.com/LionsAd/cbf84e5e70b05c1ca11e is a different approach to do the same, but keeps BC.

( Need to ignore the change for #cache though as that was needed for something else.)

Not really, my patch keeps BC so can go in whenever.

Just ran into this exception and found the @todo via #3496369: Multiple load path aliases without the preload cache and #3437499: Use placeholdering for more blocks - if you combine the two MRs from those issues and don't have bigpipe installed, you get the exception on various pages.

Converted Fabianx's 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 #3496369: Multiple load path aliases without the preload cache (where we enter and leave different rendering contexts).

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

I think the next step here is #3518179: Renderer::executeInRenderContext() needs to handle nested calls and suspended fibers 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.

#3516034: Add cacheable metadata to SelectInterface and entity QueryInterface objects 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 #3496369: Multiple load path aliases without the preload cache, everything explodes on cache misses.

The problem was (completely) missed in #3377570: Add PHP Fibers support to BigPipe, but this pre-existing issue solves it, alongside #3518179: Renderer::executeInRenderContext() needs to handle nested calls and suspended fibers 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 #3518179: Renderer::executeInRenderContext() needs to handle nested calls and suspended fibers 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.

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

Thanks!

Next up for anyone following along is #3518179: Renderer::executeInRenderContext() needs to handle nested calls and suspended fibers.

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: #3524626: Renderer::doRender() and ::doRenderRoot() contain some outdated information

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 #3524738: Fatal error when passing NULL to Renderer::render()

Next step here for unblocking async rendering is #3518179: Renderer::executeInRenderContext() needs to handle nested calls and suspended fibers.