Factor render->fragment code out to a service

This services converts a render array to a html fragment, so what about simply go with RenderHtmlFragmentConverter?

Nice a patch from crell and just 1000 problems.

+++ b/core/lib/Drupal/Core/Page/RenderHtmlFragmentConverter.php
@@ -0,0 +1,83 @@
+
+class RenderHtmlFragmentConverter {
+

Render is such a generic term, that I did not know what this class is all about, maybe go with RenderToHtmlFragmentConverter?

Okay, this seem to make sense, though you still need to fix the actual patch.

The patch is indeed not complex on the architectual level, just extract logic into a proper place.
Though at least it should be possible to take the time and actually document it a little bit. Maybe someone here at the sprint will
find the time to work on it.

1. +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
   @@ -74,9 +74,11 @@ class ExceptionController extends HtmlControllerBase implements ContainerAwareIn
   +   * @param \Drupal\Core\Page\RenderHtmlRenderer
   +   *   The render array converter.
   
   +++ b/core/lib/Drupal/Core/Controller/HtmlControllerBase.php
   @@ -28,11 +27,11 @@ class HtmlControllerBase {
   +   * @var RenderHtmlRendererInterface
   
   @@ -41,10 +40,12 @@ class HtmlControllerBase {
   +   * @param \Drupal\Core\Page\RenderHtmlRenderer
   +   *   The render array converter.
   
   +++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
   @@ -7,6 +7,8 @@
   +use Drupal\Core\Page\RenderHtmlRenderer;
   +use Drupal\Core\Page\RenderHtmlRendererInterface;
   
   @@ -31,9 +33,11 @@ class HtmlPageController extends HtmlControllerBase {
   +   * @param \Drupal\Core\Page\RenderHtmlRenderer
   +   *   The render array converter.
   

   Let's just always document the interface and not use the class which is not needed.

2. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,76 @@
   +class RenderHtmlRenderer implements RenderHtmlRendererInterface {
   +
   

   At least one line could be really done here.

Crell asked me to take a look at what's being done in RenderHtmlRenderer::render() to see if it's complete, so here's some feedback about that, but since this patch just moves code from the existing base class, these are all pre-existing conditions in HEAD, so addressing them can be done in followups, IMO.

1. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,76 @@
   +      $render_array = array(
   +        'main' => array(
   +          '#markup' => $render_array,
   +        ),
   +      );
   

   I don't think we need the 'main' wrapper any more. I think that's a legacy from when this was being returned into a $page array, and we wanted to put the main content into a region named 'main', but that's no longer the case, as we're now rendering this into a string directly and letting the caller decide where to put that string.

2. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,76 @@
   +    $cache = !empty($render_array['#cache']['tags']) ? ['tags' => $render_array['#cache']['tags']] : [];
   +    $fragment = new HtmlFragment($content, $cache);
   

   If we're dealing with cache tags, we probably also need to deal with #post_render_cache. There's separate work related to that in #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render and possibly other issues. Wim Leers is our current expert on all matters related to #post_render_cache :)

3. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,76 @@
   +    $attached = drupal_render_collect_attached($render_array, TRUE);
   

   This is running after we've already called drupal_render() earlier in the function, so we shouldn't need to collect. Rather, the info should already be fully collected and merged in $elements['#attached'].

4. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,76 @@
   +    // Add feed links from the page content.
   +    foreach ($attached['drupal_add_feed'] as $feed) {
   +      $feed_link = new FeedLinkElement($feed[1], $this->urlGenerator->generateFromPath($feed[0]));
   +      $fragment->addLinkElement($feed_link);
   +    }
   

   In addition to drupal_add_feed(), we also have drupal_add_http_header(), drupal_add_html_head(), and drupal_add_html_head_link(). They probably all need some refactoring.

:(

1. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,86 @@
   +  /**
   +   * @var \Drupal\Core\Routing\UrlGeneratorInterface
   +   */
   ...
   +   * @param UrlGeneratorInterface $generator
   +   *   The URL Generator service.
   +   */
   
   +++ b/core/lib/Drupal/Core/Page/RenderHtmlRendererInterface.php
   @@ -0,0 +1,28 @@
   +   * @return HtmlFragment
   +   *   The equivalent HtmlFragment object.
   

   CS talks about using full namespace and even one liner docs for silly service properties. I know you know that ...

2. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,86 @@
   +      $render_array = ['#markup' => $render_array];
   

   +1 for using short array syntax

3. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRendererInterface.php
   @@ -0,0 +1,28 @@
   +  public function render($render_array);
   +}
   diff --git a/core/tests/Drupal/Tests/Core/Controller/ExceptionControllerTest.php b/core/tests/Drupal/Tests/Core/Controller/ExceptionControllerTest.php
   

   CS says empty line here

Seem fine (even if you ignored the one line description)

How does this relate to #2327277: [Meta] Page rendering meta discussion? Will it help solve that?

1. +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
   @@ -82,12 +82,11 @@ class ExceptionController extends HtmlControllerBase implements ContainerAwareIn
   +   * @param \Drupal\Core\Page\RenderHtmlRenderer
   +   *   The render array converter.   */
   +  public function __construct(ContentNegotiation $negotiation, TitleResolverInterface $title_resolver, HtmlPageRendererInterface $renderer, HtmlFragmentRendererInterface $fragment_renderer, TranslationInterface $string_translation, LoggerChannelFactoryInterface $logger_factory, RenderHtmlRendererInterface $render_converter) {
   

   Also can you fix this */

2. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,86 @@
   +  /**
   +   * @var \Drupal\Core\Routing\UrlGeneratorInterface
   +   */
   +  protected $urlGenerator;
   

   I think he meant here?

1. +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
   @@ -82,12 +82,11 @@ class ExceptionController extends HtmlControllerBase implements ContainerAwareIn
   +   *   The render array converter.   */
   

   Stupid docs typo :)

2. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,86 @@
   + * Default render array renderer.
   

   This makes me think this is an alternative to drupal_render(). I think this would benefit from a better description.

3. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,86 @@
   +  /**
   +   * @var \Drupal\Core\Routing\UrlGeneratorInterface
   +   */
   

   Missing description.

4. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,86 @@
   +   *   The URL Generator service.
   

   s/Generator/generator/

5. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,86 @@
   +  public function render($render_array) {
   

   Why not typehint?

6. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,86 @@
   +      $render_array = ['#markup' => $render_array];
   

   :)

7. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,86 @@
   +    // A title defined in the return always wins.
   

   What's "the return"?

8. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRendererInterface.php
   @@ -0,0 +1,28 @@
   +   * @param array|string $render_array
   +   *   The render array to convert. A string may also be passed, in which case
   +   *   it will be treated as a a render array with just a #markup property.
   

   Why also accept a string? This feels like a concession to accommodate the existing codebase. It also conflicts with the intent and documentation of the interface.

9. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRendererInterface.php
   @@ -0,0 +1,28 @@
   +   * @return \Drupal\Core\Page\HtmlFragment
   +   *   The equivalent HtmlFragment object.
   

   Shouldn't this be typehinted on the interface?

   Also, there should be a newline before @return.

10. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRendererInterface.php
    @@ -0,0 +1,28 @@
    +  public function render($render_array);
    +}
    

    Missing a newline.

1. +++ b/core/lib/Drupal/Core/Controller/HtmlControllerBase.php
   @@ -28,11 +27,11 @@ class HtmlControllerBase {
   +   * @var RenderHtmlRendererInterface
       */
   ...
   +  protected $renderConverter;
   

   :( Please let's use absolute full namespaces for now.

2. +++ b/core/lib/Drupal/Core/Controller/HtmlControllerBase.php
   @@ -41,10 +40,12 @@ class HtmlControllerBase {
   +   * @param \Drupal\Core\Page\RenderHtmlRenderer
   +   *   The render array converter.
       */
   ...
   +  public function __construct(TitleResolverInterface $title_resolver, RenderHtmlRendererInterface $render_converter) {
   
   +++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
   @@ -31,9 +33,11 @@ class HtmlPageController extends HtmlControllerBase {
   +   * @param \Drupal\Core\Page\RenderHtmlRenderer
   +   *   The render array converter.
   

   Let's also document interfaces.

3. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,87 @@
   +    $fragment = new HtmlFragment($content, $cache);
   +
   +    if (isset($render_array['#title'])) {
   +      $fragment->setTitle($render_array['#title'], Title::FILTER_XSS_ADMIN);
   +    }
   +
   +    $attached = isset($render_array['#attached']) ? $render_array['#attached'] : [];
   +    $attached += [
   +      'drupal_add_feed' => [],
   +      'drupal_add_html_head' => [],
   +      'drupal_add_html_head_link' => [],
   +    ];
   +
   +    // Add feed links from the page content.
   +    foreach ($attached['drupal_add_feed'] as $feed) {
   +      $fragment->addLinkElement(new FeedLinkElement($feed[1], $this->urlGenerator->generateFromPath($feed[0])));
   +    }
   +
   +    // Add generic links from the page content.
   +    foreach ($attached['drupal_add_html_head_link'] as $link) {
   +      $fragment->addLinkElement(new LinkElement($this->urlGenerator->generateFromPath($link[0]['href']), $link[0]['rel']));
   +    }
   

   This could be certainly used also to convert blocks into fragments, at least indirectly.

Sorry for being annoying.

Currently, a string is a nominally legal render array. Controllers can return "render array or string".

Is it? drupal_render() doesn't support strings at all (you'll get a fatal if you try).
The fact that controllers may return strings doesn't matter — that's unrelated to "converting a render array to a HTML Fragment", which is what this new service is all about (or at least, that's what its docs say).

If this (supporting strings *and* render arrays) is what you want, then the interface is named incorrectly. But I think what you really want is to have this new interface to be solely responsible for converting a render array (and hence also type hint on array) to a HTMLFragment, and to then have the following code in HtmlControllerBase:

if (is_string($page_content)) {
  $fragment = new HtmlFragment($page_content);
}
else {
  $fragment = $this->renderConverter->render($page_content);
}

I'm sorry, but I find the current patch very confusing. I think it should be much more explicit. I think my proposal would make it much more explicit: make this new interface (and service) have a very specific purpose, which will make the interface docs actually truthful (rather than misleading lies) and have a very clear conceptual purpose: an interface and service to convert a render array to a HtmlFragment, which would indeed be a very valuable component in the process of rendering blocks as HtmlFragments.
You could then also make the interface and service name much clearer: RenderHtmlRenderer is… rather vague. I think RenderArrayFragmentConverter or even RenderArrayToFragment or Fragmentizer (:P) would be clearer.

1. +++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
   @@ -31,9 +33,11 @@ class HtmlPageController extends HtmlControllerBase {
   +   * @param \Drupal\Core\Page\RenderHtmlRenderer
   

   This should be the interface.

2. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,87 @@
   + * Default render array renderer.
   

   This is still a very confusing, extremely abstract description.

3. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,87 @@
   +   * Url generator service.
   

   s/Url/URL/

4. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,87 @@
   +  }
   +}
   

   Missing newline.

5. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRendererInterface.php
   @@ -0,0 +1,30 @@
   + * Interface for HTML Render Array Renderers.
   ...
   + * An HTML Fragment Renderer is responsible for translating a Drupal render
   

   Inappropriate capitalizations?

The basic idea here of extracting code into a new object sounds reasonable enough, but this patch is *really* confusing.

+/**
+ * Interface for HTML Render Array Renderers.
+ *
+ * An HTML Fragment Renderer is responsible for translating a Drupal render
+ * array into an HtmlFragmentInterface object.
+ */
+interface RenderHtmlRendererInterface {
+
+  /**
+   * Converts a render array into a corresponding HtmlFragment object.

The language here shuts off my brain.

Is it rendering or converting? Shouldnt this just be RenderArrayToHtmlFragmentConverter? Or if its creating new HtmlFragment objects...HtmlFragmentFactory::createFromRenderArray?

But as long as you want the burden of dealing with strings to lie in a "render array to HtmlFragment" converter service, you will end up with confusing code.

Again: strings are not valid render arrays.

So this in #29 is wrong: Currently, a string is a nominally legal render array.. But this part is true: Controllers can return "render array or string". — however, note that "controller return values" is not the same as "render arrays".
If you really want to go this way, then you'll want ControllerReturnValueRendererInterface. Which wouldn't make anybody happy either, I suspect.

Is there a good reason any controller must still be able to return either a HTML string or a render array? If everything returned render arrays, this patch could be made a lot clearer.

Finally, I don't see how #36 answers #35's question of why we can't have a HtmlFragment::createFromRenderArray() static method? If you eventually want to move away from render arrays, then a static method that does the conversion makes a lot of sense to me.

I do agree, on a academical level, it is not strictly a render array but you know what I express if I talk of academia.

Again: strings are not valid render arrays.

Well, we do support strings ATM. The issue is to extract that logic into a generic service, that is all.
I hope you don't want to just block crell's important work here, which he started a long long time ago.
Note, the talk you complain about is a 3 liner, nothing complex at all. You can easily remove that in a follow up if you wish.

Finally, I don't see how #36 answers #35's question of why we can't have a HtmlFragment::createFromRenderArray() static method? If you eventually want to move away from render arrays, then a static method that does the conversion makes a lot of sense to me.

Well a html fragment is just a stupid object, it should not know how to create itself, as this is really not a straightforward operation.

I'm not trying to block this at all, I'm just very concerned that this will make the way Drupal 8 renders pages will become even harder to understand (for an explanation as to why it's already hard: #2327277: [Meta] Page rendering meta discussion).

This patch introduces an abstraction but then uses it in only one place. That's strange.

Furthermore, it doesn't even remove HtmlControllerBase::createHtmlFragment() — it just removes most of that function's body.

And then finally, all documentation indicates that it's about converting a render array to a HtmlFragment, but then it turns out it also deals with strings.

Why is this okay:

  protected function createHtmlFragment($page_content, Request $request) {
    // Allow controllers to return a HtmlFragment or a Response object directly.
    if ($page_content instanceof HtmlFragment || $page_content instanceof Response) {
      return $page_content;
    }

    $fragment = $this->renderConverter->render($page_content);

    if (!$fragment->getTitle() && $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)) {
      $fragment->setTitle($this->titleResolver->getTitle($request, $route), Title::PASS_THROUGH);
    }

    return $fragment;
  }

and not this:

  protected function createHtmlFragment($page_content, Request $request) {
    // Allow controllers to return a HtmlFragment or a Response object directly.
    if ($page_content instanceof HtmlFragment || $page_content instanceof Response) {
      return $page_content;
    }

    if (is_string($page_content)) {
      $fragment = new HtmlFragment($page_content);
    }
    else {
      $fragment = $this->renderConverter->render($page_content);
    }

    if (!$fragment->getTitle() && $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)) {
      $fragment->setTitle($this->titleResolver->getTitle($request, $route), Title::PASS_THROUGH);
    }

    return $fragment;
  }

? There already are two instanceof checks on $page_content. Why are those okay but this third (is_string()) isn't? I proposed exactly this 5 comments ago. A future patch can still remove this if it makes sense then, but for now, that's the only way I see this patch making sense.

The problem of this new abstraction/service only being used in a single place would still be true, but at least its responsibility would be *clear*. I'd be fine with that.

In other words: this patch is simply not good and clear enough yet.

+++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
@@ -0,0 +1,87 @@
+
+    // @todo Also transfer the contents of "drupal_add_html_head" once
+    // https://www.drupal.org/node/2296951 lands.
+
+    // @todo Transfer CSS and JS over to the fragment once those are supported
+    // on the fragment object.
+

See #2334029: Add js/css/libraries/head links to HTML fragments and pass it along to the page.

Better, thank you. Getting closer.

That still leaves:

1. the missing @todo pointed out by dawehner in #40.
2. the naming concerns raised in #34 and #35
3. the architectural concerns raised in #34 and #39 (This patch introduces an abstraction but then uses it in only one place. That's strange.)

Point 1 is a trivial fix, alternatives have been suggested for point 2, and I suspect you have a good explanation/reasoning for point 3 but you haven't posted it yet AFAICT?

(Oh and I'm very, very sorry about the terribly slow review this time — I was deep in another set of patches :()

@Wim Leers @Crell
Can't we just open an issue to figure out which _content controller still return a string? This feels like a really nitpicky discussion though, tbh.

This patch introduces an abstraction but then uses it in only one place. That's strange.

We certainly will have more places where we could use it: blocks to fragment conversion.

Given that @Crell do you want to give this now a new reroll after the exception modernization?

1. Yep :)
2. Missed that, sorry.
3. I should've worded that better. That something like a third of the services in core you mention is not really truthful though: that third is about services where it's likely that sites may want to override a service, for scalability or site-specific optimizations or some other reason. I don't see how anybody would ever want to override RenderHtmlRenderer.

I won't hold this up further because I want all of us to make progress, but I'm still not really convinced. As long as this is an intermediate step towards something better, I'm fine with it.

In that light, here's another review:

1. +++ b/core/core.services.yml
   @@ -649,6 +649,9 @@ services:
   +  render_array_renderer:
   +    class: Drupal\Core\Page\RenderHtmlRenderer
   

   Let's make the service name consistent with the class name.

2. +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
   @@ -80,14 +80,14 @@ class ExceptionController extends HtmlControllerBase implements ContainerAwareIn
   +   * @param \Drupal\Core\Page\RenderHtmlRendererInterface
   +   *   The render array converter.
   
   +++ b/core/lib/Drupal/Core/Controller/HtmlControllerBase.php
   @@ -23,28 +22,28 @@ class HtmlControllerBase {
   +   * The render array converter.
   ...
   +  protected $renderConverter;
   ...
   +   * @param \Drupal\Core\Page\RenderHtmlRendererInterface $render_converter
   +   *   The render array converter.
   ...
   +    $this->renderConverter = $render_converter;
   
   +++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
   @@ -29,11 +31,11 @@ class HtmlPageController extends HtmlControllerBase {
   +   *   The render array converter.
   

   … I agree that "converter" is a better name. But that's for that follow-up issue. Let's be consistent.

3. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,85 @@
   + * Default render array renderer.
   

   This description definitely needs to be better though.

4. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,85 @@
   +   * Url generator service.
   ...
   +   *   The URL generator service.
   

   Two types of capitalization.

5. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -0,0 +1,85 @@
   +  public function render(array $render_array) {
   +
   +    $content = $this->drupalRender($render_array);
   

   Extraneous blank line.

6. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRendererInterface.php
   @@ -0,0 +1,29 @@
   + * Interface for HTML Render Array Renderers.
   ...
   + * An HTML Fragment Renderer is responsible for translating a Drupal render
   

   Bizarre capitalization.

7. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRendererInterface.php
   @@ -0,0 +1,29 @@
   +   * Converts a render array into a corresponding HtmlFragment object.
   

   HtmlFragmentInterface

Thank you for your review, wim!

I should've worded that better. That something like a third of the services in core you mention is not really truthful though: that third is about services where it's likely that sites may want to override a service, for scalability or site-specific optimizations or some other reason. I don't see how anybody would ever want to override RenderHtmlRenderer.

well, it is not only about the overrideability, this is also about dependencies etc. For example that service depends on drupal_render() somehow.
Services also ensures that your code can be somehow reused.j

I won't hold this up further because I want all of us to make progress, but I'm still not really convinced. As long as this is an intermediate step towards something better, I'm fine with it.

Oh absolute, just to make this clear, this issue is just one of many steps, with every step an incremental small improvement, with though obviously a lot of still opened problems.

Crell is okay with me taking over that in order to ensure that something moves forward until Drupalcon.

… I agree that "converter" is a better name. But that's for that follow-up issue. Let's be consistent.

Sure, no big deal.

This description definitely needs to be better though.

Oh yeah, what explaining which parts of the render this one takes into account? Better suggestions are highly recommended.

After talking with Crell on IRC we wanted to ensure that we have a immutable version (the interface), see #2339475: Introduce a mutable HTML fragment interface for
adding a mutable interface as well.

Crell is okay with me taking over that in order to ensure that something moves forward until Drupalcon.

Alright, let's get a very fast reroll/review cycle going :) Let's do this!

Just a small few remaining bits… after this it's RTBC.

1. +++ b/core/lib/Drupal/Core/Controller/HtmlControllerBase.php
   @@ -27,23 +27,23 @@ class HtmlControllerBase {
   +   * The render array to HTML fragment converter.
   

   Still "converter".

2. +++ b/core/lib/Drupal/Core/Controller/HtmlControllerBase.php
   @@ -27,23 +27,23 @@ class HtmlControllerBase {
   +   *   The render html renderer.
   
   +++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
   @@ -31,11 +29,11 @@ class HtmlPageController extends HtmlControllerBase {
   +   *   The render html renderer.
   

   s/html/HTML/

3. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
   @@ -11,12 +11,15 @@
    /**
   - * Default render array renderer.
   + * Provides an implementation for an render array to HTML fragment renderer.
   + *
   + * This renderer takes into account the cache information, the attached assets
   + * as well as the title and HTML HEAD elements.
     */
   

   Thank you, this is *much* better!

4. +++ b/core/lib/Drupal/Core/Page/RenderHtmlRendererInterface.php
   @@ -21,7 +21,7 @@
   -   * @return \Drupal\Core\Page\HtmlFragmentInterface
   +   * @return \Drupal\Core\Page\HtmlFragment
       *   The equivalent HtmlFragment object.
   

   This change is wrong; it's the comment that should've changed from HtmlFragment object to HtmlFragmentInterface object.

   (I really wish we didn't have to write a description when in cases like these; just the @return TYPE is more than enough!)

Still "converter".

Oh I thought I got all.

This change is wrong; it's the comment that should've changed from HtmlFragment object to HtmlFragmentInterface object.

(I really wish we didn't have to write a description when in cases like these; just the @return TYPE is more than enough!)

Yeah this change was 100% intended.

@Wim Leers
I tend to prefer using phpstorm for actual in depth reviews as you spot things like that.

Aha! That connection wasn't clear to me. That leaves just points 1–2, i.e. only two of the tiniest nitpicks. I think it's okay for me to fix those and still RTBC this patch.

Also linked to #2339475: Introduce a mutable HTML fragment interface regarding point 4.

EDIT: cross-posted with dawehner… but I'm still posting this patch, because it includes that link for #49.4/#50 to ensure we fix it later on. Hope that's okay.

See you in #2334029: Add js/css/libraries/head links to HTML fragments and pass it along to the page.

My spider sense tells me that catch will probably want to look at this one before it goes in. If I'm wrong about that, feel free to unassign.

+++ b/core/lib/Drupal/Core/Controller/HtmlControllerBase.php
@@ -65,47 +64,16 @@ protected function createHtmlFragment($page_content, Request $request) {
+    $fragment = $this->renderHtmlRenderer->render($page_content);
...
+    if (!$fragment->getTitle() && $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)) {
       $fragment->setTitle($this->titleResolver->getTitle($request, $route), Title::PASS_THROUGH);

+++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
@@ -0,0 +1,88 @@
+    if (isset($render_array['#title'])) {
+      $fragment->setTitle($render_array['#title'], Title::FILTER_XSS_ADMIN);

any reason for xss here? the logic is strange:
1) render page
2) try to set title for rendered fragment?

I'm not happy with the overall situation regarding HtmlFragment/HtmlPage, and #2327277: [Meta] Page rendering meta discussion has a lot of gaps/hand waving in terms of what the path forward will be. However this patch doesn't make any of that worse, so committed/pushed to 8.0.x, thanks!