Allow field blocks to display the configuration label when set in Layout Builder

Thanks for the patch!
This will need automated tests

#2 worked for me (still on version 8.6.15).

I think the comments in the preprocess block could be improved. You have this:

/**
 * Implements hook_preprocess_block().
 */
function layout_builder_preprocess_block(&$variables) {
  // Only override the label with the configuration label for field blocks.
  if ($variables['elements']['#base_plugin_id'] === 'field_block') {
    if (!empty($variables['configuration']['label']) && !empty($variables['configuration']['label_display'])) {
      $variables['label'] = $variables['configuration']['label'];
    }
  }
}

How about this:

/**
 * Implements hook_preprocess_block().
 */
function layout_builder_preprocess_block(&$variables) {
  // Prevent field label from overriding the configured block title for field blocks.
  // See template_preprocess_block from block module for why this is necessary.
  if ($variables['elements']['#base_plugin_id'] === 'field_block' && !empty($variables['configuration']['label']) && !empty($variables['configuration']['label_display']) {
    $variables['label'] = $variables['configuration']['label'];
  }
}

Also, this may be a breaking change at this point since LB is now stable. I think the impact is probably quite low, but it's possible people have previously configured their block titles to something different than field label, and now that alternate title will show.

Agree with #10, so adding updated patch with an interdiff.

I think it's missing a closing parenthesis

That's right @bkosborne, Updated patch with an interdiff.

#14 seems to work with 8.8

Le me re-roll the patch.

It shows error: 1) Drupal\Tests\layout_builder\Functional\LayoutBuilderTest::testFieldBlockLabel
The link Add Block was not found on the page.
Failed asserting that an array has the key 0

can someone clarify this?

I have fixed the broken tests, test it out manually and it works as expected.

I tested the latest patch (#22) on Drupal 8.9 and it worked great. Before the patch, my custom title did not show on the page, only the default. After the patch, my custom title is rendered.

Queued a test-run on 9.1

I've also tested the patch in #22 and can confirm it's working as expected now.

I've also tested patch #22 in Drupal 9.1 via dreditor/simplytest and it works as intended in multiple block tests.

Assigning to myself to test patch #22

Tested patch #22 in Drupal 9.1 via dreditor/simplytest and it works as intended in multiple block tests.

This is not a bug, it's a feature request. Field UI does not allow you to change the text of the Field label. That textfield shown is the for Block title.
Label != Title.
Yes, that's confusing to have both.
But I think this patch makes them even more confusing by reusing one as the other. This would break the display of existing sites.

Aha, I partially misunderstood the issue, due to the label vs title in discussions.

+++ b/core/modules/layout_builder/layout_builder.module
@@ -406,6 +406,18 @@ function layout_builder_preprocess_language_content_settings_table(&$variables)
+  // Prevent field label from overriding the configured block title for field
+  // blocks. See template_preprocess_block from block module for why this is
+  // necessary.
+  if ($variables['elements']['#base_plugin_id'] === 'field_block' && !empty($variables['configuration']['label']) && !empty($variables['configuration']['label_display'])) {
+    $variables['label'] = $variables['configuration']['label'];
+  }

The conditional doesn't match with what's in template_preprocess_block. It's not just fields that are the problem, but anything with a #title that happens to be in the exact magical spot.

Here's an alternate patch. Test is identical, just swapped the preprocess change for one to FieldBlock.

I can confirm patch #33 works for me on 8.9.x as well.

Moved this comment to correct issue.

#33 works for me as well on both D8 and D9. Thank you!
However, I don't get why the tests are failing.

#22 worked for me as well in drupal 8

Resolved the above custom commands failed issues in #33 Working fine with 9.2.x

This should fix the remaining failures

Fix of first fail, not sure yet about second as that one passes locally :)

#46 worked for me on 9.2.7

#46 working on 9.3.6

Also confirming that #46 works with 9.3.6

Confirming that #46 works with 9.3.7

#46 not working for 9.4.5

#46 is working for 9.3.19

#46 is working for me on Drupal 9.5.5

#46 patch applied cleanly and fixed issue. Patch tests are passing on both 10.0.x and 10.1.x. I don't see any comments with outstanding issues so setting to RTBC.

Just adding some context since it's not the first time I've bumped into this issue and tried to recall how this worked:

2158003 added the "the exact magical spot" referred to in #33. It's a way for the rendered block content to override the configured title (e.g. views blocks). It appears that the intent is to have the implementing block place #title to override the block title (or at least it's a change to accommodate how Views did block title overrides). Patch fixes the issue by avoiding the field's #title property from being in the magical spot by placing it in an array, nesting it a level down.

2942623 exists, but might be closed. Although it brings up an interesting point of why the title override comes after the block's configured label. You would expect that block configuration overrides whatever the block content supplier (be it Views or Field Block) supplies.

This leads into behavior change on existing sites because they might be relying on this behavior. There could be also information in the administrative labels that is not intended to be printed on the public site. Tagging for release manager review to get their feedback on how they'd prefer to handle this.

@lauriii asked me about this one in slack.

The issue summary could use an update, the original 'proposed solution' was a bit weird, the new proposed solution just changes how the $build array is structured and seems fine.

On the behaviour change I think this is fine in a minor release with a release note. There seem to be two possibilities where it's a surprise:

1. Someone tried to customize the title, it didn't work, they forgot about it and just dealt with it (or changed the actual field title), suddenly it starts working. This could happen but seems like we just fixed their bug. Worst case they have to update the layout configuration.

2. Someone's relying on having this extra text field to store 'stuff' (metadata or similar) and suddenly their stuff that wasn't intended for public consumption is shown on the site. I think this is extremely unlikely because the UI for field blocks is exactly the same as all other kinds of blocks, and it's just this one that doesn't work.

Remaining question is whether this goes into 10.2.x or 10.1.x, I don't have a strong opinion on that.

Forgot to untag.

Committed 7598b15 and pushed to 11.x. Thanks!

I don't necessarily see a lot of difference between committing this to 10.2.x and 10.1.x so I went ahead and cherry-picked this to 10.1.x.

This broke my site. No content on pages using field blocks.

A developer had written this:

    {% for item in content['#items'] | keys %}
      {{ content[item] }}
    {% endfor %}

That little changed test at the end of the patch, it was detecting this was going to break. :(

Its getting late on my Friday so I haven't dug in too deep into why the developer did this but wanted to raise this since the scope of the break might be bigger than captured.

Let's add a change record for this. It's tagged for release notes - I didn't check the release notes draft but I also can't see a snippet in the issue summary.

The render array change is allowed in a minor so it's a case where we'd expect custom code to adapt, but this went in to 10.1.x quite late so it's not leaving that much time. Did you already fix this for your site, and if so is it compatible with both the new and old way now? i.e. I wouldn't like to break this twice.

I added a paragraph directly to the release notes about the behavior change, copied that here too to reduce confusion. The snipped I added didn't mention the render array change. Should we mention the render array change in both, CR and release notes?

I normally wouldn't bother (unless it was a very commonly altered form or similar) but I think since we have an example of someone relying on it, it would be a good idea in this case. Not sure how to word it except something like 'The render array structure for field blocks now has an extra level of nesting.' - although maybe that would be enough?

Did you already fix this for your site, and if so is it compatible with both the new and old way now? i.e. I wouldn't like to break this twice.

Yes and yes but I can watch this space and deal with the fix either way.

Should we mention the render array change in both, CR and release notes?

I think so. I actually went to the change records but couldn't find any recent layout builder changes which made narrowing down the problem, fix, and this issue a lot harder.

Not sure how to word it except something like 'The render array structure for field blocks now has an extra level of nesting.' - although maybe that would be enough?

Yeah I think so.

The fix for me was a bit complicated because this seems to have been done to avoid the field wrapping divs. The trivial fix was just to output the individual items using the children filter but that added add the field wrappers which broke layout. To fully work around it afterwards actually required quite a bit of complexity or contrib. I went with contrib and field_value being provided by twig_field_value.

Before:

    {% for item in content['#items'] | keys %}
      {{ content[item] }}
    {% endfor %}

After:

    {% for item in content|children %}
      {{ item|field_value }}
    {% endfor %}

I created a CR but I'm not sure what would be the before and after with just core because we don't have |children filter in core. Also, shouldn't it take into account that there could be something to render on the first level of the render array?

Wow... that seems like an oversight on core's fault. That's super useful with render arrays.

I'm not sure how you'd do it either without implement Element::children inside the twig template which seems... not great.

That said, this can probably be closed since 10.1 is out and there's a changelog?

Brought this up with @lauriii in #needs-review-queue-initative and agreed it could be closed again. Well marked fixed to be auto closed later.

This has a change record, removing tag