Layout builder doesn't show new "extra fields"

Hello "Anybody" and thanks you.

I confirm that it works by deleting in hidden.

To allow your field be visible on layout builder "add component" form, you have to set "visible = true" in definition.
Found the case when it is possible to successfully configure and export display config with layout builder enabled. Follow next steps:

1. Configure your entity fully before managing displays
2. Before enabling layout builder, add your filed to content area using default display manager
3. Enable layout builder and configure you display

Second step is important, this will add field configuration in content

content:
  extra_field_your_field:
    weight: -30
    region: content
    settings: {  }
    third_party_settings: {  }

and will remove this field from hidden list.

Difference between regular fields and extra_filed fields is regular fields add its own configuration to display (to content area) when it is added, but extra_field fields don't have such trigger. Here is 2 possible solution:

1. Add field configuration to content list during discovering of fileds
2. Watch layout builder form and manage it after fields added

Looks more like cache issue, visibility was fixed in #3020572: Layout builder block for extra fields should appear in "Content Fields" category and not "Content"

@andypost issue us not related to visibility of blocks you can add, it is about render of display

Follow this task for this issue #3069732: Problems with layout_builder and possibly this task can be closed i guess since problem is in extra_field module.

Yes, marking as a duplicate.
If someone can reproduce this without using that module (aka implementing hook_entity_extra_field_info() directly), please reopen with your code example.

Bug is present even using hook_entity_extra_field_info(). For example, le't asume we have following extra-field:

/**
 * Implements hook_entity_extra_field_info().
 */
function my_module_entity_extra_field_info() {
  $extra['node']['test']['display']['module_extra_field_test'] = array(
    'label' => t('Test module extra-field'),
    'description' => t('This is my own extra-field.'),
    'weight' => 100,
    'visible' => TRUE,
  );

  return $extra;
}

/**
 * Implements hook_node_view().
 */
function my_module_node_view(array &$build, \Drupal\Core\Entity\EntityInterface $entity, \Drupal\Core\Entity\Display\EntityViewDisplayInterface $display, $view_mode) {
  if ($entity->bundle() == 'test' && $display->getComponent('module_extra_field_test')) {
    $build['module_extra_field_test'] = array(
      '#type' => 'inline_template',
      '#template' => '<div class="extra-field-test"><h2>{{ text }}</h2></div>',
      '#context' => [
        'text' => 'This is an extra-field test.',
      ],
    );
  }
}

Without Layout builder, field works fine. It's displayed by default (keep note on property 'visible' => TRUE, at first hook).

Then, if I remove field from display, and I enable layout builder and I try to re-add field, it doesn't work. Field content is not rendered. We could to think it's needed to disable layout builder to append field to the "base" display and then, re-enable it again. But this is not desired behavior, because there could be sites with complex layouts. Also, keep in mind "visible" is already enabled into the definition.

Seems problem could reside into the core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php::getComponent method. It returns "NULL" even for some core fields (for example, the node "Links" field).

Again: Just wanted to hear opinion: Attached patch is basically to ilustrate some issues. It would be great to know, if approach is concerning this class.

Possibly, reroll will be needed

Against 9.1.x branch reroll is not needed

As patch fails

Working on this.

Notice: Undefined offset: 3 in Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay->preSave() (line 166 of core\modules\layout_builder\src\Entity\LayoutBuilderEntityViewDisplay.php).

list (, , , $field_name) = explode($plugin::DERIVATIVE_SEPARATOR, $plugin->getPluginId(), 4);

Fix this to

if($plugin instanceof ExtraFieldBlock){

            list (, , , $field_name) = explode($plugin::DERIVATIVE_SEPARATOR, $plugin->getPluginId(), 4);
            if(!isset($this->content[$field_name])){
              parent::setComponent($field_name);
            }
          }

to me working thanks

fix of #14 patch. wrong expression , see interdiff.

Thanks for patch #21, but issue from #20 still exists.

New to this issue here. I was working on #3144010: New pseudo-fields cannot be removed, InvalidArgumentException thrown and I think I solved this issue too. Can someone review this patch?

This patch modifies a class that does not exist in 9.0.x, so this will only apply on 9.1.x.

This patch does not work. See comment #27. Sorry!

Here's a patch that uses the same approach as the one in #23 but it's for 9.0.x and it should work in 8.9.x.

This patch does not work. See comment #27. Sorry!

NW for an issue summary update and for tests.

I have some other thoughts about the change and how best to document this code, but that should wait until after tests are written.

Actually, closing this one in favor of #3144010: New pseudo-fields cannot be removed, InvalidArgumentException thrown as the fixes are the same.

Turns out I was totally wrong about my patches in #23 and #24 solving this issue. It is not a duplicate of #3144010: New pseudo-fields cannot be removed, InvalidArgumentException thrown.

Please see comment from @sorlov on the other issue: https://www.drupal.org/project/drupal/issues/3144010#comment-13825300

Sorry about the confusion. Re-opening this issue. I plan to post a fail test soon.

Here's a FAIL patch that I think exposes the issue cleanly.

Here's the patch from #21 tested against the FAIL patch of #28. The test passes locally, which is good news.

There's a comment in #20 that may or may not be resolved yet.

UPDATE:
I'm seeing failed tests with this message:

Notice: Undefined offset: 3
Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay->preSave()() (Line: 166)

I think that's the same thing brought up in #20.

I incorporated the suggestion from #20 (from @Regnoy) into this latest patch. See interdiff and comment #20.

@danflanagan8 ah, okay that makes more sense :)

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -157,6 +158,20 @@ public function preSave(EntityStorageInterface $storage) {
   +          if ($plugin instanceof ExtraFieldBlock) {
   
   @@ -479,6 +494,9 @@ public function getComponent($name) {
   +        if ($plugin->getBaseId() == 'extra_field_block') {
   

   I think the base ID should be checked in both cases.

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -157,6 +158,20 @@ public function preSave(EntityStorageInterface $storage) {
   +              parent::setComponent($field_name);
   

   I've never seen parent:: calls to _other methods_ before. Why not call $this->setComponent?

   Additionally, why not do this in
   an override of ::init()?

   Or if it's possible, from within setComponent directly?

   Doing this on every preSave() doesn't feel right.

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -479,6 +494,9 @@ public function getComponent($name) {
              return $configuration['formatter'];
   ...
   +          return $configuration;
   

   Can we possibly change ExtraFieldBlock to not need this special casing?

4. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -535,7 +553,7 @@ private function getSectionComponentForFieldName($field_name) {
   +        if ($plugin instanceof DerivativeInspectionInterface && ($plugin->getBaseId() === 'field_block' || $plugin->getBaseId() === 'extra_field_block')) {
   

   Nit: This could use in_array()

5. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/layout_builder_test.module
   @@ -47,6 +47,12 @@ function layout_builder_test_entity_extra_field_info() {
   +    'label' => t('Pseudofield 2'),
   +    'description' => t('Pseudofield description'),
   
   @@ -59,6 +65,11 @@ function layout_builder_test_node_view(array &$build, EntityInterface $entity, E
   +      '#markup' => 'Pseudofield 2 is hidden by default.',
   

   Idk where "pseudofield" comes from, let's call it an "extra field" like it is in the code base.

Idk where "pseudofield" comes from

That's so funny, @tim.plunkett! I learned these as "pseudofields" a few years ago and I toss it around as if it's the standard word. In the docblock for hook_entity_extra_field_info() it reads:

Exposes "pseudo-field" components on content entities.

Even core is a little bit confused on the matter! Anyway, changing the wording from "Pseudofield 2" to "Extra field 2" would be no problem.

As for your other comments on the patch, I haven't looked at the functional code in the patch too closely and I don't really understand what's going on. Other than the test part of the patch, I was mindlessly compiling previously posted patches and comments.

Maybe someone who worked on the previous patches could take a look?

I took some time to look into this and have a patch with a somewhat different approach than the other patches. I took @tim.plunkett's idea of looking into overriding the init() function.

Seems to be a lot of duplication of the parent implementation. Is it possible to still call parent::init() and add in just the LB specific code?

Yeah, that would be a good idea. Let's see if all the tests pass. If they do I'll post an updated patch with less duplicated code.

Two failed tests. The one that's in Layout Builder is an easy fix, but I think the failed test in Umami indicates my approach is no good. The new init() function moves the links extra field from hidden to content (like it does with all extra fields), causing the test on umami config to fail.

I kind of liked overriding since it didn't require changes to any other functions, but I think now that the best approach will be to make changes to the getComponent() function, as was done in the patches prior to #33.

Here's yet another attempt. This patch is a lot like the one in #30 but with the comments of #31 taken into account. This patch does not make any changes to preSave(). The changes to getComponent() and getSectionComponentForFieldName() are hopefully just a little bit clearer. I added a helper function to EntityDisplayBase in an attempt to keep things as clean as possible.

I started feeling like #37 contained some overkill. This new patch is basically the patch from #12 plus the test. I think @waspper had it right from the start.

Also, I never addressed this comment:

Can we possibly change ExtraFieldBlock to not need this special casing?

I think the only way to avoid special casing in getComponent() is to add something like 'formatter' => TRUE as default configuration for extra field blocks. That's pretty hacky though.

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -479,6 +479,10 @@ public function getComponent($name) {
   +          return TRUE;
   

   We can't return a boolean here. The valid return types are an array or NULL. I think adding ['formatter' => []] to ExtraFieldBlock is a pretty good idea (instead of TRUE)

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -535,7 +539,7 @@ private function getSectionComponentForFieldName($field_name) {
   +        if ($plugin instanceof DerivativeInspectionInterface && in_array($plugin->getBaseId(),['field_block', 'extra_field_block'])) {
   

   Missing space between , and [

   Also please add TRUE as the 3rd param to in_array

@tim.plunkett

I think adding ['formatter' => []] to ExtraFieldBlock is a pretty good idea (instead of TRUE)

I'm not sure that and empty array would work because we typically see expressions like the following in hook_node_view to handle extra fields:

function layout_builder_test_node_view(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display, $view_mode) {
  if ($display->getComponent('layout_builder_test')) {
     // Do something
  }
}

If 'formatter' is set to an empty array, which is falsey, the Do something won't happen. Is there something truthy we could pick that isn't as hacky-looking as TRUE?

Let's try a new patch where we set 'formatter' in the extra field block to some essentially empty but not falsey array. I wouldn't be surprised to see some failed tests resulting from this change. We'll see.

This is looking really good!
Couple points of feedback.

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -535,7 +535,7 @@ private function getSectionComponentForFieldName($field_name) {
   +        if ($plugin instanceof DerivativeInspectionInterface && in_array($plugin->getBaseId(),['field_block', 'extra_field_block'], TRUE)) {
   

   Still missing a space after the comma here

2. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -85,6 +85,12 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +      'formatter' => [
   +        'label' => NULL,
   +        'type' => NULL,
   +        'settings' => [],
   +        'third_party_settings' => [],
   +      ],
   

   According to EntityDisplayBase::setComponent() this is the correct fix, but only for settings/third_party_settings. Leave out label/type

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -887,6 +887,24 @@ public function testExtraFields() {
   +    // Consider extra field that is hidden by default. See Issue #3069578.
   +    // First make sure it's not displayed.
   

   Combine these two comments, and leave out the issue #.

4. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -887,6 +887,24 @@ public function testExtraFields() {
   +    // View the layout and try to add the extra field that is not visible by default.
   

   Remove "try to" from this sentence

5. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -887,6 +887,24 @@ public function testExtraFields() {
   +    $this->drupalGet("admin/structure/types/manage/bundle_with_section_field/display/default/layout");
   

   Nit: swap those " for '

Made the changes requested in #44. Please review.

Looks like #46 addresses all the concerns from #44. I'll set this RTBC.

In general, you shouldn't RTBC an issue you wrote patches for.
In this case, I was about to RTBC anyway, so it's not a problem.

Thanks for your work on this!

Right, I guess I should recuse myself in such situations!

Drupal\Tests\ckeditor\FunctionalJavascript\CKEditorIntegrationTest is an unrelated random failure. Resetting to RTBC.

1. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -85,6 +85,10 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +      'formatter' => [
   +        'settings' => [],
   +        'third_party_settings' => [],
   +      ],
   

   Should there be a config-schema change to go with this - block default configuration normally maps 1:1 to a schema entry

2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -887,6 +887,23 @@ public function testExtraFields() {
   +    // Consider extra field that is hidden by default, make sure it's not displayed.
   

   nit: more than 80 chars here

Ah yes, block.settings.field_block:*:*:* should be duplicated exact as-is but as block.settings.extra_field_block:*:*:*

@danflanagan8, can you do that so that I can reRTBC?

There ya go. I also shortened the comment called out in #52.

Thanks! I think that suffices.

Committed b94f170 and pushed to 9.1.x. Thanks!

+1 for a backport of this to 8.x. Without this patch the Toc.js node field does not render.

The #54 patch applied cleanly and fixes the issue with 8.9.10 so I don't think a backport will need any modification to the patch.

This is still here in D9.2. When I add a field to a content type that has been configured in layout mode, the field does not appear in the layout. As above, if you get out of layout mode and go to manage fields, the field is there but disabled. If you move it to enabled and reenable layout mode it now appears, but you have lost all the layout customization in the process.

Hi @yktdan
I'm confident that this issue is resolved, but the issue summary is not very clear. This issue is specifically about "extra fields" that are not defined with 'visible' => TRUE in hook_extra_field_info. These field were previously not renderable by LB until they were moved out of the "hidden" region in Field UI.

Is this the problem you are having?

It looks to me like you are dealing with something that may be unexpected and annoying but not a bug. If you have a node with Layout overrides (like it's customized beyond the default layout), then it no longer picks up new changes that are made to the default layout. Is that what you're running into?

Yes, I think that describes my issue.