Use the 'yield' option instead of output buffering for twig rendering to support async rendering

The bug was initially surfaced via a single Umami test failure in #1237636: Lazy load multiple entities at a time using fibers - this asserts the order of various content on the Umami front page.

What was happening with the bug itself was elusive for a while. Initially I thought that placeholders and their output were getting 'swapped' somehow. Need to open a couple of spin-off issues for cleaning up code that ultimately turned out not to be a problem, will link from here once that's done.

Closer inspection of the lazy builder markup output indicated that instead of being swapped, bits of placeholders were getting mashed together in the output of different placeholders. This was even more confusing and I still couldn't track it down. We're not assigning output to a static cache or anything like that anywhere, Fiber rendering can't be doing anything that weird even to recursive rendering of render arrays etc.

Then in the shower this morning (really) I remembered that Twig uses output buffering. So once the output buffer was started, different bits of different lazy builder markup were going into the output buffer depending on which placeholders suspended fibers.

MR may be the most hours per lines of code I've ever submitted.

Added some test coverage.

(Note: a percentage of the test coverage/test module was written using Claude cli, had to rewrite a lot of it but at least the skeleton was Claude. This is because Tag1 sponsored some time to work on any core/contrib issue, but specifically using Claude in order to evaluate it. I chose this one because I knew if I was able to track down the bug that I'd want to try to write a kernel test for it that didn't involve nested media/image rendering (which is what surfaced it in the entity loading issue). I don't think it saved any time overall since things constantly go wrong and it takes as much time thinking how to prompt it to get what you want as it would to have typed it out, and the things that do work could often be copypastad from a different test or module and altered (which is what I usually do to save time). The one thing that maybe saved time waswhen I asked it for some alternative ways to execute PHP code during Twig template rendering it suggested a custom twig function, which was a fairly minimalist way to do that - probably would have go there myself but hadn't yet at the point I prompted. - so a mixed bag).

Draft test only MR pipeline shows the correct test failure: https://git.drupalcode.org/issue/drupal-3546376/-/jobs/6520638#L4067

Making it clear we're changing a Twig option here and not rewriting how it renders templates.

The nightwatch failures are reproducible with the standard profile - you need to add a second level of menu items, then things get weird.

It looks like it's possible to have twig nodes that aren't compatible with yield - e.g. that print or echo, but I can't see any sign of anything like that yet.

This is (at least part of) the markup difference:

-<ul class="menu primary-nav__menu primary-nav__menu--level-1" data-drupal-selector="primary-nav-menu--level-1" data-once="olivero-automatic-mobile-nav">
+<ul class="menu primary-nav__menu" data-drupal-selector="primary-nav-menu--level-2" id="primary-menu-item-1">
             
                           
         
@@ -17,7 +17,7 @@
                                       
               <span data-drupal-selector="primary-nav-menu-🥕" class="primary-nav__menu-🥕"></span>
     
-    <ul class="menu primary-nav__menu primary-nav__menu--level-2" data-drupal-selector="primary-nav-menu--level-2" id="primary-menu-item-1">
+    <ul class="menu primary-nav__menu" data-drupal-selector="primary-nav-menu--level-2" id="primary-menu-item-1">

The missing class ( primary-nav__menu--level-1 or primary-nav__menu--level-2 is added in the olivero primary menu template with ::addClass(). This happens inside the menu_links macro so it's very possible it's something to do with the macro/recursion, no idea what though.

That is indeed very annoying.

Had a play around with different approaches, if we make sure create_attribute also clones, we can skip the toArray() - same overall approach though.

Pushed a commit for the original MR for that.

I looked at what was going on with Twig rendering a bit more.

Here's the relevant output from the actual generated twig template (e.g. what gets written to sites/default/files/php/twig):

    // line 48
                yield "
    <ul ";
                // line 49
                yield $this->extensions['Drupal\Core\Template\TwigExtension']->escapeFilter($this->env, CoreExtension::getAttribute($this->env, $this->source, CoreExtension::    getAttribute($this->env, $this->source, ($context["attributes"] ?? null), "addClass", ["primary-nav__menu", ($context["primary_nav_level"] ?? null)], "method", false, false,     true, 49), "setAttribute", ["data-drupal-selector", ($context["drupal_selector_primary_nav_level"] ?? null)], "method", false, false, true, 49), "html", null, true);
                yield ">
      ";
                // line 50
                $context["attributes"] = CoreExtension::getAttribute($this->env, $this->source, ($context["attributes"] ?? null), "removeClass",                                  [($context["primary_nav_level"] ?? null)], "method", false, false, true, 50);
                // line 51
                yield "      ";

I was confused why the ::escapeFilter() call doesn't cause the attribute to cast to string there, because Attributes implement ::__toString()

However, escapeFilter() passes through MarkupInterface objects without casting to string. Now I actually understand what's going on finally (maybe).

I've pushed a change that reverts all the template changes, and special cases atrributes in escapeFilter so that they get cast earlier.

Olivero menu rendering still works, let's see if this causes any other test failures though. If it works and doesn't cause other problems, that lets us switch to Yield with no changes for template authors.

I feel like we could even go a step further and cast any MarkupInterface object to a string in ::escapeFilter(), leaving only Twig Markup object completely intact there. But will see if we get a green test run first.

OK pushed another change that now early-casts for all MarkupInterface objects, this would mean anything attribute-like either now or in the future would also be covered.

Without use_yield, the MarkupInterface objects that are preserved in TwigExtension::escapeFilter() get immediately cast to a string anyway as soon as they leave it, with use_yield that can be slightly delayed which causes the problem here.

While this changes which method they get cast to a string by, I'm pretty sure it's only a fraction earlier with nothing else happening to them in-between, which allows us to keep the behaviour transparent for everything else.

If I'm overlooking something, we could go back to only casting Attributes early and handle the rest in a follow-up.

Copy and pasted #20 into a new test class. The unit test change doesn't prove that we fixed the bug, only that we changed the behaviour, the nightwatch test is... a nightwatch test.

Updated the issue summary. With this approach I don't think we need a change record for Twig template authors, but we probably need a general one in case someone somewhere is doing something weird with print in Twig nodes or something.

Added a change record.

@godotislate

One other option, though I'm not sure it's better: in escapeFilter() , return a clone of the object if an Attribute object is passed in.

I was doing this in an earlier revision, but didn't really want to special-case Attribute because something else could theoretically be implemented similarly and suffer from the same problem.

@alexpott

I wonder why we didn't do this in the first place.

I thought about this, and it looks to me like we just wanted to pass through anything that doesn't get sanitized with the least amount of change + it will get cast to a string almost immediately after this logic runs anyway. The method name is only about sanitization not about casting, so there might have been a reluctance to do more than was advertised too. But also I reckon we probably didn't think about the sanitization-skipping logic too much.