Move Field Layout data model and API directly into \Drupal\Core\Entity\EntityDisplayBase

We need some kind of tag to track all the blocker. Maybe something like https://www.drupal.org/project/issues/search?issue_tags_op=all+of&issue_....

I think it is safe to say that we need some manual testing and some reviews from framework manager and sub-system maintainers.

1. +++ b/core/composer.json
   @@ -96,7 +96,6 @@
   -        "drupal/field_layout": "self.version",
   
   @@ -107,7 +106,6 @@
   -        "drupal/layout_discovery": "self.version",
   

   It would be interesting to count the number of days in core for these modules when this issue is fixed.

2. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceFormatterTest.php
   @@ -188,11 +188,16 @@ public function testEntityFormatter() {
   +    ¶
   
   @@ -204,11 +209,16 @@ public function testEntityFormatter() {
   +    ¶
   

   Blank spaces.

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityViewDisplayTest.php
   @@ -1,20 +1,97 @@
   +    $expected_markup = <<<'EOS'
   ...
   +EOS;
   

   Why not call it TIM? :P

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityViewDisplayTest.php
   @@ -1,20 +1,97 @@
   +    ¶
   ...
   +      ¶
   ...
   +      ¶
   ...
   +      ¶
   

   Blank spaces.

This is a shallow review. I probably would have more questions if I did a deeper review, but overall it looks pretty good to me.

1. +++ b/core/includes/theme.inc
   @@ -1702,6 +1702,19 @@ function template_preprocess_breadcrumb(&$variables) {
   +function template_preprocess_layout(&$variables) {
   

   Can $variables be type hinted?

2. +++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
   @@ -128,4 +128,25 @@ public function getTargetBundle();
   +  /**
   +   * Gets the field's portion of the fully built entity display render array.
   +   *
   +   * @param string $field_name
   +   *   The field name.
   +   * @param array $build
   +   *   The full render array.
   +   *
   +   * @return array
   +   *   The portion of the render array corresponding to the given field name.
   +   */
   +  public function &getFieldFromBuild($field_name, array &$build);
   +
   +  /**
   +   * Gets the default region.
   +   *
   +   * @return string
   +   *   The default region for this display.
   +   */
   +  public function getDefaultRegion();
   

   Doesn't adding methods to the interface constitute a BC break, however unlikely?

3. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
   @@ -213,6 +216,32 @@ public function processForm($element, FormStateInterface $form_state, $form) {
   +  public function applyLayout($element, FormStateInterface $form_state, $form) {
   

   Can $element and $form be type hinted?

4. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -412,6 +441,18 @@ public function getHighestWeight() {
      /**
   +   * Gets the extra fields for this display.
   +   */
   +  protected function getExtraFields() {
   

   The doc comment is missing a @return.

5. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -545,13 +586,112 @@ protected function getPluginRemovedDependencies(array $plugin_dependencies, arra
   +  public function setLayoutId($layout_id, array $layout_settings = []) {
   

   I think this is named very strangely. This is more what I would expect the signature of setLayout() to be, not setLayoutId(). IMHO we should remove setLayoutId() entirely, if we have the latitude to do that, and leave setLayout() only, with this signature and logic.

6. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -545,13 +586,112 @@ protected function getPluginRemovedDependencies(array $plugin_dependencies, arra
   +    $this->setLayoutId($layout->getPluginId(), $layout->getConfiguration());
   +    return $this;
   

   Nit: setLayoutId() returns $this, so this can just directly return the result of setLayoutId().

7. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -399,13 +399,8 @@ public function viewField(FieldItemListInterface $items, $display_options = [])
   +    return $display->getFieldFromBuild($field_name, $build);
   

   Should this be returning by reference, to match getFieldFromBuild()'s return?

8. +++ b/core/lib/Drupal/Core/Layout/Plugin/Layout/DefaultLayout.php
   @@ -0,0 +1,34 @@
   + *   description = @Translation("A layout with no markup"),
   

   Can this description be changed? It's misleading and sounds like nothing will be rendered at all. Maybe just "Renders everything in the layout in a single region without any wrapping markup" or similar.

9. +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
   @@ -55,10 +66,13 @@
   +  public function __construct(FieldTypePluginManagerInterface $field_type_manager, PluginManagerBase $plugin_manager, LayoutPluginManagerInterface $layout_plugin_manager) {
   

   Should $layout_plugin_manager default to NULL to preserve BC? I think this is fine, personally, but I still thought I'd point it out.

10. +++ b/core/modules/rdf/rdf.module
    @@ -502,7 +503,19 @@ function rdf_preprocess_comment(&$variables) {
    +function rdf_entity_display_build_alter(&$build, $context) {
    +  if (!($context['entity'] instanceof CommentInterface)) {
    +    return;
    +  }
    +
    +  $comment = $context['entity'];
    +  $rdf_metadata_attributes = [];
    +  $mapping = rdf_get_mapping('comment', $comment->bundle());
    

    Wat? Why is this here? It doesn't seem to be layout related...

11. +++ b/core/modules/user/user.module
    @@ -392,13 +392,14 @@ function user_user_view(array &$build, UserInterface $account, EntityViewDisplay
    +function user_entity_display_build_alter(&$build, $context) {
    +  $account = $context['entity'];
    +  if ($account instanceof UserInterface && user_picture_enabled() && !empty($build['user_picture'])) {
    

    This also seems kind of strange. I'm not sure how it ties into the layout system changes.

1. +++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
   @@ -128,4 +128,25 @@ public function getTargetBundle();
   +   *
   +   * @return array
   +   *   The portion of the render array corresponding to the given field name.
   +   */
   +  public function &getFieldFromBuild($field_name, array &$build);
   

   I'm wondering whether we can document a bit why getFieldFromBuild returns a referece

2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -545,13 +590,112 @@ protected function getPluginRemovedDependencies(array $plugin_dependencies, arra
   +    // Ignore any extra fields from the list of field definitions. Field
   +    // definitions can have a non-configurable display, but all extra fields are
   +    // always displayed.
   +    $field_definitions = array_diff_key($this->getFieldDefinitions(), $this->getExtraFields());
   +
   +    $fields_to_exclude = array_filter($field_definitions, function (FieldDefinitionInterface $field_definition) {
   +      // Remove fields with a non-configurable display.
   +      return !$field_definition->isDisplayConfigurable($this->displayContext);
   +    });
   +    $components = array_diff_key($components, $fields_to_exclude);
   +
   +    // Only include fields present in the build.
   +    $components = array_intersect_key($components, $build);
   

   I was wondering whether it would be clearer to read when it would be something like: $components =

   array_filter($components, function ($component_id) {
     return isset($field_definitions) && !isset($extra_fields) && !$field_definition->isDisplayConfigurable() && isset($build);
   }, ARRAY_FILTER_USE_KEY);

   Then I was wondering though: Is the current implementation better O(*) wise? I would argue no, because isset() aka. a hashmap lookup by key is O(1), so the total complexity is O(N), which should be better than what it is now, see https://stackoverflow.com/questions/2473989/list-of-big-o-for-php-functions for a list of complexity time for all the various PHP functions.

2) You could use \Zend\Stdlib\ArrayUtils::filter which we already ship with in core.

1. +++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
   @@ -129,19 +129,6 @@ public function getTargetBundle();
   -  public function &getFieldFromBuild($field_name, array &$build);
   
   +++ b/core/lib/Drupal/Core/Entity/Display/EntityViewDisplayInterface.php
   @@ -46,4 +46,37 @@ public function build(FieldableEntityInterface $entity);
   +  public function getFieldFromBuild($field_name, array $build);
   ...
   +  public function setFieldOnBuild($field_name, array $field_array, array &$build);
   

   <3

2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -600,22 +601,27 @@ protected function getPluginRemovedDependencies(array $plugin_dependencies, arra
   +    return ArrayUtils::filter($components, function ($component_id) use ($field_definitions, $extra_fields, $build) {
   ...
   +    }, ArrayUtils::ARRAY_FILTER_USE_KEY);
   

   Nice improvement, I like that.
   Let's put some todo here to not require the library when we are on php 5.6?

Until #2867960: Merge Component composer.json files to account for them during build is done, a core/lib/Drupal/Core/Entity/component.json to ensure zend-stdlibs is included will not be respected.

But that never stopped us from declaring dependencies in the components anyway. (And copying some to core/composer.json to make everything actually work...)

But it's probably most effective to get #2769841: Prefer caret over tilde in composer.json in first, now that it is RTBC...

Tried to reroll this up to 8.7, but here is too many conflicts in many files. Is it this issue still actual?

Personally I'm of two minds, on one hand this is a useful module and totally makes sense, but on the other hand not everyone needs it so keeping it separate means the Field UI stays simpler (cleaner? not sure what the best word is) for those who don't need it.

Reparenting to #3007167: [META] Enable layout builder for form displays, and deprecate field_layout after discussion with @tim.plunkett.

It looks great, quickly skimmed and found only nitpick

+++ b/core/includes/theme.inc
@@ -1716,6 +1716,27 @@ function template_preprocess_breadcrumb(&$variables) {
+function template_preprocess_layout(&$variables) {

other issues are trying to kill includes so maybe better to move it to system.module?

Patch is working but we had some issue's while updating:
1. The view display settings are not updated to the new settings zo field_layout needs to have a upgrade path to the core functionality like:

  /* @var \Drupal\Core\Entity\EntityTypeManagerInterface $viewDisplayEntities */
  $entity_type_manager = \Drupal::service('entity_type.manager');
  $storage = $entity_type_manager->getStorage('entity_view_display');
  $entities = $storage->loadMultiple();
  foreach ($entities as $entity) {
    /* @var \Drupal\Core\Entity\Entity\EntityViewDisplay $entity */
    $layout_settings = $entity->getThirdPartySettings('field_layout');
    if (isset($layout_settings, $layout_settings['id'])) {
      $entity->setLayoutFromId($layout_settings['id'], $layout_settings['settings'] ?? []);
      $entity->save();
    }
  }

2. The dependentie on layout_discovery by default it breaks now selecting a new layout because layout.icon_builder cannot be found because it is not a default dependentie of core. Maybe we need to hide or make notice? when layout_discovery is not enabled?

3. Breaks parargaphs template or other templates where content.field_name is used (example paragraphs) because of the _layout structure is enabled on every EntityViewDisplay i would say that _layout only needs to be used when there is a layout selected other then default?

4. Also in field_ui is there still some old getThirdPartySettings that uses field_layout instead of the $display->getLayout see patch in new comment! (EDIT: i see that it it field_group contrib where this needs to change)

5. I see also a issue with entity_extra_field_info fields that uses hook_entity_view to insert there render array to the build but because it is moved to core now the order is not right i guess. _layout is already setup and later on the pseudo-fields is added to the render array. So that breaks the layout and shows the pseudo-fields below the layout parts.

6. How sure are we that this is going to move to core? (maybe better place in the meta issue?)

Kind regards,
Lennard Westerveld

Here is a patch of #45 for Drupal 8.6.10
With a additional patch for making the _layout optional only when not the default layout is selected that also can apply on 8.7.x #45 that fixes issues for templates that already created with for example `{{content.field_content}} ` not causing errors.

Needs reroll for 8.7.0 and 8.8.x

Reroll for 8.7.x

Something that I've noticed, and it annoys me, is that when Layout Builder and Field Layout are installed, Field Layout is completely gone on the Manage Display tab, but not on the Manage Form Display tab.

Is that a Field Layout thing, or a Layout Builder thing? Because I actually find myself wanting to use both Field Layout and Layout Builder, just on different Entity Types.

So if that's a Field Layout thing, I'd like to suggest "fixing" that here so it doesn't happen.

So where are almost 1 year further, what are the plans? about this issue?

lots to do in the world.. status is needs work!

I've tried to reroll patch from #52

Was fixed coding standards

I've applied the patch #60, but it fails (the patch works but Drupal then craps out).
In /core/lib/Drupal/Core/Entity/EntityDisplayBase.php in getPluginCollection() the DefaultSingleLazyPlugin class is used, but not imported.
Fixed by adding use Drupal\Core\Plugin\DefaultSingleLazyPluginCollection;

1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
   @@ -282,11 +284,78 @@ public function buildMultiple(array $entities) {
          \Drupal::moduleHandler()->alter('entity_display_build', $build_list[$id], $context);
   +      $this->applyLayout($build_list[$id]);
   

   probably layout should be applied before alter

2. +++ b/core/modules/workspaces/config/install/core.entity_form_display.workspace.workspace.deploy.yml
   @@ -13,3 +13,5 @@ content: {  }
   +layout_settings: {  }
   \ No newline at end of file
   
   +++ b/core/profiles/demo_umami/config/install/core.entity_form_display.media.image.default.yml
   @@ -72,3 +72,5 @@ content:
   +layout_settings: {  }
   \ No newline at end of file
   
   +++ b/core/profiles/demo_umami/config/install/core.entity_form_display.node.recipe.default.yml
   @@ -179,3 +179,5 @@ content:
   +layout_settings: {  }
   \ No newline at end of file
   
   +++ b/core/profiles/demo_umami/config/install/core.entity_view_display.node.article.full.yml
   @@ -51,3 +51,5 @@ content:
   +layout_settings: {  }
   \ No newline at end of file
   
   +++ b/core/profiles/demo_umami/config/install/core.entity_view_display.node.page.default.yml
   @@ -31,3 +31,5 @@ content:
   +layout_settings: {  }
   \ No newline at end of file
   

   nitpick

Here I have made changes as suggested in comment #63.

Reroll patch for Drupal 9.1

Rerolled patch 9.1.x as patch #67 failed to apply.

Back to Need Work as patch #69 didn't pass the tests.

There are errors shown by test bot, which are as below.

/var/lib/drupalci/workspace/jenkins-drupal_patches-45340/source/core/lib/Drupal/Core/Entity/Display/EntityDisplayWithLayoutInterface.php ✗ 1 more
line 5	Unused use statement
/var/lib/drupalci/workspace/jenkins-drupal_patches-45340/source/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php ✗ 5 more
242	Line indented incorrectly; expected 8 spaces, found 7
244	Closing brace indented incorrectly; expected 7 spaces, found 8
371	Whitespace found at end of line
375	Expected 1 space before opening brace; found 0
385	Whitespace found at end of line
/var/lib/drupalci/workspace/jenkins-drupal_patches-45340/source/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php ✗ 8 more
303	Line indented incorrectly; expected 6 spaces, found 5
303	Closing brace indented incorrectly; expected 6 spaces, found 5
377	Whitespace found at end of line
378	Line indented incorrectly; expected 2 spaces, found 3
381	Expected 1 space before opening brace; found 0
390	Expected 1 blank line after function; 0 found
390	Whitespace found at end of line
391	The closing brace for the class must have an empty line before it
/var/lib/drupalci/workspace/jenkins-drupal_patches-45340/source/core/lib/Drupal/Core/Entity/EntityDisplayBase.php ✗ 2 more
612	Line indented incorrectly; expected 2 spaces, found 1
614	Closing brace indented incorrectly; expected 1 spaces, found 2
/var/lib/drupalci/workspace/jenkins-drupal_patches-45340/source/core/modules/node/tests/src/Kernel/SummaryLengthTest.php ✗ 1 more
114	Whitespace found at end of line
/var/lib/drupalci/workspace/jenkins-drupal_patches-45340/source/core/tests/Drupal/Tests/EntityViewTrait.php ✗ 4 more
12	Whitespace found at end of line
13	Line indented incorrectly; expected 2 spaces, found 3
22	Whitespace found at end of line
44	Whitespace found at end of line

Fixed coding standards mentioned in #74

Just re-rolled against 9.1
Let's see that test bot will say

Next try

Fixed coding standards issues and fixed wrong path for these tests in patch file:

core/tests/Drupal/Tests/Core/Entity/Display/EntityFormDisplayTest.php
core/tests/Drupal/Tests/Core/Entity/Display/EntityViewDisplayTest.php

@aleevas,

#80 is applied on local.
I have removed some unused use statement, I tried to check test cases but there are failed a lot of test cases, need to work for test cases.

I ported patch #82 for d8.9 for anyone else needing it.

Rerolled the patch in #82 and tried to fix test failures.

Thanks!

Fixing custom command failure errors in #86, thanks!

#87 was not applying, and when I rerolled it actually wouldn't work because of missing getLayout() in EntityDisplayBase. Ended up doing a mixture of the last instance with getLayout(), which was from #75, and the most recent rebase, #87.

Some other issues I ran into:

• any displays that already exist and don't have layoutid or layoutsettings errored, so I added in defaults in their get methods.
• exporting config doesn't seem to actually set layout id in some cases, can't figure out why
• can't seem to figure out how to select the layout in the ui itself

Amendment to my comment above (#89): fixing namespacing issue for an old Zend reference. New patch attached.

Still needs proper reroll

reroll of #90 for 9.3.x

Fixed CS and some test failures, added few todos as not sure this methods are needed

fix more cs

Is there any progress ?

I think this is the same direction as this! #3389432: Make regions and fields pluggable or alterable in EntityDisplayFormBase