Make "Page title block" work A) also outside regions, B) on the only routes XB currently supports: content entity routes

Just ran into this. Error in log:

LogicException: The page_title_block block plugin does not support previews. in Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent->renderComponent() (line 145 of /var/www/html/web/modules/contrib/experience_builder/src/Plugin/ExperienceBuilder/ComponentSource/BlockComponent.php).

Related: the generalized variant of this problem/symptom: #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log).

Fixing: not possible until much later, and why

fixing the block

This is not possible until maybe #3491701: [later phase] ApiLayoutController must use the previewed route's controller, and override canonical content entity routes' received entity object, because this is one of the two highly special blocks in Drupal: they're fed information resulting from the execution of the route's controller!

Hence the error message, which originates from:

// Allow global context to be injected by suspending the fiber.
// @see \Drupal\experience_builder\Plugin\DisplayVariant\PageTemplateDisplayVariant::build()
if ($block instanceof MainContentBlockPluginInterface || $block instanceof TitleBlockPluginInterface) {
if (\Fiber::getCurrent() === NULL) {
throw new \LogicException(sprintf('The %s block plugin does not support previews.', $block->getPluginId()));
}
\Fiber::suspend($block);
}

Note that the "main content" block has the same problem, and triggers the same exact error message (I just manually tested to verify). Issue title updated. 👍

And even then (after #3491701) this would be tricky to get right: if you're modifying the title of a node, then we'd have to:

• get XB UI-submitted data to be transformed into an ephemeral content entity object
• automatically get the canonical entity route's _title callback executed:
• IOW: pass the ephemeral entity to \Drupal\Core\Entity\Controller\EntityController::title()
• pass the return value of that into \Drupal\experience_builder\Plugin\DisplayVariant\PageTemplateDisplayVariant
• … which in turn would allow the code above to be modified to call \Drupal\Core\Block\TitleBlockPluginInterface::setTitle()

🤯, right?!

But we'll have to do almost exactly that. We can't change all innards of Drupal.

Work-around: fallback

Unless you are fine with adding fallback rendering! 😄

We could make ApiPreviewController leap ahead of #3491701 slightly and make it assume that it's always used on a content entity route (which is indeed true in XB's UI today):

1. use $label_field_name = $entity->getEntityType()->getkey('label') to get the name of the label field
2. use $body['entity_form_fields'][$label_field_name . '[0][value]'] to get the current entity title
3. wrap the ->toRenderable() call in fiber execution similar to \Drupal\experience_builder\Plugin\DisplayVariant\PageTemplateDisplayVariant::build()

Tada, it works!

Alternative: disallowing

disallowing adding the block there

This is maybe possible. Depends on what you mean by "there".

You talked before about how the "main content" block should always be placed in the content region. Can we do the same for the "page title" block? Even though some themes may want to place it somewhere else?

Olivero puts it in the content_above region by default, so this seems like a bad idea?

👍

P.S.: d'oh, right, I finished writing this while on a meeting — but yes, I intend for #3501600: Split 1 PageTemplate config entity into N PageRegion config entities to hardcode what the content PageRegion config entity contains: the main block, and nothing else!

@lauriii opened #3505872: Page title block should not be required for #10, and I fixed it.

I think the work-around I articulated 15 days ago at might've become simpler thanks to #3506267: Changing page title doesn't update in the review changes panel.

What I proposed appears to work. This still needs refining though:

diff --git a/src/Controller/ApiLayoutController.php b/src/Controller/ApiLayoutController.php
index e8804d48..287a39bc 100644
--- a/src/Controller/ApiLayoutController.php
+++ b/src/Controller/ApiLayoutController.php
@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace Drupal\experience_builder\Controller;
 
 use Drupal\Component\Utility\NestedArray;
+use Drupal\Core\Block\TitleBlockPluginInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\EntityPublishedInterface;
 use Drupal\Core\Entity\EntityTypeInterface;
@@ -37,6 +38,8 @@ final class ApiLayoutController {
   private array $regions;
   private array $regionsClientSideIds;
 
+  private $t;
+
   public function __construct(
     private readonly AutoSaveManager $autoSaveManager,
     private readonly ThemeManagerInterface $themeManager,
@@ -353,10 +356,25 @@ final class ApiLayoutController {
       'model' => $model,
       'entity_form_fields' => $body['entity_form_fields'],
     ], $entity, validate: FALSE);
+    // @todo improve
+    $entity_title = $body['entity_form_fields']['title[0][value]'];
     $field_name = InternalXbFieldNameResolver::getXbFieldName($entity);
     $item = $entity->get($field_name)->first();
     assert($item instanceof ComponentTreeItem);
-    $renderable = $item->toRenderable();
+    $fiber = new \Fiber(fn() => $item->toRenderable());
+    $component_instance = $fiber->start();
+    // @see \Drupal\experience_builder\Plugin\DisplayVariant\XbPageVariant::build()
+    while ($fiber->isSuspended()) {
+      $component_instance = match (TRUE) {
+        $component_instance instanceof TitleBlockPluginInterface => (function () use ($component_instance, $fiber, $entity_title) {
+          $component_instance->setTitle($entity_title);
+          return $fiber->resume();
+        })(),
+        default => new \LogicException(),
+      };
+    }
+    assert($fiber->isTerminated());
+    $renderable = $fiber->getReturn();
 
     if (isset($renderable[ComponentTreeStructure::ROOT_UUID])) {
       $build = $renderable[ComponentTreeStructure::ROOT_UUID];

Test coverage in \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest seems like the most feasible approach.

Test coverage looks solid! 👍

@larowlan's review made me realize that once #3512385: Changes to code components are not visible in global regions until published is implemented, a much nicer solution is possible here!

#3512385: Changes to code components are not visible in global regions until published is in!

CI-only job fails, proving the test coverage is accurate 👍

Spotted one bug though, which also shows test coverage is not yet complete.

… and having written #26, I spotted the tag.

I added it in #21. And … that does suggest a better approach that should be possible to implement now: one where

1. we actually don't need this new trait
2. leave all the page title block shenanigans to XbPageVariant
3. and instead ensure that \Drupal\experience_builder\Controller\ApiLayoutController::getLabel()'s result gets passed into \Drupal\experience_builder\Plugin\DisplayVariant\XbPageVariant::setTitle()

😅🙈

IMHO blocks #3515932: Milestone 1.0.0-beta1: Enable creation of non-throwaway sites because the page title block should work correctly if it is available at all.

Regarding #27.2:
What about block title shenanigans for situations, where XB is not handling page templates? This currently is the default behaviour on fresh install.
In such situations there are no Page Region entities and Drupal\experience_builder\EventSubscriber\PageVariantSelectorSubscriber:: onSelectPageDisplayVariant() returns early instead of selecting XbPageVariant to do rendering.

I was playing around with different approach - removing the trait, removing the logic from ApiLayoutController and letting BlockComponent plugin source handle title block value with title_resolver service. It works, but only on requests with routes that have title resolution. It also breaks some of the tests - haven't dug deep enough to see what's happening, seemed something to do with XbPageVariants cache tags.
It's in MR1045 if you want to take a look, although it needs more work and it does add dependencies to BlockComponent that I am uncertain about.

Should this MR also remove validation from schema.yml and in-code that prevents users from placing title blocks wherever/however they want on in layout? Or do I misinterpreted #5 and #10?

Very good point — @lauriii in #5 specifically stated:

I assume that it would be more common for people set this in the content type template rather than in the page template.

→ Which means:

1. we cannot rely on XbPageVariant, because that code is only executed if XB PageRegion config entities exist ⇒ this means @larowlan's insight linked from #21 actually is not a viable solution, despite being more elegant.
2. we must remove the Drupal\Core\Block\TitleBlockPluginInterface from the absence list of experience_builder.content_template.*.*.*

(but not of other component trees AFAICT, because it would still make little sense on individual content entities — unless @lauriii says otherwise — assigning to him to get final confirmation of that, #10 does seem to suggest he wants it to be supported everywhere?)

letting BlockComponent plugin source handle title block value with title_resolver service. It works, but only on requests with routes that have title resolution.

That indeed won't work universally. It's not required for a route to specify _title or _title_callback — the render array returned by a controller can also contain the page title. See https://www.drupal.org/node/2067859 and specifically #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks.

So: 👎

Removed Drupal\Core\Block\TitleBlockPluginInterface from absence list in experience_builder.content_template schema, updated tests accordingly.

We still need @lauriii to clarify this:

(but not of other component trees AFAICT, because it would still make little sense on individual content entities — unless @lauriii says otherwise — assigning to him to get final confirmation of that, #10 does seem to suggest he wants it to be supported everywhere?)

Don't thinks it's difficult per se. When you say "pages" - we currently have still have limitations on field component tree and on patterns. I assume you mean we should remove limitation from field component tree? This would allow title blocks to be placed and saved on individual Node layouts.
If so, I would argue that it would make sense to allow patterns to use title block too, as otherwise we're leaving in somewhat convoluted and meaningless restriction.

Removing limitations in general is not difficult, it becomes difficult if there's "granularity and rules" to how limitations should be applied (if the assumptions above are incorrect, for example).

I have also just discovered a bug - with XB not handling templates, we need to figure out how to handle Block title rendering outside of preview.

This looks great! 👍

AFAICT block-form.cy.js is consistently failing? 🙈😬

Seems to have started since 433b7a1e342e19108edcda376726f3fcda1f6502: Unrestric usage of title block. Seems legitimate.

@wim-leers The issue seems legitimate, but not related to your suggestion in MR. I'll look into that, but first we need to figure out a way forward out of "Page Title Block isn't rendering, when placed in content region and viewed outside of XB".

We have so many paths for rendering page title block, but not all of them work:

1. XB doesn't handle templates and title block is placed in a content region during preview - works as intended ✅
2. XB handles templates and title block is placed in a non-content region during preview - works as intended ✅
3. XB handles templates and title block is placed in a non-content region during page view - works as intended ✅
4. XB handles templates and title block is placed in a content region during preview - works as intended ✅
5. XB doesn't handle templates and title block is placed in a non-content region during page view (using Drupal blocks placement) works as intended ✅
6. XB doesn't handle templates and title block is placed in a content region during page view - doesn't work ❌
7. XB handles templates and title block is placed in a content region during page view - doesn't work ❌

The issue is combination of things:

• XbPageVariant only renders components in fibers, if XB manages templates and there are PageRegion entities
• XbPageVariant handles content region differently from the rest of the region (we intentionally skip over creating PageRegion entity for the content in the first place)
• ApiLayoutController solves "title block in content region" problem during Preview, by rendering title block in a fiber and providing title value
• Outside of Preview nothing starts the fiber for Page Title block - XbPageVariant skips this region and ApiLayoutController only works during Preview in XB (hence the weird conditional in BlockComponent::renderComponent() on line 161
• Outside of Preview, nothing provides title block value

We could resolve this by moving "title resolution and value setting" logic out of XbPageVariant and ApiLayoutController into BlockComponent::renderComponent(). This is what "alt approach" MR was trying to achieve. It works, but adds seemingly unnecessary dependencies and complicates BlockComponent class for one special case handling.

We could also fire some sort of ComponentBlockPreRender event in  BlockComponent::renderComponent() - that would allow us to extract this logic into new PageTitleBlockComponentRenderSubscriber. This way the dependencies and logic required for resolving and setting title value could live in it's own isolated subscriber. This would allow us to only concern ourselves with value coming from Title Resolver service for now, as we currently only have limited support for Nodes. Later this be extended by modifying such subscriber or registering additional subscribers for additional routes, without touching BlockComponent class itself.
This might prove to be a useful mechanism down the line for setting block plugin values before rendering happens for other types of blocks - Views blocks immediately come to mind.

Alternatively, we could try to do "title resolution and value setting" where we render "content" region, but that feels quite iffy.

Is there a simple alternative I am unable to see here?
Is there any particular reason _we have_ to do "alter blocks by rendering them in fibers" bit in XbPageVariant, especially when it comes to title block? Like an upcoming feature I am not aware of that would allow us to "have different Page variants with different titles"?

We could also do the easiest solution and add the validation that returns us to "you can place title block in content region and XB doesn't crash, but you can't save that". Personally, not a fan of doing that.

I like ComponentBlockPreRender event.
We might need this for blocks that we don't support yet aside of TitleBlock. E.g. those with lazy render placeholders?
Might be used too in #3485502: [needs design] Handle block contexts (aka implicit inputs vs `settings` aka explicit inputs) for setting the context values when we support context aware blocks.

Or even a more general ComponentSourcePreRender event, because this might apply to other component sources, not only blocks.

By the way, Layout Builder doesn't let you add the page title block either, with an @todo pointing to #2938129: PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant.

Personally, I don't think we need to support the page title block until that issue is fixed. Instead, I think we can:

• Add the page title into drupalSettings. E.g., perhaps via a hook_preprocess_html()? Then code components have access to it easily, so you could just implement a code component that does <h1>{drupalSettings.pageTitle}</h1> or similar.
• Later, when we add (UI) support for dynamic prop sources, we could make the page title available to SDCs, such as to a regular Heading component, as the value of whatever string prop gets linked to the page title.

Then, when #2938129: PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant is resolved, we can make the page title block available for people who really want to do it as a block, though if the above two options are available, what's the point of the block?

Just my 2 cents.

@f.mazeikis in #41: .6 and .7 — ouch, great finds! 😬 This clearly indicates missing test coverage for that. Let's add those.

Points 6 and 7 are impossible to solve using the current mechanisms, because a controller returning ['#title' => 'something'] literally has not yet been executed, because the XB field is rendering the page title block before the main content has been rendered:

We could also fire some sort of ComponentBlockPreRender event in BlockComponent::renderComponent()

I worry about the performance impact of this — one such event fired per component instance could result in hundreds of events fired for complex component trees 😅

This might prove to be a useful mechanism down the line for setting block plugin values before rendering happens for other types of blocks - Views blocks immediately come to mind.

🤔 Why would we want/need to do that? What's really needed for views blocks (that show something contextually relevant, e.g. all related news articles) do so using the context system. For that, we have #3485502: [needs design] Handle block contexts (aka implicit inputs vs `settings` aka explicit inputs).

Is there any particular reason _we have_ to do "alter blocks by rendering them in fibers" bit in XbPageVariant, especially when it comes to title block?

That reason is \Drupal\Core\Display\PageVariantInterface::setTitle(), which itself is necessary because of controllers being allowed to return render arrays with #title in their root, and that will override the _title_resolver result of the associated route 😬 See the relevant CR.

@penyaskito in #43: RE #lazy_builders for block plugins: see #3518656, and #3518656-12: Add Dynamic Page Cache and BigPipe support: `block` ComponentSource does not use placeholders/#lazy_builder for my outline of a plan for how to support that.

@effulgentsia in #44: wow, TIL about #2938129! And glad your mind is also going to (new) prop sources 🤓

Thinking… 🤔

Note: the original scope was to just make the page title block work in the places where it can be placed in HEAD.

But @lauriii requested the scope to be expanded. I'm still not really convinced why it's a good idea to place the page title block in e.g. a landing page (xb_page content entity) or an article. IMHO this likely means the page title will appear twice — because if it ever is placed in a landing page, then essentially every content template will have to place it, too. Which … I guess is the very intent/point, to have that freedom. It kinda makes sense, but it seems debatable.

In any case: updating the issue title to reflect the broader scope.

@penyaskito's #lazy_builder remark in #43 gave me an idea 🤓

1. The root of the problem here is that the page title is global state.
2. That global state is literally not yet available at the time we render the page title block inside a rendered content entity.
3. Drupal's rendering system has a mechanism to delay rendering: #lazy_builders.
4. If we can receive the global state, and put it in a place where it'll be accessible to the (stateless & dependency-free!) #lazy_builders, then we should be able to make it work.
5. Sadly, on sites with BigPipe installed, that'll mean that the page title will flicker, because it'll be delivered later. Except … that was fixed recently, with the introduction of #placeholder_strategy_denylist: https://www.drupal.org/node/3511562
6. Result: #41.6 works now:
   

Making the lazy builder preview-aware fixes 50% of the scenarios in the XB preview:

And making ApiLayoutController pass the current entity title (auto-saved or not) results in 100% working:

Reflecting that @f.mazeikis is reviewing & working on making tests pass :)

Met with @f.mazeikis and dug in to the remaining challenges. We agreed there's 2 remaining steps:

1. Add 2 test cases to \Drupal\Tests\experience_builder\Kernel\Plugin\Field\FieldType\ComponentTreeItemListTest::provider():
   

       yield 'component tree with a page title block component' => [...self::removePrefixSuffix($component_tree_with_page_title_block_component), 'isPreview' => FALSE];
       yield 'component tree with a page title block component in preview' => [...self::modifyExpectationFromLiveToPreview($component_tree_with_page_title_block_component, TRUE), 'isPreview' => TRUE];
   

   — essentially reusing $component_tree_with_single_block_component but changing it to use the page title block instead.

   This will require an addition like this to \Drupal\Tests\experience_builder\Kernel\Plugin\Field\FieldType\ComponentTreeItemListTest::testHydrationAndRendering():

   diff --git a/tests/src/Kernel/Plugin/Field/FieldType/ComponentTreeItemListTest.php b/tests/src/Kernel/Plugin/Field/FieldType/ComponentTreeItemListTest.php
   index ba8df6349..76d51f06a 100644
   --- a/tests/src/Kernel/Plugin/Field/FieldType/ComponentTreeItemListTest.php
   +++ b/tests/src/Kernel/Plugin/Field/FieldType/ComponentTreeItemListTest.php
   @@ -18,6 +18,7 @@ use Drupal\experience_builder\Element\RenderSafeComponentContainer;
    use Drupal\experience_builder\Entity\AssetLibrary;
    use Drupal\experience_builder\Entity\Page;
    use Drupal\experience_builder\Exception\SubtreeInjectionException;
   +use Drupal\experience_builder\Plugin\DisplayVariant\XbPageVariant;
    use Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItemList;
    use Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItemListInstantiatorTrait;
    use Drupal\experience_builder\Render\ImportMapResponseAttachmentsProcessor;
   @@ -118,6 +119,7 @@ class ComponentTreeItemListTest extends KernelTestBase {
        });
        $this->assertEquals($expected_renderable, $renderable);
    
   +    XbPageVariant::finalTitle = 'Funny string chosen by Felix :D';
        $html = (string) $this->container->get(RendererInterface::class)->renderInIsolation($renderable);
        // Strip trailing whitespace to make heredocs easier to write.
        $html = preg_replace('/ +$/m', '', $html);
   

2. Make sure the page title block also works when not using XbPageVariant:

   1. final class XbBlockPageVariant extends BlockPageVariant {
        public function setTitle($title) {
          XbPageVariant::finalTitle = $title;
          return parent::setTitle($title);
        }
      }
      

   2. usie hook_display_variant_plugin_info_alter() to override the class used for the block_page page variant to XbBlockPageVariant::class

   Once those 2 things are done, let's land this!

   Pre-emptively RTBC'ing because I'll be out in the next 2 days.

I've implemented things we discussed with Wim and he outlined in #52.

All of this sounded really reasonable, but I've hit some issues.

1. New test in ComponentTreeItemListTest checks if setting a value to BlockComponent::$finalTitle results in that value being used for page title block component. What it doesn't test is whether BlockComponent::$finalTitle is always initialised and has value without us doing so on Line 122 of ComponentTreeItemListTest.
2. When rendering a Node page as a logged in Drupal user with permissions to access admin theme the initial page load works correctly and has the page tile value rendered via lazy builder callback. After refreshing the page it seems that when lazy_builder makes a callback for the page title block component it gets re-rendered instead of being served from cache and this time (and all the following attempts) result in exception Typed static property Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::$finalTitle must not be accessed before initialization. This happens because placeholder is being re-rendered in isolation, without invoking BlockPageVariants or any other code that would set BlockComponent::$finalTitle value. This doesn't seem to happen for fully rendered page visited by anonymous users - see the video.

Screen recording of the Exception

I've tried updating to Drupal 11.2 in order to see if using '#placeholder_strategy_denylist' => [BigPipeStrategy::class => TRUE], would resolve this problem - it did not. The same error happens in the same way, it's just that rendering of placeholder in isolation happens via Drupal Core renderer and not through BigPipe.

We could add our best attempt to resolve page title via title_resolver service to the BlockComponent::renderPageTitleBlock with fallback to empty string if it fails.

Otherwise I am not quite sure what are our options, until #3370946, #2403359, #2359901 and #2938129 are resolved. As it seems that the majority of our problems here are related to Page Title Block not resolving page title by itself and instead relying on page title value to be resolved and set elsewhere, like in BlockPageVariant.

@effulgentsia Suggested disallowing the use of page title block and switching to a title code component that uses title set in drupal.Settings, but I wonder if we would hit exact same set of problems where we need to resolve the page title value in order to be able to pass it to drupal.Settings.

The test you added in https://git.drupalcode.org/project/experience_builder/-/merge_requests/7... looks solid. 👍

I’ll review the new challenges you identified in #53 on Monday. (Thanks for the detailed comment, that is very appreciated!)

Just move on to a different issue, I don’t want you to have nightmares about this over the weekend 🧟 😅

This kinda belongs more under #3520484: [META] Production-ready ComponentSource plugins, so moving it there.

I think this is close to landing, so whether or not it's tagged as a "beta blocker" is probably moot, but I'm untagging it per #44.

ℹ️ #3531249: Populate data in `drupalSettings` to enable simple use cases in dynamic code components landed, so now this issue should update the page title bits that introduced.

Still getting these:

"page_title_block block plugin does not support previews"

in the logs

MR needs updating as patch fails

@kristen pol: yes, that is known.

Unfortunately, shortly after this was assigned to me to try to overcome the last obstacles identified by Felix, this was deprioritized in favor of higher-priority issues. Unassigning.

For awareness: #3561564: Support the same block being used for page title & main content