Handle adding page title to content

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: 📌 Improve server-side error handling Active .

Fixing: not possible until much later, and why

fixing the block

This is not possible until maybe 📌 [later phase] ApiPreviewController must use the previewed route's controller, and override canonical content entity routes' received entity object Postponed , 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?

I think the solution for "Page title block" and "Main page content block" are different.

The "Main page content block" we should just hide from the XB library completely. I already opened an issue for this 🐛 Hide main page content block from XB Active .

The "Page title block" we should not hard code to a region because it's not uncommon for a theme to not want to display the page title at all. I assume that it would be more common for people set this in the content type template rather than in the page template. There are some edge cases where this doesn't work but in most cases people wouldn't want to show a title at least on the XB pages.

The proposed fallback seems like a good approach for now. The assumption that XB is always used on a content entity route is acceptable for now but this may change in future, e.g., to allow entering XB using a view page URL.

👍

P.S.: d'oh, right, I finished writing this while on a meeting — but yes, I intend for 📌 Consider refactoring page_template into page_region(s) Active to hardcode what the content PageRegion config entity contains: the main block, and nothing else!

The 'Drupal\Core\Block\TitleBlockPluginInterface' component interface must be present.

It looks like we also have an exception in place for the scenario where a title isn't placed. We should get rid of that since we should accept the fact that some sites may not want to place the title block in the page template.

@lauriii opened 📌 Page title should not be required Active for #10, and I fixed it.

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

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];

f.mazeikis → made their first commit to this issue’s fork.

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

f.mazeikis → changed the visibility of the branch 3502371-make-page-title to hidden.

f.mazeikis → changed the visibility of the branch 3502371-make-page-title to active.

Test coverage looks solid! 👍

@larowlan's review made me realize that once 🐛 Changes to code components in global regions is not loaded until published Active is implemented, a much nicer solution is possible here!

wim leers → credited larowlan → .

🐛 Changes to code components in global regions is not loaded until published Active 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 🌱 Milestone 1.0.0-beta1: Start creating non-throwaway sites Active because the page title block should work correctly if it is available at all.

Regarding #27.2 🐛 Handle adding page title to content Active :
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 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.
It's in MR1045, although it need 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 🐛 Handle adding page title to content Active and #10 🐛 Handle adding page title to content Active ?

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: 👎

f.mazeikis → changed the visibility of the branch 3502371-alt-approach to hidden.

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

Best experience would be to support adding the title block for pages too. How difficult would this be? If this is something that would be difficult to build and/or hard to maintain, we may want to reconsider that.

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.

So let's remove that restriction from both patterns and field component tree 👍

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"?

f.mazeikis → changed the visibility of the branch 3502371-alt-approach to active.

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 📌 Handle block contexts Active 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 🐛 PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant Needs work .

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 🐛 PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant Needs work 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.

+1 that we should not stick to the current page title block if it turns out to be more work than working around it would be.

@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 📌 Handle block contexts Active .

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: XB's `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:

f.mazeikis → changed the visibility of the branch 3502371-alt-approach to hidden.

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 🐛 Handle adding page title to content Active .

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 page as a logged in Drupal user with permissions to access admin theme, inital page load works correctly and has the page tile value rendered via lazy builder callback. But after refreshing the page, it seems that lazy builder makes a callback to re-render the page title block component 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 with fallback to empty string if it fails. Otherwise I am not quite sure what are our options, until #3370946 🐛 Page title should contextualize the local navigation Needs work , #2403359 → , #2359901 → and #2938129 🐛 PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant Needs work are not resolved.
@effulgentsia Suggested not allowing the use of page title block at all and switching to a title code component that uses title set in drupal.Settings, but I wonder if we would hit exact same issue where we need to resolve the value 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 newly challenges you identified in #53 on Monday.

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 🌱 [META] Production-ready ComponentSource plugins Active , 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.