Field blocks in the layout builder do not have view mode suggestions

Thanks for opening the issue! I think we can categorize this as a bug, as it was an oversight when writing the module.

Do you have a custom hook_theme_suggestions_alter() in a module or theme?
Because when I inspect the template suggestions I do not see one for the view mode at all:

<!-- FILE NAME SUGGESTIONS:
   * field--node--body--article.html.twig
   * field--node--body.html.twig
   * field--node--article.html.twig
   * field--body.html.twig
   x field--text-with-summary.html.twig
   * field.html.twig
-->

#3007973: Layout builder prevents the rendering of extra fields (like Links) on pages not using Layout Builder might be a duplicate of this issue, linking so we can check back.

I've applied the latest patch from the referenced related issue and I can confirm that this is still an issue.

Field blocks displayed in different view modes in layout builder all reference a `_custom` view mode inside the $vars['element']['#view_mode'] value. Attempting to create a custom hook suggestions for field_blocks per the host entities view mode(s), something like below inside a hook_suggestions_alter() produces the attached results.

For a use case example: I have a field_block_node_title being printed on the page twice with two different view modes. One for the current nodes title (full view mode). A secondary for the `related content` title (secondary view mode). At the moment there is no way to control a separate theming template, from the field or block level per content (entity) view mode.

function hook_theme_suggestions_alter(array &$suggestions, array $variables, $hook) {
  if ($variables['theme_hook_original'] === 'field') {
    $element = $variables['element'];
    $suggestions[] = $variables['theme_hook_original'] . '__' . $element['#entity_type'] . '__' . $element['#field_name'] . '__' . $element['#view_mode'];
  }
}

Still needs tests.
And a decision on whether to add this suggestion from within Layout Builder, or just stop short at making it possible to add.

As seen in the patch, this adds the correct information at

$element['#third_party_settings']['layout_builder']['view_mode']

I don't think it is possible to change the actual $element['#view_mode'] key. _custom is technically the correct value at that point, it's just that this is a nested rendering situation...

This issue needs at least two issues opened as blockers: one for the change to FormatterBase, one for ContextHandler.

Blocked by #3029627: FormatterBase should pass along third party settings and #3029625: Do not throw an exception when a context is missing while applying context mapping to a plugin if that context was previously set

Having the view mode suggestion in there seems like a pretty common use case. So I think this make sense to provide as a suggestion from layout builder, rather than just making it possible by placing it in #third_party_settings.

Fair point. This is still blocked by (and includes) those two issues (one RTBC, the other needs tests)

Postponed on #3029627: FormatterBase should pass along third party settings

Reroll against 8.7.x-dev. It looks like several things added in #13 were committed elsewhere.

Fixes patch paths.

#17 sort of works but there is an issue with actually using the suggestions. The reason is that the suggestions get added to the template suggestions before the system suggestions and will be placed at the bottom of all suggestions.

There are two solutions:
1) Change the module weight of layout_builder so the hook will be executed after the system module
2) Change the hook to hook_theme_suggestions_alter
3) Use hook_module_implements_alter to reorder the modules

I discovered another issue with adding the view mode as context. The module "layout_library" can create layouts without having a view mode as display context. Since we do not provide a default view mode when creating a field block and the context is required, this will break.

I went ahead and added changes from #18 and from this comment to patch #17.

Adding missing interdiff

I've give it a try (patch from #19 with interdiff from #21) and it is working for me, providing me with "field--node--title--page--teaser.html.twig" suggestion. That what I was looking after.

Still working fine after update to 8.7.1.

Can confirm, worked for me

Working on this

About this patch

1. Addressed test failures in earlier patches by updating LayoutEntityHelperTrait::getSectionStorageForEntity to return a view mode context for every condition. Without this, inline blocks added to layout defaults were not being saved as block content entities, only serialized in config.
2. View mode was appearing in the block add/config form. I opted to address this in the $form build, rather than making the view mode context required. When I tried the approach of making the view mode context required, FieldBlockTest would fail due to the deriver added in the layout_builder_fieldblock_test module, which in turn suggested making it required could potentially create problems for contrib modules. I think this will be a more easily managed solution unless there are specific reasons why the required-context approach should be used over doing it via $form.
3. Added a functional test to confirm that view mode specific templates will be used when present. It seems like there's already sufficient kernel/unit coverage, but not sure enough to remove the "Needs tests" tag.

Thanks for picking this up!

1. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
   @@ -100,7 +100,7 @@ public function view(FieldItemListInterface $items, $langcode = NULL) {
   -        '#third_party_settings' => $this->getThirdPartySettings(),
   +        '#third_party_settings' => $this->thirdPartySettings,
   

   I think this is an unintentional revert introduced by a bad reroll, may have been my fault

2. +++ b/core/modules/layout_builder/src/Context/LayoutBuilderContextTrait.php
   @@ -47,6 +49,13 @@ protected function getAvailableContexts(SectionStorageInterface $section_storage
   +    // In some instances, such as the layout_library module, view_mode is not
   +    // an available context, despite being required. If view_mode has no value,
   +    // set it to 'default'.
   +    if (!isset($contexts['view_mode']) || !$contexts['view_mode']->getContextValue()) {
   +      $contexts['view_mode'] = new Context(new ContextDefinition('string'), 'default');
   
   +++ b/core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php
   @@ -123,6 +124,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +            'view_mode' => new ContextDefinition('string', 'View Mode', FALSE, FALSE, NULL, 'default'),
   

   Why doesn't the default value passed here satisfy this?

   Also this should switch to (new ContextDefinition('string'))->setDefaultValue('default'), to sidestep all those other params

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -33,6 +34,7 @@
   + *     "view_mode" = @ContextDefinition("string"),
   

   Ah, maybe this is the part that needs to change. This can be @ContextDefinition("string", default_value = "default"),

4. +++ b/core/modules/layout_builder/tests/modules/layout_builder_field_block_theme_suggestions_test/layout_builder_field_block_theme_suggestions_test.module
   @@ -0,0 +1,17 @@
   +function layout_builder_field_block_theme_suggestions_test_theme() {
   +  return [
   +    'field__node__body__bundle_with_section_field__default' => [
   +      'base hook' => 'field',
   

   Not 100% clear to me why this is needed, don't the template suggestions handle this for you?

Along with the next patch, can you include a failing patch with just the functional test? Thanks!

#28.1 -- fixed
#28 2/3 setting the default value in the annotation for DefaultsSectionStorage removed the need to set a default value in the deriver. I also figured out a way to get FieldBlockTest to work while making the deriver's view_mode required. This was accomplished by adding a ContextProvider service to layout_builder_fieldblock_test
#4

Not 100% clear to me why this is needed, don't the template suggestions handle this for you?

. If the template was provided in a theme, it would automatically be discovered based on filename and directly placement. As far as I know, Drupal will not know to look in the templates directory of a module without an implementation of hook_theme(). There may be a more elegant way to do it, but it doesn't automatically check the contents of the templates dir like it would in a theme.

Awesome, thanks!

1. +++ b/core/modules/layout_builder/src/Context/LayoutBuilderContextTrait.php
   @@ -47,6 +49,13 @@ protected function getAvailableContexts(SectionStorageInterface $section_storage
   +    // In some instances, such as the layout_library module, view_mode is not
   +    // an available context, despite being required. If view_mode has no value,
   +    // set it to 'default'.
   +    if (!isset($contexts['view_mode']) || !$contexts['view_mode']->getContextValue()) {
   +      $contexts['view_mode'] = new Context(new ContextDefinition('string'), 'default');
   +    }
   

   Idk if I highlighted this hunk as well above, but this seems like another instance of passing `default` that seems like it's in the wrong place

2. +++ b/core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php
   @@ -123,6 +124,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +            'view_mode' => (new ContextDefinition('string')),
   

   If there's no method call on the new object, then you don't need the extra ()

3. +++ b/core/modules/layout_builder/tests/modules/layout_builder_field_block_theme_suggestions_test/layout_builder_field_block_theme_suggestions_test.module
   @@ -0,0 +1,17 @@
   +function layout_builder_field_block_theme_suggestions_test_theme() {
   

   Thanks for clarifying this, can you put an inline comment that says "this is only needed because it's a module, not a theme"?

#30.1

this seems like another instance of passing `default` that seems like it's in the wrong place

I moved this to what seems like a more appropriate place Drupal\layout_builder\Plugin\SectionStorage\SectionStorageBase::getContextsFromPreview() and revised the content to better explain what is being addressed. Interested to know if there's a better way to go about this - the problem this addresses can be reproduced by installing Layout Library and visiting an "Edit Layout" page. They Layout library SectionStorage has its own getContextsDuringPreview() method that does not return view_mode and there is not a default defined. Perhaps this is something that could be implemented with a deprecation path but I'm not entirely sure how that would be done.

#30.2 Unnecessary parentheses removed.

#30.3 added some clarification to the hook_theme() implementation

The test failure in #32 was due to OverridesSectionStorage having a view mode context without a default value (which surfaced due to adding a check for view mode in Drupal\layout_builder\Plugin\SectionStorage\SectionStorageBase::getContextsFromPreview()

Although this was addressed by updating OverridesSectionStorage to have a default value in the view mode context, I also added a validation condition inside the getContextsFromPreview() conditional that checks for a view mode context, so it does not break contrib modules with SectionStorage plugins that have view mode contexts without default values.

This patch doesn't seem to work for me. Here's what I get for the body field placed via Layout builder:

<!-- FILE NAME SUGGESTIONS:
   * field--node--body--page.html.twig
   * field--node--body.html.twig
   * field--node--page.html.twig
   x field--body.html.twig
   * field--text-with-summary.html.twig
   * field.html.twig
-->

When I try to add a suggestion for view_mode, like this:

function hook_theme_suggestions_field_alter(array &$suggestions, array $variables) {
  $element = $variables['element'];
  $field_name = $element['#field_name'];
  $view_mode = $element['#view_mode'];
  $suggestions[] = 'field__' . $field_name . '__' . $view_mode;
}

I get these suggestions:

<!-- FILE NAME SUGGESTIONS:
   * field--body--layout-builder-full-0-7fa9615d-fe2f-4f85-92d2-5e5743e0e5a8-799451-7bae016a755459cba0f1ba2743f6121012bc338e1840a40d8f1791bc3aac218c.html.twig
   * field--node--body--page.html.twig
   * field--node--body.html.twig
   * field--node--page.html.twig
   x field--body.html.twig
   * field--text-with-summary.html.twig
   * field.html.twig
-->

$element['#view_mode']; is not changed in this issue. It will function exactly that way with and without the path.

As shown in the patch, it is using $element['#third_party_settings']['layout_builder']['view_mode']

Curious that with the latest patch, layout_builder_theme_suggestions_field_alter() isn't handling it for you. It should be.

Apologies, I was not using the latest version of core. The template suggestion with view mode appears now without the need to use a hook. Thanks for all the work here!

<!-- FILE NAME SUGGESTIONS:
   * field--node--body--page--full.html.twig
   * field--node--body--page.html.twig
   * field--node--body.html.twig
   * field--node--page.html.twig
   x field--body.html.twig
   * field--text-with-summary.html.twig
   * field.html.twig
-->

If a themer adds suggestions for fields with view mode added without layout builder, there is still an extraneous entry for layout builder added fields. Maybe that could be unset?

When I try to add a suggestion with view_mode for fields added displayed without Layout Builder, like this:

function hook_theme_suggestions_field_alter(array &$suggestions, array $variables) {
  $element = $variables['element'];
  $field_name = $element['#field_name'];
  $view_mode = $element['#view_mode'];
  $suggestions[] = 'field__' . $field_name . '__' . $view_mode;
}

For a field added with Layout builder I then get:

<!-- FILE NAME SUGGESTIONS:
   * field--body--layout-builder-full-0-7fa9615d-fe2f-4f85-92d2-5e5743e0e5a8-799451-7bae016a755459cba0f1ba2743f6121012bc338e1840a40d8f1791bc3aac218c.html.twig
   * field--node--body--page--full.html.twig
   * field--node--body--page.html.twig
   * field--node--body.html.twig
   * field--node--page.html.twig
   x field--body.html.twig
   * field--text-with-summary.html.twig
   * field.html.twig
-->

That additional one with all the UUIDs is from the Quick Edit module integration. It can be ignored.

I only found 2 really small nitpicks that are related to the coding standards. Otherwise I don't see anything with this patch that could be improved.

1. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderFieldBlockThemeSuggestionsTest.php
   @@ -0,0 +1,68 @@
   +class LayoutBuilderFieldBlockThemeSuggestionsTest extends BrowserTestBase {
   +  /**
   

   neeeds addtional newline.

2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderFieldBlockThemeSuggestionsTest.php
   @@ -0,0 +1,68 @@
   +
   +  }
   

   I think this blank line can go. But I am not sure if we have a PHPCS rule for it.

Comments addressed in #39 & added an interdiff as well.

My nitpicks have been resolved, this looks good overall.

I concur that this is ready to go. Thanks everyone!

Hmm. It seems I must have misattributed this patch as the source of issues in #3063081.

Restoring status after recognizing my error.

I think that test failure is unrelated as it was in a media js-test.

issue credits

Committed 90534ac and pushed to 8.8.x. Thanks!

I'm of the opinion that this is not eligble for back-port to 8.7.x because it could have unintended consequences to templates/markup - see allowed changes - but its a grey area so I'll put it to a poll of other committers

I realise this has already gone into 8.8 - but could I request a posthumous change record, it would help with the decision around backporting.

This doesn't work on 8.7 because #3029627: FormatterBase should pass along third party settings wasn't backported. Reopened that one

Can confirm when using both the patches , https://www.drupal.org/files/issues/2019-06-17/3001313-41.patch and https://www.drupal.org/files/issues/2019-04-12/3029627-tps-10.patch works in 8.7x for generating the view mode suggestions !!

Can we get a change notice here, if this is to be backported, we need to let people know - thanks

For #54

Using the patch in #41 was still allowing failures of view_mode context using 8.7.x. This updates #41 to ensure a default value when getting derivative definitions.

Fixes line endings for files in #56.

SORRY - my stupid mistake, I didn't see this was already committed and you were just discussing the backport to 8.7.x!

Now where 8.8.0 is out - should we close this FIXED?

Agreed.

This fix is causing problems for users of the mini_layouts module - hopefully someone can offer some advice.

https://www.drupal.org/project/mini_layouts/issues/3110850
https://www.drupal.org/project/mini_layouts/issues/3112090

People often create mini layouts with a required context or two and then use FieldBlocks to render fields. These mini layouts can then be placed as a block anywhere else. This fix has caused all of those mini_layouts to throw fatal errors because the 'view_mode' context is not satisfied.

I can't work out what the appropriate way forward is - do mini layouts users have to define two required contexts for each entity they're using in their mini layout? That seems like an uncomfortable burden to place on the users. Can we make it so that, if there isn't a view_mode context, the default view mode (or even we revert to the 'custom' behaviour as before!) is used rather than throwing an error?

I know this patch will probably need a seperate issue to get committed, but I think this is probably all it needs to avoid breaking old mini_layouts. Just set the default value of the context definition to 'default'.