[meta] Add a replacement for renderRoot() that returns a cacheable render array instead of a string

Is this really major? I think you marked it as major based on your discussions with Crell, where you concluded that if every bit of rendering that Renderer does returns such a thin render array, that we enable significant clean-up/simplification elsewhere?

If so, could you please explain that in the IS? Currently, this issue looks like it's a minor nice-to-have.

Aha :) Could you update the IS with that? Thanks!

This was postponed on #2407195: Move attachment processing to services and per-type response subclasses which is now fixed so marking as needs work. (hope that is correct & okay!)

Is this still possible to do now? It feels like too big of an API change but maybe I'm misunderstanding something…

I have to agree with @Cottser. Even you are supposed to not call renderRoot() directly, we still have 80 calls to it, so the chance is not unlikely that people use it.

So the "correct" replacement for most renderRoot() usages is probably renderPlain(), correct? Would it be correct to mark renderRoot() internal? The docs say:

Renders final HTML given a structured array tree.

Calls ::render() in such a way that placeholders are replaced.

Should therefore only be used in occasions where the final rendering is happening, just before sending a Response:

• system internals that are responsible for rendering the final HTML
• render arrays for non-HTML responses, such as feeds

(Cannot be executed within another render context.)

So the first case is clearly internal, but the second makes it sound like public API. And the second case is not a scenario where cacheability metadata matters anyway, is it? Or is it?

This change definitely could not happen in a patch release, but I can see the case for treating it as a security hardening and allowing the small BC break in a minor, with CR etc.

With value objects this would kinda make sense/work. But It seems very strange to me that a 'render' function would produce yet another array. Also a huge API change, IMO. When working with the system it just feeds like another barrier to use this API. #death_to_arrays_as_value_objects

If book export broken because of this?

  public function bookExport($type, NodeInterface $node) {
    $method = 'bookExport' . Container::camelize($type);

    // @todo Convert the custom export functionality to serializer.
    if (!method_exists($this->bookExport, $method)) {
      drupal_set_message(t('Unknown export format.'));
      throw new NotFoundHttpException();
    }

    $exported_book = $this->bookExport->{$method}($node);
    return new Response($this->renderer->renderRoot($exported_book));
  }

Or does a something come and grab the cacheable metadata later?

I suggest that we create a new method, remove all usages of the old method in core and finally deprecate it.

We discussed this issue with @alexpott, @Cottser, @lauriii, and @joelpittet at DrupalCon New Orleans. We discussed that the original proposal is probably too disruptive to change in a minor release, because it is a non-BC change to the return value of an API method that we can't really call internal based on its current usage. However, we agreed that it is a major issue. The path forward is:

1. Decide the new method name (it should not actually be renderRootReplacementNeedsAName()) and add it. Add documentation to renderRoot() about the new method as well as about the workaround above that is required for it to be cacheable.
2. Convert those core uses that should be renderPlain() already to renderPlain() (i.e. simple renderables with no assets or caching requirements).
3. Convert remaining core usages to the new method.
4. Deprecate renderRoot().