Page Title block for Layout Builder does not have a place holder

Modified page title block to use route to populate title during build(), added check in Section rendering loop to change title text to a placeholder. There is some question on what routes should be used to check against so this only appears in layout builder admin, or a better way to identify when the placeholder should be shown.

Created with @tedbow, @geinosky, and @lokapujya

I concur.

Testing latest patch.

1. Enabled Field Layout, Layout Builder, Layout Discovery modules (not sure what all I need, but that seems comprehensive).
2. Navigated to /admin/structure/types/manage/article/display-layout/default .
3. Added page title, confirmed undesirable behavior.
4. Canceled layout editing.
5. Installed patch: $ curl -l https://www.drupal.org/files/issues/2018-04-13/page-title-block-2959962-4.patch | git apply -v. Succeeded with no errors or warnings.
6. Re-navigated to /admin/structure/types/manage/article/display-layout/default .
7. Added page title, confirmed there is now a placeholder. See screenshot below.

Moving to RTBC.

How should this be tested? A test that checks all Core provided blocks in the layout builder that should have a placeholder?

@tedbow, given that it's unclear to me when programmatic tests are needed, should I still be moving things to RTBC when I manually test them? I figure if I don't move things to RTBC and they don't need programmatic tests, then someone else has to come along, notice that, and move it to RTBC (which slows the process down for those issues).

Happy to follow whatever process is recommended.

@runeasgar, Thank You for testing the issue! Any bugfix in Drupal core needs an automated test. See: Drupal core gates. So, instead of marking RTBC, add the "Needs Tests" tag.

Some day I'll need to learn to write said tests ^_^ seems like low-hanging fruit.

Thanks! I will take a look when I have some time to sit down and learn.

Started a test.

Let me actually run this and see what happens.

Nice test!

1. +++ b/core/lib/Drupal/Core/Block/Plugin/Block/PageTitleBlock.php
   @@ -16,14 +21,75 @@
       * @var string|array
       */
   -  protected $title = '';
   +  protected $title = NULL;
   +
   

   The @var needs updating

2. +++ b/core/lib/Drupal/Core/Block/Plugin/Block/PageTitleBlock.php
   @@ -16,14 +21,75 @@
   +  protected $title = NULL;
   ...
   +  protected $titleResolver = NULL;
   ...
   +  protected $request = NULL;
   ...
   +  protected $routeMatch;
   

   = NULL is redundant, please remove

3. +++ b/core/lib/Drupal/Core/Block/Plugin/Block/PageTitleBlock.php
   @@ -16,14 +21,75 @@
   +   * @var null|TitleResolver $titleResolver
   ...
   +   * @var null|RequestStack
   ...
   +   * @var \Drupal\Core\Routing\RouteMatchInterface
   

   These should use the fully-qualified class name, not include the variable name, and should not have the |null (if they're set in the constructor, which they are)

4. +++ b/core/modules/layout_builder/src/Section.php
   @@ -78,6 +79,15 @@ public function toRenderArray(array $contexts = [], $in_preview = FALSE) {
   +        // Add placeholder for page_title_block in layout_builder admin.
   +        if (isset($output['#plugin_id']) && $output['#plugin_id'] === 'page_title_block') {
   +          $route = \Drupal::routeMatch();
   +          // @todo Many layout routes are run from admin, this may be too broad.
   +          if (strpos($route->getRouteName(), 'layout_builder') !== FALSE) {
   +            // Change title to a placeholder.
   +            $output['content']['#title'] = new TranslatableMarkup('@label - Placeholder for the Page Title Block', ['@label' => $output['#configuration']['label']]);
   +          }
   +        }
   

   Hmmm. I really don't like special-casing this code here. If it must live outside the page title block, can you move this to an event subscriber for the LayoutBuilderEvents::SECTION_COMPONENT_BUILD_RENDER_ARRAY event?

@tim.plunkett for this:
if (strpos($route->getRouteName(), 'layout_builder') !== FALSE) {

I saw you mentioned in slack that if ($route_match->getRouteObject()->getOption('_layout_builder')) { may be a better option, should we do that here?

Yes definitely would be better. But my point remains, this is not the place to be inserting per-block logic.

Also, at the Nashville sprint I thought it was suggested that this could use a lazy builder and placeholder, and then it could live inside the block itself. Was that ever explored?

Adressed #21.

Not entirely sure if the PageTitleSubscriber should belong in Core like in this uploaded patch, or in layout_builder, ... or somewhere else.

Alright, looks like testbot did answer that question for me. Here it is in layout_builder module instead.

Oh this makes me feel so much better to see as an event! It will give people an example of how to manipulate block output specifically for Layout Builder.
We might eventually want to codify this better with an event base class... But that can be for another time

1. +++ b/core/lib/Drupal/Core/Block/Plugin/Block/PageTitleBlock.php
   @@ -44,6 +110,10 @@ public function defaultConfiguration() {
   +    // Page Title may not already be set before build, populate via route.
   +    if ($this->title === NULL) {
   +      $this->title = $this->titleResolver->getTitle($this->request->getCurrentRequest(), $this->routeMatch->getRouteObject());
   +    }
   

   I'm not 100% sure how this relates to the placeholder change. And it doesn't seem to have its own test coverage

2. +++ b/core/modules/layout_builder/src/EventSubscriber/PageTitleSubscriber.php
   @@ -0,0 +1,70 @@
   +    $block = $event->getPlugin();
   ...
   +    if (isset($build['#plugin_id']) && $build['#plugin_id'] === 'page_title_block') {
   

   $build isn't super reliable, this could be $block->getPluginId() === 'page_title_block'

3. +++ b/core/modules/layout_builder/src/EventSubscriber/PageTitleSubscriber.php
   @@ -0,0 +1,70 @@
   +        $build['content']['#title'] = new TranslatableMarkup('@label - Placeholder for the Page Title Block', ['@label' => $build['#configuration']['label']]);
   
   +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -319,4 +319,41 @@ public function testLayoutBuilderUiFullViewMode() {
   +    $assert_session->pageTextContains('Page title - Placeholder for the Page Title Block');
   

   The "@label - Placeholder..." seems redundant.

   Instead I'd suggest 'Placeholder for the "@admin_label" block' with @admin_label corresponding to $block->getPluginDefinition()['admin_label']
   Also, should it be #title or #markup?

1. It kinda does. Actually this issue solves two bugs (I just realised myself), because Page title does not render when placed on LB UI without this part of the code.
So, this part of the test kinda verifies this code change, but not very explicitly:

$assert_session->elementExists('css', '.block-page-title-block');

So perhaps it deserves to be solved in another issue which this one may be blocked on, or we write a test for it here, or we can extract that part of the code and let the page title subscriber actually set the title in the context for LB. What do you think?

The Page title block title is set by BlockPageVariant when placed on a region through Block UIs.

Edit: Looks like these problems already been adressed in this issue: https://www.drupal.org/project/drupal/issues/2938129

2. Fixed

3. Fixed

It should be #title, since that is the variable used by the page_title render element.

+++ b/core/lib/Drupal/Core/Block/Plugin/Block/PageTitleBlock.php
@@ -16,14 +21,75 @@
-class PageTitleBlock extends BlockBase implements TitleBlockPluginInterface {
+class PageTitleBlock extends BlockBase implements TitleBlockPluginInterface, ContainerFactoryPluginInterface {

I'd rather move these changes to #2938129: PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant (good find, I knew there was an issue but couldn't locate it)

The interdiff changes are great.

Alright, I reverted changes made in PageTitleBlock. Tests remains the same, so no test only reupload.

Nice, this looks really great. One more round of feedback from me.

1. +++ b/core/modules/layout_builder/src/EventSubscriber/PageTitleSubscriber.php
   @@ -0,0 +1,70 @@
   +    $events[LayoutBuilderEvents::SECTION_COMPONENT_BUILD_RENDER_ARRAY] = ['onBuildRender', 90];
   

   Why the 90 priority? If it needs to be something other than 0, this needs a comment explaining it.

2. +++ b/core/modules/layout_builder/src/EventSubscriber/PageTitleSubscriber.php
   @@ -0,0 +1,70 @@
   +    $build = $event->getBuild();
   ...
   +        $build['content']['#title'] = new TranslatableMarkup('Placeholder for the "@admin_label" block', ['@admin_label' => $block->getPluginDefinition()['admin_label']]);
   +        $event->setBuild($build);
   

   I know you said that the existing build needs this to be #title. But I would expect that instead of retrieving the existing build, that this could simply set build to a new render array. And if not, the getBuild could move inside the if()

3. +++ b/core/modules/layout_builder/src/EventSubscriber/PageTitleSubscriber.php
   @@ -0,0 +1,70 @@
   +    if ($block->getPluginId() === 'page_title_block') {
   +      if ($this->routeMatch->getRouteObject()->getOption('_layout_builder')) {
   

   Might as well one-line this

#40.1 Fixed.

#40.2 Moved getBuild() within if().

#40.3 Fixed.

We discussed the other part of #41.2, and the current approach is needed to keep the rest of the block working.

If that passes tests (only reason it wouldn't is the priority change), then I think this is done!

Enabled Field Layout, Layout Builder, Layout Discovery modules (not sure what all I need, but that seems comprehensive).

Keeping this up to date.

1. I've already reproduced this issue previously on 8.6.x, so skipping straight to testing the fix.
2. Applying patch: curl -l https://www.drupal.org/files/issues/2018-05-01/2959962-46.patch | git apply -v
3. Applied clean.
4. Enabling Field Layout, Layout Builder, Layout Discovery modules.
5. Navigated to /admin/structure/types/manage/article/display-layout/default
6. Added page title: confirmed new, desirable behavior. Screenshot below.

However, there is an inconsistency.. sort of. Look at the comments placeholder.. it's much more verbose - I know it's for comments. But, "darithijacrogiuucloswojuuaprahinispeliw"? I realize this is some random generation thing, meant to help ensure your content will fit where you're putting it.. but this seems really confusing for an end-user.

How would I know this is the title if I was another end-user coming in to look at this later? I'd have to click the pencil, then configure, in order to see that. Seems pretty bad.

Regardless, setting as RTBC. It's better than the current behavior.

Page title does not render when placed on LB UI when javascript is enabled; The test passes because it doesn't use AJAX. Another patch (event subscriber?) is needed in order to fix the LB UI.

@runeasgar
This random text is the page title override.

+++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
@@ -349,4 +349,41 @@ public function testLayoutBuilderChooseBlocksAlter() {
+    $assert_session->elementExists('css', '.block-page-title-block');

Yeah but this is not doing the expected thing of actually displaying the page title. I've tested this manually and it's empty on node/1 even though the page title is the node title.

Also as #48 points out with Javascript enabled the UI doesn't really work either.

Am I missing something?

@alexpott

See #34 and #38. This issue only intends to solve the placeholder on LB UI for the Page title.

Okay well we still have the problem with real-life javascript enabled usage.

Re-rolled against 8.7.x

Manually tested this and it works as expected.

Concerning the real-life JavaScript issue mentioned in #49/#51, this also happens for the core Tabs block.
After refreshing the page the block content is visible until you start rearranging blocks.
If we can agree this is something for a follow up, I'm fine with RTBC-ing this again.

Applied manually #54 and it shows the placeholder only after page refreshing. Once you add the block - no placeholder is shown.
Attaching screenshot.

Let's postpone this on the new proposed block placeholder API: #2992410: Provide placeholders for empty blocks (for example, an empty Views listing)

New approach

FYI #3029819: Do not allow Page Title block to be placed in Layout Builder until it works properly removed the page title block from the LB block selection list, with an @todo pointing to #2938129: PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant to restore it.

So this block now no longer exists in the UI? Is that right? So do we postpone this on #2938129: PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant ?

Yea I think this can be marked a duplicate for #2938129: PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant believe when that lands this will be fixed. Am leaving all credit too for all the work.