No ability to control "extra fields" with Layout Builder

Here a start with basically just the deriver.

Figure out a way to render them (since they are usually rendered in hook_entity_view_alter())

Still trying figure this out but it seems tricky.

The node module seems to add its extra links field in \Drupal\node\NodeViewBuilder::buildComponents() but yes others add in hook_entity_view_alter

I tried getting the entity view builder.

    $view_builder = $this->entityTypeManager->getViewBuilder($this->entityTypeId);
    $build[$entity->id() ?: 0] = $view_builder->view($entity);
    $build = $view_builder->buildComponents($build, [$entity], [$this->bundle => $view_display], 'default');
    return isset($build[$this->fieldName]) ? $build[$this->fieldName] : [];

But this causes recursion because the entity view is already being built at this point.

To avoid the recursion I did the hacky

// @todo How do we avoid building of sections here that will cause recursion.
$entity->get('layout_builder__layout')->setValue([]);

Not sure how to avoid this.

I also implementing hook_entity_view_alter otherwise the other modules implementation will render the extra fields too.

@todo Either make sure our implementation of hook_entity_view_alter runs last or just add #preRender callback in hook_entity_view_alter which will remove extra fields from render.

Ok after talking with @tim.plunkett here is much simpler implementation.

Now \Drupal\layout_builder\Plugin\Block\ExtraFieldBlock::build simply adds a placeholder and then in layout_builder_entity_view_alter the placeholder is replaced by the built extra field. Then the extra field in the main render array is unset.

@todo We should probably remove the extra field has no output. Like the node links for a logged in user.

This patch does:

1. Updates \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::setComponent BC logic to handle extra fields. This allows extra fields that were configured in 'field_layout' to work in layout builder.
2. Adds placeholder text so that in the layout builder placeholder text will show for extra fields. hook_builder_entity_view_alter is not fired in the builder so the actual extra fields will not be generated.
3. Change the default configuration to not show the block title.

Now with tests!

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -295,6 +295,20 @@ public function setComponent($name, array $options = []) {
   +  protected function init() {
   +    parent::init();
   +    // Call ::setComponent() on extra fields to ensure they are set properly.
   +    $extra_fields = $this->entityFieldManager->getExtraFields($this->targetEntityType, $this->bundle);
   +    foreach ($extra_fields['display'] as $name => $definition) {
   +      if ($component = $this->getComponent($name)) {
   +        $this->setComponent($name, $component);
   +      }
   +    }
   +  }
   

   \Drupal\Core\Entity\EntityDisplayBase::init is calling setComponent for all field definition components but not for the extra fields so the BC logic wasn't being handled for extra fields.

2. +++ b/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
   @@ -63,11 +63,18 @@ public static function create(ContainerInterface $container, $base_plugin_id) {
   -      if (!in_array(FieldableEntityInterface::class, class_implements($entity_type->getClass())) || !$entity_type->getBundleEntityType()) {
   +      if (!in_array(FieldableEntityInterface::class, class_implements($entity_type->getClass()))) {
            continue;
          }
   -      $bundle_type = $this->entityTypeManager->getStorage($entity_type->getBundleEntityType());
   -      foreach ($bundle_type->loadMultiple() as $bundle_id => $bundle) {
   +      if ($entity_type->getBundleEntityType()) {
   +        $bundle_type = $this->entityTypeManager->getStorage($entity_type->getBundleEntityType());
   +        $bundle_ids = array_keys($bundle_type->loadMultiple());
   +      }
   +      else {
   +        $bundle_ids = [$entity_type_id];
   +      }
   +
   +      foreach ($bundle_ids as $bundle_id) {
   

   Tests were breaking for because of user extra fields because user has no bundle type.

   This handles fieldable entity types that don't have bundles.

3. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/layout_builder_test.module
   @@ -0,0 +1,25 @@
   +function layout_builder_test_node_links_alter(array &$links, NodeInterface $node, array &$context) {
   +  // Create test node link that will always render.
   +  $links['layout_builder_test'] = [
   

   The regular not links don't show up usually so to make testing easier I added a test link that will always render.

Instead of using the node links I changed the test module to just have its own extra field.

This doesn't fix the cache test failures. Not sure on those yet.

Ok think I have track down the problem to the fact that the 'member_for' extra field being displayed on the page.

I have commented out this extra field and I think the layout builder tests will pass.

Of course this cause the user tests will fail

Ok think I have track down the problem to the fact that the 'member_for' extra field being displayed on the page.

This should be it. I tried the patch to remove "Member for" but it didn't disappear.

@mglaman I have tried this no existing site.

The patch in #13 was just to prove that test failures for layout builder are related the member_for extra field from the user field.

Before this issue the layout_builder module moves all of the regular fields to the new display.
This issue tries to also move the extra fields. I think for the tests the concerns are links and 'member_for' because node and user are enabled.

+++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
@@ -102,6 +104,8 @@ public function testLayoutBuilderUi() {
+    $assert_session->pageTextContains('Extra, Extra read all about it.');
+    $assert_session->pageTextNotContains('Placeholder for the "Extra label" field');

The changes to this test method prove that extra field on nodes are actually being rendered.

I think is safe to assume that the "member_for" extra field from user is also being rendered. Because removing it causes the test to fail.

For me the problem was the ::init() call. It was running over and over, and shouldn't have needed to.
Instead I added a post_update hook, and test coverage for it.

Additionally I cleaned up some other things I noticed in the patch, no runtime changes.

EDIT: The interdiff is from #11

In #17 \Drupal\Tests\layout_builder\Functional\LayoutBuilderTest::testLayoutBuilderUi fails because it only fixes the extra fields for existing EntityDisplay entities. In the test a new bundle is created after layout builder is installed.

I have manually tested this and it works for bundles that exist at the time that Layout Builder is installed but not for 1 created after.

Will self review in next comment

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -24,36 +24,3 @@ function layout_builder_post_update_rebuild_plugin_dependencies(&$sandbox = NULL
   -
   -/**
   - * Ensure all extra fields are properly stored on entity view displays.
   - */
   -function layout_builder_post_update_add_extra_fields(&$sandbox = NULL) {
   

   Removed this update function because it only solves it for displays that are present on module install.

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -295,6 +295,27 @@ public function setComponent($name, array $options = []) {
   +  protected function init() {
   +    parent::init();
   +    // Only set extra field components if id is empty. ::isNew() cannot be used
   +    // here because ::enforceIsNew() has not been called yet in
   +    // in \Drupal\Core\Entity\EntityStorageBase::create() for new entities.
   +    if (empty($this->id)) {
   

   Make sure this logic only happens for new displays. Added extra testing to make sure this happens.

3. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -119,7 +119,6 @@ public function build() {
   -    CacheableMetadata::createFromObject($this)->applyTo($build);
   

   Not positive about this. Wonder if removing this would cause cache not be cleared if label is on block config is changed?

4. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -142,7 +141,7 @@ public static function replaceFieldPlaceholder(array &$build, array $built_field
   -        $merged_cache->applyTo($build[$child]);
   +        $merged_cache->applyTo($build);
   

   This is the fix that actually fixes \Drupal\Tests\layout_builder\Functional\LayoutSectionTest::testLayoutSectionFormatter
   I think the problem was that the cache was applied to $block['content'] applying it to $block fixes the cache problems.

5. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -79,9 +79,26 @@ public function testLayoutBuilderUi() {
   +    // The body field is only present once.
   +    $assert_session->elementsCount('css', '.field--name-body', 1);
   +    // The extra field is only present once.
   +    $this->assertTextAppearsOnce('Placeholder for the "Extra label" field');
   +    // Save the defaults.
   +    $assert_session->linkExists('Save Layout');
   +    $this->clickLink('Save Layout');
   +    $assert_session->addressEquals("$field_ui_prefix/display/default");
   +
   +    // Load the default layouts again after saving to confirms fields are only
   +    // added on new layouts.
   +    $this->drupalGet("$field_ui_prefix/display/default");
   +    $assert_session->linkExists('Manage layout');
   +    $this->clickLink('Manage layout');
   +    $assert_session->addressEquals("$field_ui_prefix/display-layout/default");
   +    // The body field is only present once.
   +    $assert_session->elementsCount('css', '.field--name-body', 1);
   +    // The extra field is only present once.
   +    $this->assertTextAppearsOnce('Placeholder for the "Extra label" field');
   +
   

   I noticed in manual testing that extra field was being added again every time the display was saved. While this test passed in #11 it was checking this.

   The change to \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::init in this patch fixes this.

#20 failed on \Drupal\Tests\layout_builder\Functional\Update\ExtraFieldUpdatePathTest

Added back layout_builder_post_update_add_extra_fields() that @tim.plunkett added in #17 to handle existing displays.

I found the bug that was necessitating that init() addition.
#2925657: EntityDisplayBase::init() should use ::setComponent() for fields only fixed init() for full fields and ignored extra fields.
This change allows us to not need the custom handling.

Also, I restored the CacheableMetadata::createFromObject($this)->applyTo($build); to match FieldBlock.
The true bug was fixed by the interdiff in #20:

+++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
@@ -142,7 +141,7 @@ public static function replaceFieldPlaceholder(array &$build, array $built_field
-        $merged_cache->applyTo($build[$child]);
+        $merged_cache->applyTo($build);

Also fixing an indentation error added in #22.

Blocked by #2956202: EntityDisplayBase::init() should use ::setComponent() for extra fields as that fix is not directly within LB.

Postponed on the above fix.

Thought that'd be a faster way to get this in. Whoops!
The blocker was committed to 8.6

Huh. Interesting. label_display is a string, thanks to \Drupal\Core\Block\BlockPluginInterface::BLOCK_LABEL_VISIBLE.
But not consistently within core! Only in tests that trigger a schema check.

Bundles are not always config entities. Using the service to retrieve them.

Rolled against a stale local 8.6, whoops.

This looks great and has unblocked using embedded node teasers with Layout Builder. My one nit was about removing layout_builder_entity_view_alter and having LayoutEntityDisplay do all the logic. But, as tim.plunkett pointed out, Core itself is invoking that alter and Core is attaching extra fields through it.

Added screenshots

Thanks @tim.plunkett for the screenshots.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,42 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +/**
   + * Implements hook_entity_view_alter().
   + */
   +function layout_builder_entity_view_alter(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display) {
   +  if ($display instanceof LayoutEntityDisplayInterface) {
   +    /** @var \Drupal\Core\Entity\EntityFieldManagerInterface $field_manager */
   +    $field_manager = \Drupal::service('entity_field.manager');
   +    $extra_fields = $field_manager->getExtraFields($entity->getEntityTypeId(), $entity->bundle());
   +    if (!empty($extra_fields['display'])) {
   +      foreach ($extra_fields['display'] as $field_name => $extra_field) {
   +        if (isset($build[$field_name])) {
   +          $replacement = $build[$field_name];
   +        }
   +        else {
   +          // If the extra field is not set replace with an empty array to avoid
   +          // the placeholder text from being rendered.
   +          $replacement = [];
   +        }
   +        ExtraFieldBlock::replaceFieldPlaceholder($build, $replacement, $field_name);
   +        unset($build[$field_name]);
   +      }
   +    }
   +  }
   +}
   +
   

   It'd probably be good for the hook implementation here to some have some documentation of "what/why".

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,42 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +function layout_builder_module_implements_alter(&$implementations, $hook) {
   +  if ($hook == 'entity_view_alter') {
   +    // Ensure that this module's implementation of hook_entity_view_alter runs
   +    // last so that other modules that use this hook to render extra fields will
   +    // run before it.
   +    $group = $implementations['layout_builder'];
   +    unset($implementations['layout_builder']);
   +    $implementations['layout_builder'] = $group;
   +  }
   

   Hmm, this seems brittle. Why do we get to assume we get to be the last one? What happens if another module does the same thing? Content Translation also has a hook implementation that does essentially this, so it seems kind of like a "I get to run last" cold war, or a semi-deterministic/unpredictable situation in which the module that implement-alters last to be last is the one that actually gets to be last.

   What happens with both LB and CT? (I'm making an assumption here that we want content layout stuff to be translatable.) At the least, I think we should have a test that uses this together with content translation to explicitly document what we expect to happen in that case.

   CT is in fact the only other hook_module_implements_alter() in core and we only went that route because CT is "special". :) So that's another thing that raises questions for me.

   Meanwhile, a nit: hook_entity_view_alter() needs parens.

3. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -0,0 +1,161 @@
   + * Provides a block that renders a field from an entity.
   

   Don't we already have a different block that does that? This one is specifically for the extra fields, right? It'd be good to @see the normal field block and mention its relationship to this.

   I think it's also worth directly saying that this is for extra fields as provided by hook_entity_extra_field_info().

4. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -0,0 +1,161 @@
   +    // Add placeholder to replace after entity view is built.
   

   Nit: This sentence should have articles (a placeholder, the entity view).

5. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -0,0 +1,161 @@
   +      '#__extra_field_place_holder_field_name' => $this->fieldName,
   

   Whoa. I've never seen the underscore-prefixed render array build keys before. Do we have other examples of this?

   Also, nitpick: I think placeholder should be one word (not place_holder).

6. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -0,0 +1,161 @@
   +        self::replaceFieldPlaceholder($build[$child], $built_field, $field_name);
   

   Normally we use static:: and not self::; is there a particular reason we're using self:: here?

7. +++ b/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
   @@ -0,0 +1,114 @@
   + * @internal
   

   Am I internal because LB is experimental or because other reasons? Would be good to document here one way or the other. (We should do that with any @internal, really.)

8. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -389,4 +416,14 @@ public function testDeletedView() {
   +  /**
   +   * Asserts that a text string only appears once on the page.
   +   *
   +   * @param string $needle
   +   *   The string to look for.
   +   */
   +  protected function assertTextAppearsOnce($needle) {
   +    $this->assertEquals(1, substr_count($this->getSession()->getPage()->getContent(), $needle), "'$needle' only appears once on the page.");
   +  }
   +
   

   Huh, I feel like I've either used or written a method much like this recently. Might be worth a followup to actually add this to the BTB API. (Not relevant/blocking here, of course; just a thought.)

9. +++ b/core/modules/layout_builder/tests/src/Functional/Update/ExtraFieldUpdatePathTest.php
   @@ -0,0 +1,40 @@
   + * Tests the upgrade path for Layout Builder extra fields.
   + *
   + * @group layout_builder
   + */
   +class ExtraFieldUpdatePathTest extends UpdatePathTestBase {
   

   An update test even. Nice work! :)

10. It's definitely good to have a generic test for extra fields that's decoupled from content moderation, but I wonder if we should have a test that specifically tests with content moderation (so we can make sure that doesn't regress if content moderation changes)? I'm on the fence about whether that's a good idea or not but thought I'd ask for a second opinion. :)

@xjm thanks for the review.
1. Added what/way.
2.

Hmm, this seems brittle. Why do we get to assume we get to be the last one? What happens if another module does the same thing?

This is hard to get around because modules are suppose to add the rendered output for extra fields in hook_ entity_view_alter. But that also means that we would only affected if another module did the same thing if they were also using hook_ entity_view_alter to add extra fields. There would many, most uses of the hook_ entity_view_alter that would not matter if they ran after LB's version.

What happens with both LB and CT? (I'm making an assumption here that we want content layout stuff to be translatable.)

Even though LB and CT both implement hook_module_implements_alter they react moving implementations for different hooks
LB, entity_view_alter and CT entity_type_alter, entity_bundle_info_alter. so they would have no effect on each other.

Looking at \Drupal\Core\Extension\ModuleHandler::buildImplementationInfo() This hook is called once for each hook with their own list of $implementations so these are totally separate arrays.

fixed nit
3. fixed.
4. fixed
5. I don't think we need this. Fixed.
6. fixed
7.
Many class in LB have

 * @internal
 *   Layout Builder is currently experimental and should only be leveraged by
 *   experimental modules and development releases of contributed modules.
 *   See https://www.drupal.org/core/experimental for more information.

I don't see anything policy about derivers here https://www.drupal.org/core/d8-bc-policy

I added in

 * @internal
 *   Plugin classes are internal.

Though I don't see other classes in core specify why they are internal, so do we really need this.

8. This does seem familiar but I can't find it.

10. not sure

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   + * Modules that implement 'hook_entity_extra_field_info' use their
   

   I don't think hook names should be quoted; this should just say hook_entity_extra_field_info(), same with all other functions mentioned in this comment.

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   + *  implementations of 'hook_entity_view_alter' to add the rendered output of
   

   Supernit: 'implementations' is indented one space too far.

3. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   + * the extra fields they provide so we cannot get the rendered output of extra
   

   Can we have a comma after "provide"?

4. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   + * fields before this in the view process.
   

   Can we say "before this point in..."?

5. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   + * 'hook_entity_view_alter' to the last of the list.
   

   "to the end of the list"

6. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +    $field_manager = \Drupal::service('entity_field.manager');
   +    $extra_fields = $field_manager->getExtraFields($entity->getEntityTypeId(), $entity->bundle());
   

   There is no need to keep $field_manager in its own variable.

7. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +      foreach ($extra_fields['display'] as $field_name => $extra_field) {
   

   $extra_field is never used, so let's just loop over array_keys($extra_fields['display']) instead.

8. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +        if (isset($build[$field_name])) {
   +          $replacement = $build[$field_name];
   +        }
   +        else {
   +          // If the extra field is not set replace with an empty array to avoid
   +          // the placeholder text from being rendered.
   +          $replacement = [];
   +        }
   

   This can be a single ternary line.

9. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +function layout_builder_module_implements_alter(&$implementations, $hook) {
   +  if ($hook == 'entity_view_alter') {
   +    // Ensure that this module's implementation of 'hook_entity_view_alter' runs
   +    // last so that other modules that use this hook to render extra fields will
   +    // run before it.
   +    $group = $implementations['layout_builder'];
   +    unset($implementations['layout_builder']);
   +    $implementations['layout_builder'] = $group;
   +  }
   +}
   

   Ideally, we should have rock-solid, high-level test coverage of this entire thing. We should be damn sure that our view hook is being properly invoked for the "variants" of hook_entity_view_alter(), e.g. hook_node_view_alter(). I say this because I've been personally burned by the mind-bending logic of ModuleHandler::buildImplementationInfo(), which does a rather strange dance for hooks with varying names.

10. +++ b/core/modules/layout_builder/layout_builder.post_update.php
    @@ -24,3 +27,23 @@ function layout_builder_post_update_rebuild_plugin_dependencies(&$sandbox = NULL
    +function layout_builder_post_update_add_extra_fields(&$sandbox = NULL) {
    +  $entity_field_manager = \Drupal::service('entity_field.manager');
    +  \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'entity_view_display', function (EntityViewDisplayInterface $display) use ($entity_field_manager) {
    +    $extra_fields = $entity_field_manager->getExtraFields($display->getTargetEntityTypeId(), $display->getTargetBundle());
    +    $components = $display->getComponents();
    +    uasort($components, 'Drupal\Component\Utility\SortArray::sortByWeightElement');
    +    $result = FALSE;
    +    foreach ($components as $name => $component) {
    +      if (isset($extra_fields['display'][$name])) {
    +        $display->setComponent($name, $component);
    +        $result = TRUE;
    +      }
    +    }
    +    return $result;
    +  });
    +}
    

    This could benefit from some inline comments explaining what it's doing and why.

11. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
    @@ -257,12 +273,18 @@ public function setComponent($name, array $options = []) {
    +        $keys = array_flip(['type', 'label', 'settings', 'third_party_settings']);
    +        $configuration['formatter'] = array_intersect_key($options, $keys);
    

    I know it already existed, but this could use a comment.

12. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
    @@ -0,0 +1,171 @@
    + * This block handles fields that are provided by implementations of
    + * 'hook_entity_extra_field_info'
    

    The function name should have () after it, and not be quoted.

13. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
    @@ -0,0 +1,171 @@
    +    list (, , , $field_name) = explode(static::DERIVATIVE_SEPARATOR, $plugin_id, 4);
    +    $this->fieldName = $field_name;
    

    Maybe throw an \InvalidArgumentException() here if $field_name is empty?

14. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
    @@ -0,0 +1,171 @@
    +      '#markup' => new TranslatableMarkup('Placeholder for the "@field" field', ['@field' => $extra_fields['display'][$this->fieldName]['label']]),
    

    Does this need to be translatable?

15. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
    @@ -0,0 +1,171 @@
    +   * Replace all placeholders for a given field.
    

    "Replaces"

16. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
    @@ -0,0 +1,171 @@
    +   * @param array $build
    +   *   Built elements.
    

    This description should probably say something like "The built render array for the elements" or similar.

17. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
    @@ -0,0 +1,171 @@
    +   * @param array $built_field
    +   *   The built field to replace the placeholder.
    

    If this is a render array, the description should say so.

18. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
    @@ -0,0 +1,171 @@
    +      if (isset($build[$child]['#extra_field_placeholder_field_name']) && $build[$child]['#extra_field_placeholder_field_name'] == $field_name) {
    

    ===

19. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
    @@ -0,0 +1,171 @@
    +    $entity = $this->getEntity();
    +    return $entity->access('view', $account, TRUE);
    

    Nit: This can be condensed to one line.

20. +++ b/core/modules/layout_builder/tests/fixtures/update/layout-builder.php
    @@ -0,0 +1,31 @@
    +$extensions['module']['layout_test'] = 0;
    

    Should this be here?

Great "cooking" here! 1+ Did some "kitchen-cleaning" along #38. :)

With reference to the last #slack Layout Builder Initiative meeting on 26th of June with @timplunkett and @tedbow the points 1, 2, 3, 4, 5, 7, 12, 14, 15, 16, 17 made by @phenaproxima have been addressed in my "cleaning". Points not listed above need some more discussion. No time to be creative at the moment over here, so just some notes:

• 6) There is no need to keep $field_manager in its own variable.: Sorry but the phrasing is a little bit confusing for a non native tongue. Not sure if it refers to "we do not need a variable declaration here before X->Y" or to "we should empty the variable afterwards ..."
• 7) Done. But shouldn't this also be reflected in if (!empty($extra_fields['display'])) { right before?
• 8) A single ternary line? How about the comment then on the second step?
• 9, 11, 13, 19, 20) Needs more explanation maybe. Sorry, not tonight ... I am awake about 21 hours now.
• 10) I think this is rather for @tedbow.
• 12) Done. Additionally added same for next line and missing period.
• 17) From what I see line 140 says so (?)

$PSDefaultParameterValues['Out-File:Encoding'] = 'utf8' seems not to be enough in my git diff > encoding issue of the last days. It's utf8 now, but still with BOM encoding ...

Note for myself: Always check encoding again before uploading.

@diqidoq Could you provide a interdiff from patch in #37?

The patches since #37 are missing the entire plugin, its deriver, and the test coverage.

That.was.strange.

OK, sorry folks. My dev enviroment was messed up here. Re-roll, but let me test step by step: I will additionally leave out 7) yet (revert it from #39) and will come back to it later.

So: 1-5 and 12(++), 14, 15, 16, 17, 18 of review #38 are addressed in this first step. And this time with an interdiff! :)

Fixed the double translation, and the rest of the function references in docblocks.

Looks good! The logic is bit dense in places, but that's par for the course when dealing with many entity types and fields.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   + * layout_builder_module_implements_alter moves this implementation of
   

   Need () after layout_builder_module_implements_alter.

2. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -0,0 +1,171 @@
   +    $entity = $this->getEntity();
   +    return $entity->access('view', $account, TRUE);
   

   Nit: This can be a single line.

3. +++ b/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
   @@ -0,0 +1,117 @@
   +          $derivative['category'] = $this->t('@entity', ['@entity' => $entity_type->getLabel()]);
   

   Maybe this should use $entity_type->getPluralLabel() instead?

4. +++ b/core/modules/layout_builder/tests/src/Functional/Update/ExtraFieldUpdatePathTest.php
   @@ -0,0 +1,40 @@
   +    $this->assertEquals('extra_field_block:node:article:links', $component->getPluginId());
   

   Should be assertSame().

3) needs to stay, as it is the category used for all the other fields

Fixed the rest.

Manually reviewed the fixes mentioned in #50.
2953656-extra_field-52.patch looks good to me. +1 to RTBC

Hey, it's a patented phenaproxima Nitpick Review(tm)!

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +    if (!empty($extra_fields['display'])) {
   

   We don't need the !empty() check. From my reading of EntityFieldManager::getExtraFields(), if I'm interpreting it correctly, $extra_fields['display'] will be defaulted to an empty array, so we can just foreach over that and remove a level of nesting from this function.

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +        if (isset($build[$field_name])) {
   +          $replacement = $build[$field_name];
   +        }
   +        else {
   +          // If the extra field is not set replace with an empty array to avoid
   +          // the placeholder text from being rendered.
   +          $replacement = [];
   +        }
   

   This can be changed to ternary syntax.

3. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,54 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +  if ($hook == 'entity_view_alter') {
   

   ===

4. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -24,3 +27,23 @@ function layout_builder_post_update_rebuild_plugin_dependencies(&$sandbox = NULL
   +/**
   + * Ensure all extra fields are properly stored on entity view displays.
   + */
   +function layout_builder_post_update_add_extra_fields(&$sandbox = NULL) {
   

   Can the doc comment explain *why* this needs to be done? It looks like it's just saving already-existing configuration.

5. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -24,3 +27,23 @@ function layout_builder_post_update_rebuild_plugin_dependencies(&$sandbox = NULL
   +    uasort($components, 'Drupal\Component\Utility\SortArray::sortByWeightElement');
   

   Why do the components need to be sorted? Can we at least get a comment on this?

6. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -257,12 +273,18 @@ public function setComponent($name, array $options = []) {
   +    if (($field_definition && $field_definition->isDisplayConfigurable('view') && isset($options['type'])) || isset($extra_fields['display'][$name])) {
          $configuration = [];
   -      $configuration['id'] = 'field_block:' . $this->getTargetEntityTypeId() . ':' . $this->getTargetBundle() . ':' . $name;
   -      $configuration['label_display'] = FALSE;
   -      $keys = array_flip(['type', 'label', 'settings', 'third_party_settings']);
   -      $configuration['formatter'] = array_intersect_key($options, $keys);
   +      if ($field_definition) {
   +        $configuration['id'] = 'field_block:' . $this->getTargetEntityTypeId() . ':' . $this->getTargetBundle() . ':' . $name;
   +        $keys = array_flip(['type', 'label', 'settings', 'third_party_settings']);
   +        $configuration['formatter'] = array_intersect_key($options, $keys);
   +      }
   +      else {
   +        $configuration['id'] = 'extra_field_block:' . $this->getTargetEntityTypeId() . ':' . $this->getTargetBundle() . ':' . $name;
   +      }
   

   This feels a little convoluted and hard-to-read. Maybe we can refactor it like so, to avoid expanding the size of that if statement:

   if ($field_definition && $field_definition->isDisplayConfigurable('view') && isset($options['type'])) {
     // Use field_block
   }
   elseif (isset($extra_fields['display'][$name])) {
     // Use extra_field_block
   }
   

7. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -0,0 +1,170 @@
   + * @see \Drupal\layout_builder\Plugin\Block\FieldBlock
   + *   This block plugin handles all other field entities not provide by
   + *   hook_entity_extra_field_info().
   

   Should we also @see layout_builder_entity_view_alter()? Also, "provide" should be "provided".

8. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -0,0 +1,170 @@
   +    list (, , , $field_name) = explode(static::DERIVATIVE_SEPARATOR, $plugin_id, 4);
   +    $this->fieldName = $field_name;
   

   Can we assert() that $field_name is not empty?

9. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -0,0 +1,170 @@
   +    if (!isset($extra_fields['display'][$this->fieldName])) {
   +      return [];
   +    }
   

   Are we sure we don't want to cache this result?

10. +++ b/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
    @@ -0,0 +1,117 @@
    +        // Skip bundles without any extra fields.
    +        if (empty($extra_fields['display'])) {
    +          continue;
    +        }
    

    No need for this; if $extra_fields['display'] is empty, looping over it will be a no-op.

11. +++ b/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
    @@ -0,0 +1,117 @@
    +          $derivative['category'] = $this->t('@entity', ['@entity' => $entity_type->getLabel()]);
    

    Do we need the $this->t() call? I think entity type annotations already use @Translation for the label.

12. +++ b/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
    @@ -0,0 +1,117 @@
    +          $context_definition = new ContextDefinition('entity:' . $entity_type_id, $entity_type->getLabel(), TRUE);
    

    Nitpick: We probably don't want to encourage the use of the constructor with ContextDefinition (I think the ultimate dream is to make it protected, in fact), so let's use ::create() here so that we can also chain the subsequent calls to setLabel() and addConstraint(). Also, we don't need to explicitly mark it required, since that's the default.

1.

From my reading of EntityFieldManager::getExtraFields(), if I'm interpreting it correctly, $extra_fields['display'] will be defaulted to an empty array,

Actually looking at it EntityFieldManager::getExtraFields() it looks like it will only be an empty array if the key is not set by one of the of the hooks. So it could be FALSE or NULL depending on how a hook is implemented. So I think it is not a bad idea to leave it.
2. fixed
3. fixed
4. Expanded the comment to my understanding.
One thing I didn't understand about logic in layout_builder_post_update_add_extra_fields. Since only \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::setComponent() was updated and not other instance of setComponent() do we really need to call it if !$display instanceof LayoutBuilderEntityViewDisplay?
5. Added a comment. @tim.plunkett should probably double check my reasoning.
6. I shortened the if statement. I also notice that we were initializing $configuration as an empty array but then adding certain elements to the array in both cases. So I changed it to initialize the array with these values.
7. Fixed "provided" we already have a @see to layout_builder_entity_view_alter in build() near the placeholder logic.
8. added
9. added caching
10. Since the value is providing by hook implementations which may set it as empty but not an array I don't see the harm in leaving this.
11. I think you are right. fixed
12. Changing to EntityContextDefinition::fromEntityType() and removing the todo to #2932462: Add EntityContextDefinition for the 80% use case. We don't need to set the label now.

4) Once the module is installed, it will always be the right class. If this lands after the opt-in patch lands, we'll want to check ::isEnabled(). So we're fine for now.

Interdiff review:

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -30,12 +30,21 @@ function layout_builder_post_update_rebuild_plugin_dependencies(&$sandbox = NULL
   + * calls 'setComponent()` for all extra field components to ensure the updated
   ...
   +    // Calling 'setCompontent()' will set the weight of the component if it is
   

   ' vs ` around setComponent()

   Usually we don't quote method names like that. Either ::setComponent() or the full ClassName::setComponent()

2. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -30,12 +30,21 @@ function layout_builder_post_update_rebuild_plugin_dependencies(&$sandbox = NULL
   +    // Calling 'setCompontent()' will set the weight of the component if it is
   +    // not already set. To avoid reordering the components first sort the
   +    // components.
   

   I don't think the first sentence is needed (and also not 100% relevant/correct). The second one is fine by itself

3. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -73,6 +73,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +    assert(!empty($field_name));
   

   Nice!

Addressing all in #57

This is very close.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,49 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +        ExtraFieldBlock::replaceFieldPlaceholder($build, $replacement, $field_name);
   +        unset($build[$field_name]);
   

   This is a bit obscure; it's not very clear why we are unsetting $build[$field_name]. A comment would be beneficial here.

2. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -24,3 +27,30 @@ function layout_builder_post_update_rebuild_plugin_dependencies(&$sandbox = NULL
   + * calls '::setComponent()' for all extra field components to ensure the updated
   

   '::setComponent()' should not be quoted, nor should it have the :: prefix.

3. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -24,3 +27,30 @@ function layout_builder_post_update_rebuild_plugin_dependencies(&$sandbox = NULL
   +    // To avoid reordering the components first sort the components.
   +    uasort($components, 'Drupal\Component\Utility\SortArray::sortByWeightElement');
   

   Nit: Can we rephrase the comment? "Sort the components to avoid them being reordered by setComponent()."

4. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -25,6 +25,22 @@ class LayoutBuilderEntityViewDisplay extends BaseEntityViewDisplay implements La
   +  /**
   +   * The entity field manager.
   +   *
   +   * @var \Drupal\Core\Entity\EntityFieldManagerInterface
   +   */
   +  protected $entityFieldManager;
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function __construct(array $values, $entity_type) {
   +    $this->entityFieldManager = \Drupal::service('entity_field.manager');
   

   Why are we doing this?

5. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -258,13 +274,21 @@ public function setComponent($name, array $options = []) {
   +        'label_display' => '0',
   

   In FieldBlock, this is FALSE -- which is correct?

6. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -258,13 +274,21 @@ public function setComponent($name, array $options = []) {
   +        $keys = array_flip(['type', 'label', 'settings', 'third_party_settings']);
   +        $configuration['formatter'] = array_intersect_key($options, $keys);
   

   The array_flip() call is leaking implementation details of FieldBlock. I think we should add a new method on FieldBlock (static::getFormatterOptions()?) which does this work, so as to keep all FieldBlock-relevant details confined to FieldBlock.

7. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -83,8 +83,25 @@ public function testLayoutBuilderUi() {
   +    // Load the default layouts again after saving to confirms fields are only
   

   "confirms" --> "confirm"

8. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -407,4 +434,14 @@ public function testDeletedView() {
   +    $this->assertEquals(1, substr_count($this->getSession()->getPage()->getContent(), $needle), "'$needle' only appears once on the page.");
   

   I never knew about substr_count()! Thanks :)

1. added comment.
2. fixed
3. fixed
4. Added comment

// Set $entityFieldManager before calling the parent constructor because the
// constructor will call init() which then calls setComponent() which needs
// $entityFieldManager.

5. Changing to FALSE. i think this is correct after look at other time this is set in core.
6. I think is BC code, are those settings really just field options? No all formatters?
7. fixed.
8. 👍

+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
@@ -278,7 +280,7 @@ public function setComponent($name, array $options = []) {
-        'label_display' => '0',
+        'label_display' => FALSE,

This must be an integer!
Also changing it to anything else is way out of scope.

This must be an integer!

Alas, fixed

I'm satisfied. Let's do it.

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -24,3 +27,30 @@ function layout_builder_post_update_rebuild_plugin_dependencies(&$sandbox = NULL
   +function layout_builder_post_update_add_extra_fields(&$sandbox = NULL) {
   +  $entity_field_manager = \Drupal::service('entity_field.manager');
   +  \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'entity_view_display', function (EntityViewDisplayInterface $display) use ($entity_field_manager) {
   +    $extra_fields = $entity_field_manager->getExtraFields($display->getTargetEntityTypeId(), $display->getTargetBundle());
   +    $components = $display->getComponents();
   +    // Sort the components to avoid them being reordered by setComponent().
   +    uasort($components, 'Drupal\Component\Utility\SortArray::sortByWeightElement');
   +    $result = FALSE;
   +    foreach ($components as $name => $component) {
   +      if (isset($extra_fields['display'][$name])) {
   +        $display->setComponent($name, $component);
   +        $result = TRUE;
   +      }
   +    }
   +    return $result;
   +  });
   

   Does this need to be run on all displays? Or only displays that implement LayoutEntityDisplayInterface? Maybe I'm wrong looking at the update test. But I noticed that layout_builder_entity_view_alter() limited to displays with this interface.

2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -407,4 +434,14 @@ public function testDeletedView() {
   +    $this->assertEquals(1, substr_count($this->getSession()->getPage()->getContent(), $needle), "'$needle' only appears once on the page.");
   

   I think we should use mb_substr_count() just in case later we add unicode to something... ah looking at pageTextContains() we maybe want to keep things consistent... so something like:

      $actual = $this->session->getPage()->getText();
      $actual = preg_replace('/\s+/u', ' ', $actual);
      $regex = '/'.preg_quote($text, '/').'/ui';
      $message = sprintf('The text "%s" only appears once on the current page.', $text);
      $this->assertSession()->assertResponseText((bool) preg_match_all($regex, $actual) === 1, $message);
   

   Also maybe a follow up to add this to \Drupal\Tests\WebAssert as this looks useful.

3. Leaving at RTBC because I don't think these changes need to through a needs works -> needs review -> rtbc cycle if the patch authors think they are worth doing.

1)
All displays implement that once the module is installed, due to how entity type class switching works.
The only difference here vs in layout_builder_entity_view_alter() is for typehinting purposes. The alter could have also used a /** @var */ comment instead for the same effect...
I think this is fine as-is

2)
I could have sworn there was an issue to add something like pageTextContainsOnce(), @tedbow would know I'm sure.

I opened #2987478: Add WebAssert::pageTextContainsOnce() as a follow-up.

Adding review and issue management credit.

Committed and pushed 045fcb5d44 to 8.7.x and 09530d7ea8 to 8.6.x. Thanks!

diff --git a/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php b/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
index 225d51829a..d6621c0f47 100644
--- a/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
+++ b/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
@@ -8,8 +8,6 @@
 use Drupal\Core\Entity\EntityTypeBundleInfoInterface;
 use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\Entity\FieldableEntityInterface;
-use Drupal\Core\Plugin\Context\ContextDefinition;
-use Drupal\Core\Plugin\Context\EntityContext;
 use Drupal\Core\Plugin\Context\EntityContextDefinition;
 use Drupal\Core\Plugin\Discovery\ContainerDeriverInterface;
 use Drupal\Core\StringTranslation\StringTranslationTrait;

Remove used uses on commit.

@alexpott thanks!

Awesome, thanks @all.

Thanks work :)

If someone else should also run into issues with layout_builder + extra fields now (e.g. by hook or via extra_fields / extra_fields_plus module), I found two follow-up issues you should check and help to fix:

• #3069578: Layout builder doesn't show new "extra fields"
• #3069861: [3.x] Settings do not show up in layout_builder

Hi all here,

sorry to disturb here, but I'd like to ask if someone of you could help to answer, if adding a third_party_settings form to extra_fields in layout_builder is logically / technically possible in the current implementation after this patch. I tried to find that out but honestly I'm not sure if that's a follow-up and currently not cleanly possible or if I'm just to stupid to find the required information.

Background: Extra Field Settings Provider ("extra_fields_plus") "provides an interface and the extra field base plugins with editable display settings.". That works great and clean, but doesn't work with layout_builder as documented in #3069861: [3.x] Settings do not show up in layout_builder. The only patch idea there looks a bit dirty to me, so I tried to find a better solution, but failed. I guess with or without this module it makes sense to be able to allow third_party_settings (forms) for extra_fields to configure output.

I would very much appreciate the help of someone who helped to create the patch and has a better understanding of the logics in layout_builder combined with extra_field and third_party_settings.

I guess we need hook_field_formatter_third_party_settings_form to work on extra_fields?

Thank you very much in advance.

Of course I'll create a follow-up if it's not possible yet and makes sense to you, but decided to ask here first, because I think it's very close related to this fix.

@tim.plunkett, @phenaproxima, @tedbow, sorry to address you directly. I know you have a lot to do and my English is not good enough to express how much I appreciate your work! I really feel bad to ask you that and I hope this question is important and general enough to ask you for a short look and feedback on #79.

I did a lot of research, but couldn't find a clean solution to solve this general problem in contrib. If it is possible and you have a short feedback on that without investing much of your valuable time, could you perhaps have a short look at the linked issue and reply there or here for general documentation / state / plan?

I guess there are other important modules which have a similar demand... sorry in advance. I guess there are not many people as deep in this topic as you are. Feel free to advise me to create a follow-up documentation issue for that, if it makes sense for you ;)

Thank you very, very much in advance!