Add experimental Field Layout module to allow entity view/form modes to switch between layouts

Elegance in simplicity

1. +++ b/core/modules/field_layout/field_layout.info.yml
   @@ -0,0 +1,8 @@
   +package: Core (Experimental)
   

   Given there's so little code - does this need to be experimental? Does it even need to live outside field_ui?

2. +++ b/core/modules/field_layout/field_layout.module
   @@ -0,0 +1,89 @@
   +function _field_layout_get_layout() {
   

   If this was a service - could contrib extend it now to return new regions and layouts?

3. +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityViewDisplayEditForm.php
   @@ -0,0 +1,127 @@
   +    $position = array_search('parent_wrapper', array_keys($row)) + 1;
   +    return array_merge(array_slice($row, 0, $position), $new_row, array_slice($row, $position));
   ...
   +    array_splice($header, 3, 0, [$this->t('Region')]);
   

   These could be constants?

Yay!

My first reaction was 'Why so conservative'. Given that I just rewatched the Batman trilogy, this sounded very dramatic, but I'm a nicer guy than the joker :) I guess with #2296423: Implement layout plugin type in core being moved into core directly instead of a module, I was more or less assuming to move this functionality into core directly too. However, I understand that we're just in MVP phase, so it's better to play safe here. It's just a bit weird to see the same code (or at least same entry paths to alter) that happened in DS (don't know how panelizer/panels does).

Two things that stood out already in the patch:

1. +++ b/core/modules/field_layout/field_layout.info.yml
   @@ -0,0 +1,8 @@
   +dependencies:
   +  - field_ui
   

   This would mean that you can't disable Field UI anymore which is just a UI. You can disable field ui and still have your entity displays working too. DS works fine too without Field UI enabled. I think we need the same here too. I guess it's a step to far to create a field layout ui module for it.

2. +++ b/core/modules/field_layout/field_layout.module
   @@ -0,0 +1,152 @@
   +  $field_layout = $display->getThirdPartySetting('field_layout', 'fields', []);
   +  foreach ($display->getComponents() as $name => $component) {
   +    if (isset($build[$name]) && isset($field_layout[$name]['region'])) {
   +      $region = $field_layout[$name]['region'];
   +      if ($region !== 'hidden') {
   +        $build[$region][$name] = $build[$name];
   +      }
   +      unset($build[$name]);
   +    }
   

   Why loop over the components ? This looks weird. If we take the configuration from the layout, then we know which fields are going to be rendered. All other fields should automatically be hidden and not be rendered. this is what we do in DS This would mean the 'unset($build[$name])' can be removed too (I guess in the end #access FALSE would be better anyway instead of unsetting).

Another thought, but more of a semantic discussion. Field Layout sounds a bit poor. We're not really configuring layouts on fields, but entity displays, so entity_layout sounds better in my mind. But I don't want to fight to hard on this though. The reason is that I know that with the entity field api people we discussed whether to rename field ui to entity ui (I thought we had an issue already for that, but can't find it, guess we just wanted to ship D8 first heh)

Hmmm, wasn't a huge fan of adding that service because of what you said in #4.2 which I agreed with initially, however you can swap it and then call the layouts if you have layout plugin module installed which would be your best bet, so yeah, I can live with it.

Or ... can me make an experimental core module dependent on the contrib layout plugin module ? :-) (/runs)

I think other then module name it is pretty much ready. +1 for the service.

+++ b/core/modules/field_layout/tests/src/FunctionalJavascript/EntityViewLayoutTest.php
@@ -0,0 +1,88 @@
+    $this->submitForm([], 'Save');

We should assert row weight before and after this.

+++ b/core/modules/field_layout/field_layout.module
@@ -0,0 +1,115 @@
+/**
+ * Implements hook_entity_view_alter().
+ */
+function field_layout_entity_view_alter(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display) {
+  _field_layout_apply_layout($build, $display);
+}
+

In DS we set the module weight to 1. I'm not 100% sure anymore why we did that, but I can at least think of one reason: extra fields might be added by other modules in the same hook, but could potentially run after this one. So either a module weight or a module_implements_alter is needed then. Shouldn't be part of this patch though, can be a follow up.

1. +++ b/core/modules/field_layout/field_layout.module
   @@ -0,0 +1,153 @@
   +  $field_layout = $display->getThirdPartySetting('field_layout', 'fields', []);
   +  foreach ($display->getComponents() as $name => $component) {
   +    if (isset($build[$name]) && isset($field_layout[$name]['region'])) {
   +      $region = $field_layout[$name]['region'];
   +      if ($region !== 'hidden') {
   +        $build[$region][$name] = $build[$name];
   +      }
   +      unset($build[$name]);
   +    }
   +  }
   

   I'm wondering whether altering the form structure itself is the right thing to do. Doesn't that make it harder for other code to alter the form as well?

2. +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityDisplayFormTrait.php
   @@ -0,0 +1,176 @@
   +  /**
   +   * Overrides \Drupal\field_ui\Form\EntityDisplayFormBase::getRowRegion().
   +   */
   +  public function getRowRegion($row) {
   

   We are getting super oldschool now :)

Briefly talked with tim in IRC because I didn't understand the hook_install and entity_presave hooks very well. In short, the idea is that a layout is required, so on install, we need to configure the displays already. That's why there's a 'default' layout defined now, which can be replaced in future patches where say the node entity type then declares the node.tpl as its default layout for instance. The install and presave dance still feels a bit weird, but maybe we can come up with better ideas in the future, for now I'm fine with that just to get this in.

It's weird though that the uninstall hook is needed, I'd swear that CMI cleans up third party settings when uninstalling a module.

We reviewed current status during today's UX meeting: https://youtu.be/vRNPFs9fISk?t=37m16s (with screenshare demo)

- This is exciting!
- Current patch uses existing user interface paradigms, which results in a sub-par ux but don't let that hold back an initial commit
- The scope seems clear enough: should we open a separate issue for exploring UI ideas for this?
- Go Tim!

Just some quick feedback while not falling asleep.

1. +++ b/core/modules/field_layout/field_layout.install
   @@ -0,0 +1,26 @@
   +  array_map($entity_save, EntityViewDisplay::loadMultiple());
   +  array_map($entity_save, EntityFormDisplay::loadMultiple());
   +
   

   Conceptually this is an array_walk() not a map. A map stores values.

2. +++ b/core/modules/field_layout/field_layout.module
   @@ -0,0 +1,156 @@
   +function field_layout_entity_view_alter(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display) {
   +  if ($display instanceof EntityDisplayWithLayoutInterface) {
   +    _field_layout_apply_layout($build, $display);
   +  }
   +}
   

   I'm wondering whether we should move this method via hook_module_implements_alter() to the end.

3. +++ b/core/modules/field_layout/field_layout.module
   @@ -0,0 +1,156 @@
   +  if (isset($layout_definition['library'])) {
   +    $build['field_layout']['#attached']['library'][] = $layout_definition['library'];
   +  }
   

   I'm wondering whether one or multiple library support is what one actually needs. I guess it just makes sense to have a single one?

4. +++ b/core/modules/field_layout/field_layout.module
   @@ -0,0 +1,156 @@
   +    // If the component is a true field, but not configurable, do not alter it.
   +    if (isset($field_definitions[$name]) && !$field_definitions[$name]->isDisplayConfigurable($display->get('displayContext'))) {
   +      continue;
   +    }
   

   It would be nice to document why we are doing this here. Why are base fields treated different

5. +++ b/core/modules/field_layout/src/LayoutRepository.php
   @@ -0,0 +1,141 @@
   +    // Add the module or theme path to the 'path'.
   +    if ($this->moduleHandler->moduleExists($definition['provider'])) {
   +      $definition['provider_type'] = 'module';
   +      $base_path = $this->moduleHandler->getModule($definition['provider'])->getPath();
   +    }
   +    elseif ($this->themeHandler->themeExists($definition['provider'])) {
   +      $definition['provider_type'] = 'theme';
   +      $base_path = $this->themeHandler->getTheme($definition['provider'])->getPath();
   +    }
   +    else {
   +      $base_path = '';
   +    }
   +    $definition['path'] = !empty($definition['path']) ? $base_path . '/' . $definition['path'] : NULL;
   

   Should we support no provider?

6. +++ b/core/modules/field_layout/src/LayoutRepository.php
   @@ -0,0 +1,141 @@
   +    // Either a theme or a template must be defined.
   +    if (empty($definition['template']) && empty($definition['theme'])) {
   +      throw new InvalidPluginDefinitionException($plugin_id);
   +    }
   

   It is interesting that we support theme functions even ... maybe we don't want to do so in the first place

7. +++ b/core/modules/field_layout/src/LayoutRepositoryInterface.php
   @@ -0,0 +1,39 @@
   +
   +  /**
   +   * Gets sorted plugin definitions grouped by category.
   +   *
   +   * In addition to grouping, both categories and its entries are sorted,
   +   * whereas plugin definitions are sorted by label.
   +   *
   +   * @param array[]|null $definitions
   +   *   (optional) The plugin definitions to group. If omitted, all plugin
   +   *   definitions are used.
   +   *
   +   * @return array[]
   +   *   Keys are category names, and values are arrays of which the keys are
   +   *   plugin IDs and the values are plugin definitions.
   +   */
   +  public function getGroupedDefinitions(array $definitions = NULL);
   

   Is the plan to move this method and use the categorize plugin manager, wants the layouts are pluggable.

8. +++ b/core/modules/field_layout/tests/src/FunctionalJavascript/FieldLayoutTest.php
   @@ -0,0 +1,253 @@
   +  /**
   +   * Tests an entity type that has fields shown by default.
   +   */
   +  public function testNodeView() {
   +    // By default, the default layout is used.
   +    $this->drupalGet('node/1');
   +    $this->assertSession()->elementExists('css', '.field-layout--default');
   +    $this->assertSession()->elementExists('css', '.field-layout-region--content .field--name-body');
   +
   +    $this->drupalGet('admin/structure/types/manage/article/display');
   +    $this->assertEquals(['Content', 'Disabled'], $this->getRegionTitles());
   +    $this->assertSession()->optionExists('fields[body][region]', 'content');
   +  }
   +
   

   At least this test doesn't have to run in the JS environment or does it?

8) It doesn't need it, but should I make a separate BTB just for this method?

We have a bit of a problem already because javascript tests aren't executed in parallel at the moment, so it slows down test results more than normally.

Interesting. Oh PHP

When manually testing this, switching layouts the first time (to "Two column") threw the notice Undefined index: type in Drupal\field_ui\Form\EntityDisplayFormBase->determineComponentAction() (line 664 of core/modules/field_ui/src/Form/EntityDisplayFormBase.php). Every subsequent change did not throw Notices.

I manually went through the entire flow of using a Two Column layout and re-positioning fields, and that seems to work as expected. Nice job!

Looking at the code:

1. I would change the details title to say "Layout settings", I contextually know from tabs and breadcrumbs what Entity Bundle and View Mode I'm editing so I'm not sure if we need to be verbose about things.
2. I would also change the name of the "Default" layout to "One column", as it's a bit confusing to be editing a "Default" Display Mode, opening a details element which says "Default", then selecting "Default" from a dropdown list.
3. Changing an Article's form display to "Two column", then moving the Image field to the right column, does not display fields in Two Columns using Seven. I believe applying a rule like "max-width: 50%;" to the regions would prevent this from happening, although without padding the form doesn't look great, so maybe this is the theme's responsibility? Not sure at this point.
4. Why is an AJAX call made when moving Fields between regions in the Field UI?

5. +/**
   + * Implements hook_entity_view_alter().
   + */
   +function field_layout_entity_view_alter(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display) {
   +  if ($display instanceof EntityDisplayWithLayoutInterface) {
   +    _field_layout_apply_layout($build, $display);
   +  }
   +}
   

   One alternative to doing alters would be to attach a custom View Builder using hook_entity_type_alter(), not sure if that's a fit or we need that complexity, but it's what Panelizer ended up doing in its Drupal 8 port. Panelizer has to deal with Entities being "Panelized" or not, and Field layout is implicitly enabled for all Entities, so this may not be relevant here.

6. +/**
   + * Implements hook_entity_type_alter().
   + */
   +function field_layout_entity_type_alter(array &$entity_types) {
   +  /** @var $entity_types \Drupal\Core\Entity\EntityTypeInterface[] */
   +  $entity_types['entity_view_display']->setClass(FieldLayoutEntityViewDisplay::class);
   +  $entity_types['entity_form_display']->setClass(FieldLayoutEntityFormDisplay::class);
   +
   +  // The form classes are only needed when Field UI is installed.
   +  if (\Drupal::moduleHandler()->moduleExists('field_ui')) {
   +    $entity_types['entity_view_display']->setFormClass('edit', FieldLayoutEntityViewDisplayEditForm::class);
   +    $entity_types['entity_form_display']->setFormClass('edit', FieldLayoutEntityFormDisplayEditForm::class);
   +  }
   +}
   

   Panelizer accomplishes this by implementing hook_form_entity_view_display_edit_form_alter() and attaching a new submit handler which makes sure the third party settings are saved appropriately. Again, just pointing this out in case it's easier to go an alternative route.

Yeah, notes 5 and 6 were just me reading through the code and thinking "I wonder why Panelizer does the opposite?". If you prefer the methods you used I'm good with that.

We can keep flexbox and add max widths and margins, I can take a stab at the CSS required for that. I assume that day one people are going to start turning this on for Forms and will expect multiple columns to work regardless of the admin theme.

This change adds a 10px margin between the left and right column, and prevents columns from being wider than 50% (which would prevent flexbox from displaying two columns).

This is going to be great! Just a couple of comments

1.   +++ b/core/modules/field_layout/field_layout.module
     @@ -0,0 +1,156 @@
     +function _field_layout_apply_layout(array &$build, EntityDisplayWithLayoutInterface $display, $in_form_context = FALSE) {
     

   This function seems like it would be better in a service. Just easier to find and it is pretty complex.

2. But either way do we need to pass in $in_form_context. Later on $display->get('displayContext') is being checked. Wouldn't this suffice to determine if we are in form context.

3.   +++ b/core/modules/field_layout/field_layout.module
     @@ -0,0 +1,156 @@
     +function field_layout_entity_presave(EntityInterface $entity) {
     

   This is being called for every entity save. Seems like it would be more efficient(though less elegant) to implement 2 hook_ENTITY_TYPE_presave's

1) It is complex, but exposing it as a service would be wrong. We don't want others to consume this, it's not an API.

In that case, just create a class and call it. We are doing this too less, because we are scared by OOP still.

Nice!

+++ b/core/modules/field_layout/config/schema/field_layout.schema.yml
@@ -0,0 +1,13 @@
+    layout:
+      type: string
+      label: 'Layout'

Now that this is using layout_plugin, you need to store layout configuration here, otherwise layouts that have it will explode!

A few nits

1. +++ b/core/modules/field_layout/config/schema/field_layout.schema.yml
   @@ -0,0 +1,16 @@
   +  label: 'Per-view mode field layout settings'
   

   I think this should be "Per view mode" not "Per-view mode". I guess it could be Per view-mode"

2. +++ b/core/modules/field_layout/field_layout.module
   @@ -0,0 +1,69 @@
   +      $output .= '<p>' . t('The Field Layout module allows you to arrange fields into regions for content forms and displays.') . '</p>';
   

   Why have "content" here?
   Field info file says "Field API to add fields to entities like nodes and users."

   Could be:
   "The Field Layout module allows you to arrange fields into regions on forms and displays of entities such as nodes and users."

3. +++ b/core/modules/field_layout/src/Entity/FieldLayoutEntityViewDisplay.php
   @@ -0,0 +1,25 @@
   + * Provides an entity view display entity that has layout.
   

   "that has a layout"

1. +++ b/core/modules/field_layout/src/FieldLayoutBuilder.php
   @@ -0,0 +1,123 @@
   +   * @param \Drupal\layout_plugin\LayoutPluginManagerInterface $layout_plugin_manager
   +   * @param \Drupal\Core\Entity\EntityFieldManagerInterface $entity_field_manager
   

   Missing @param comments

2. +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityDisplayFormTrait.php
   @@ -0,0 +1,205 @@
   +        'message' => $this->t('No field is displayed.')
   

   Missing comma at end of line

3. +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityDisplayFormTrait.php
   @@ -0,0 +1,205 @@
   +      'message' => $this->t('No field is hidden.')
   

   Missing comma at end of line

+++ b/core/modules/field_layout/src/Form/FieldLayoutEntityFormDisplayEditForm.php
@@ -0,0 +1,46 @@
+use Drupal\Core\Field\FieldDefinitionInterface;
...
+use Drupal\Core\Form\FormStateInterface;

Unused import statements

1. +++ b/core/modules/field_layout/src/FieldLayoutBuilder.php
   @@ -0,0 +1,125 @@
   +      if ($display_context === 'form') {
   +        $fill['#process'][] = '\Drupal\Core\Render\Element\RenderElement::processGroup';
   +        $fill['#pre_render'][] = '\Drupal\Core\Render\Element\RenderElement::preRenderGroup';
   +      }
   +      $regions = array_fill_keys($layout_definition->getRegionNames(), $fill);
   +
   ...
   +        if ($display_context === 'form') {
   +          $build[$name]['#group'] = $field['region'];
   +        }
   +        // Otherwise, move the field from the top-level of $build into a
   +        // region-specific section.
   +        else {
   +          $regions[$field['region']][$name] = $build[$name];
   +          unset($build[$name]);
   +        }
   

   I'm wondering whether we could get rid of having a FieldLayoutBuilder for forms and one for views

2. +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityDisplayFormTrait.php
   @@ -0,0 +1,205 @@
   +      $layout_id = $form_state->getValue('field_layout') ?: ($stored_layout_id ?: 'onecol');
   

   What about using $form_state->getValue(, $stored_layout ? ... )

3. +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityDisplayFormTrait.php
   @@ -0,0 +1,205 @@
   +    // If the layout is changing, reset all fields.
   +    if ($this->updateLayout($entity, $form_state)) {
   

   Maybe we could explain why

4. +++ b/core/modules/field_layout/tests/src/Unit/FieldLayoutBuilderTest.php
   @@ -0,0 +1,283 @@
   +class FieldLayoutBuilderTest extends UnitTestCase {
   

   I really like this test

1. Since layout ID is expected to be unique and Panels already uses 'onecol' and 'twocol', it would cause conflict/unexpected behaviour after Panels gets installed, maybe prefix them somehow.
Should modules be responsible for assuring their layout IDs are unique ?

2. After uninstalling module providing layout used on view I get this when viewing a node:

"Drupal\Component\Plugin\Exception\PluginNotFoundException: The "twocol_bricks" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 52 of core\lib\Drupal\Component\Plugin\Discovery\DiscoveryTrait.php).
Drupal\Core\Plugin\DefaultPluginManager->getDefinition('twocol_bricks') (Line: 22)
Drupal\field_layout\Entity\FieldLayoutEntityViewDisplay->getLayoutDefinition('twocol_bricks') (Line: 22)
Drupal\field_layout\Entity\FieldLayoutEntityViewDisplay->getDefaultRegion() (Line: 157)
..."

So another question is: should module providing layouts be responsible for cleaning up 'field_layout' on affected bundles ?

Other then the module name I have unable to find anything objectionable in the patch.

@tim.plunkett mentioned that @amateescu's feedback and review could be valuable here. We already have review from a (former) Field maintainer, so this might be optional (thence leaving RTBC), but also tagging.

Marking for relevant committer reviews as well.

Feature request really, but definitely a major one.

Thanks @jibran for the screenshots. That is great.

+++ b/core/modules/field_layout/field_layout.module
@@ -0,0 +1,69 @@
+/**
+ * Implements hook_form_alter().
+ */
+function field_layout_form_alter(&$form, FormStateInterface $form_state, $form_id) {
+  $form_object = $form_state->getFormObject();

IIRC, we have a hook_form_BASE_FORM_ID_alter (so we can do form_node_form_alter). Wondering whether we could introduce someting alike for contentEntityForm (or maybe that even exists already).

Should not hold up commit, but can maybe be some sort of optimisation ?

I thought about the name. Here are the few names, some are already suggested by @tim.plunkett earlier in the issue:

• Field Layout: This name makes a lot of sense. It is developer and site builder friendly. You are in the field_ui and you are changing the way the fields are appearing in form or on the page.
• Entity Layout: This name makes perfect sense because this is changing the entity form and entity view layouts but we always tried to avoid the use of the word 'entity' in the UI.
• Entity UI: We are using this functionality in field UI so why not name it entity UI but this name doesn't make a lot of sense.
• Entity Field Layout: Again, this is a very elaborative name but we don't want to use the word 'entity' in UI.
• Entity Display Layout: Yes, we are changing the entity display layouts but the word 'display' seems redundant here and this becomes Entity Layout again.
• Entity Field Display Layout: Very elaborative name but it is a very long name. The word 'display' is redundant and we avoid the use of word 'entity' in UI so back to Field Layout
• DS: This module is adding almost 70-80%^* functionality of DS⁺ contrib module in core so why not name it DS.

Given all that, I think field layout is the best fit here.

* Just a random number please don't bite my head off on this.

+ Not including the ds_extra module into the calculation.

Quick First Time review on my phone, are contextual links working in the frontend? I had to add some lines in each template to make it work (for ds)

Gave this a read-through. I found nitpicks, but nothing that should block commit, IMHO.

1. +++ b/core/modules/field_layout/field_layout.module
   @@ -0,0 +1,69 @@
   +      $output .= '<p>' . t('For more information, see the <a href=":field-layout-documentation">online documentation for the Field Layout module</a>.', [':field-layout-documentation' => 'https://www.drupal.org/documentation/modules/@todo_once_module_name_is_decided_upon']) . '</p>';
   

   Not sure this should be committed as-is :)

2. +++ b/core/modules/field_layout/field_layout.module
   @@ -0,0 +1,69 @@
   +    \Drupal::classResolver()->getInstanceFromDefinition(FieldLayoutBuilder::class)
   

   Nit: Should the call to getInstanceFromDefinition() perhaps be on its own line for readability? Not sure how that would jibe with the coding standards, though...

3. +++ b/core/modules/field_layout/field_layout.module
   @@ -0,0 +1,69 @@
   +function field_layout_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   

   &$form should be type hinted.

4. +++ b/core/modules/field_layout/layouts/twocol/twocol.layout.css
   @@ -0,0 +1,14 @@
   +  display: flex;
   

   <3

5. +++ b/core/modules/field_layout/src/Display/EntityDisplayWithLayoutInterface.php
   @@ -0,0 +1,67 @@
   +  /**
   +   * Gets the default region.
   +   *
   +   * @return string
   +   *   The default region for this display.
   +   */
   +  public function getDefaultRegion();
   

   Not a big deal, but could there be an explanation of what it means for a region to be the "default" one?

6. +++ b/core/modules/field_layout/src/Display/EntityDisplayWithLayoutInterface.php
   @@ -0,0 +1,67 @@
   +   * Gets the layout ID for this display.
   

   Should probably say "layout plugin ID", for clarity.

7. +++ b/core/modules/field_layout/src/Display/EntityDisplayWithLayoutInterface.php
   @@ -0,0 +1,67 @@
   +  public function setLayoutId($layout_id, array $layout_settings = []);
   

   Forgive my ignorance, but what is the difference between this method and setLayout()? The presence of $layout_settings means this method is capable setting more than just the layout ID. Under what circumstances would I call this method versus setLayout()?

8. +++ b/core/modules/field_layout/src/Display/EntityDisplayWithLayoutInterface.php
   @@ -0,0 +1,67 @@
   +  public function getLayoutPlugin();
   

   So setLayout() accepts a LayoutInterface, and this method returns a LayoutInterface, which implies that they are inverses of each other. Yet they are named differently (one uses the word "plugin", the other does not). Why the inconsistency?

9. +++ b/core/modules/field_layout/src/Entity/FieldLayoutEntityDisplayTrait.php
   @@ -0,0 +1,106 @@
   +/**
   + * Provides shared code for entity displays.
   + */
   

   I wish this were a little more descriptive. What is the purpose of the shared code it's providing?

10. +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityDisplayFormTrait.php
    @@ -0,0 +1,205 @@
    +      '#options' => $this->layoutPluginManager->getLayoutOptions(),
    

    I'm not a huge fan of the fact that the trait is implicitly trusting the developer to inject the layout plugin manager, but I can live with it.

11. +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityDisplayFormTrait.php
    @@ -0,0 +1,205 @@
    +  /**
    +   * Ajax callback for the field layout settings form.
    +   */
    +  public static function settingsAjax($form, FormStateInterface $form_state) {
    +    return $form['field_layouts']['settings_wrapper'];
    +  }
    +
    +  /**
    +   * Submit handler for the non-JS case.
    +   */
    +  public function settingsAjaxSubmit($form, FormStateInterface $form_state) {
    +    $form_state->set('layout_plugin', NULL);
    +    $form_state->setRebuild();
    +  }
    

    Parameters are not documented, and $form can be type hinted.

12. +++ b/core/modules/field_layout/tests/modules/field_layout_test/src/Plugin/Layout/FieldLayoutTestPlugin.php
    @@ -0,0 +1,48 @@
    + *   config_dependencies = {
    + *     "module" = {
    + *       "dependency_from_annotation",
    + *     },
    + *   },
    

    Never seen this before. What is it?

13. +++ b/core/modules/field_layout/tests/src/Kernel/FieldLayoutEntityDisplayTest.php
    @@ -0,0 +1,196 @@
    +   * @param mixed $expected
    +   * @param \Drupal\Core\Entity\EntityInterface $entity
    +   * @param string $message
    

    Missing descriptions.

14. +++ b/core/modules/field_layout/tests/src/Kernel/FieldLayoutEntityDisplayTest.php
    @@ -0,0 +1,196 @@
    +  public static function assertEntityValues($expected, EntityInterface $entity, $message = '') {
    

    Not a problem, really, but why is this public and static?

Looks good to me. Back to RTBC with you!

1. Has #88 been addressed? I see @tim.plunkett asking @dawehner for a clarification but no followup thereafter.
2. #99 sounds like it would be a lot of work even as a followup; maybe something we could introduce as a best practice and deprecate old names. Out of scope for this initiative I think, unless I'm missing something. @swentel, maybe you could file a separate issue for that?
3. @phenaproxima, thanks for the review! In the future please do put the issue back to NR or NW for something that's more than one typo. You can always RTBC it again after the patch author answers your questions or fixes the typo. :) And you are doing committers a favor by getting nitpicks out of the way so they don't interfere with our reviews when we do them.
4. Thanks @jibran for listing out the proposed names. Can we put a summary of the names that were considered and ruled out in the summary? One thing we've discovered with experimental modules is that renaming and moving them is really hard, so we should try to get a name that we feel good about on the first go. For what it's worth, I think Field Layout is a good name; it communicates to me exactly what the module does.
5. @aspilicious, do you need a chance to do a more thorough review with DS?
6. Finally, after thinking about this over the weekend, I think it's a good idea to assign this to @amateescu directly to give him a chance to sign off for the subsystem. (Per our governance, subsystem maintainers should be given the opportunity to sign off if they choose, but if the subsystem maintainer doesn't give feedback it can be escalated back to committers.) @amateescu can also choose to just untag "Needs subsystem maintainer review" and unassign if he has no concerns or doesn't have the chance to do a full review.

I plan on doing a more thorough review of this soon if possible, but I definitely think the issue needs to wait for product manager signoff. Meanwhile, NR for points 1, 5, and 6 above.

Thanks everyone!

Oh, one more thing. Was the point about plugin namespacing for this module from #91 point 1 addressed? That's also part of "try to get the name right" and one of the things we struggle most with.

Can we also add a screenshot of the UI? This would probably help product manager review.

@xjm

#99 sounds like it would be a lot of work even as a followup; maybe something we could introduce as a best practice and deprecate old names. Out of scope for this initiative I think, unless I'm missing something. @swentel, maybe you could file a separate issue for that?

Well, tim answered in #100 - I was more thinking out loud here. Since Tim has more knowledge of the form system, I'm fine with this :)

Adding one of the UI screenshots to the summary.

A couple annotated screenshots to show how it works.

The patch looks really awesome to me! I have to say that I did not have time to do a very thorough review, but this is an experimental module after all so I think it's ok to go with what we have :)

I just found two things and probably the first point needs to addressed before commit:

1. +++ b/core/modules/field_layout/field_layout.install
   @@ -0,0 +1,26 @@
   +function field_layout_install() {
   +  // Save each entity display in order to trigger ::preSave().
   +  $entity_save = function (EntityInterface $entity) {
   +    $entity->save();
   +  };
   

   I assume there's some code in the overridden entity view|form display classes that adds something to the config entity, should we also have an uninstall hook to remove that information?

2. +++ b/core/modules/field_layout/src/FieldLayoutBuilder.php
   @@ -0,0 +1,125 @@
   +    $non_configurable_fields = array_filter($field_definitions, function (FieldDefinitionInterface $field_definition) use ($display_context) {
   +      return !$field_definition->isDisplayConfigurable($display_context);
   +    });
   +    // Remove non-configurable fields.
   +    $components = array_diff_key($components, $non_configurable_fields);
   

   This variable name (and comment) is a bit confusing, since we usually refer to "configurable fields" as the ones that use dedicated tables to store their data (i.e. the ones that are handled by FieldConfig and FieldStorageConfig).

   So my initial reaction to this code was: "wait, why are we removing base fields?" :)

Nice change. Much clearer. Go back to the RTBC whence thou came!

Thanks @amateescu and @tim.plunkett.

Some parts of #107 and #108 are still not addressed as far as I can see.

I guess mostly just #108 and whether or not DS is good to go. Edit: I assume it is if @swentel is okay with it, so really just #108.

This look good to me. #108 seems to be addressed and #88 which it references.

Is there a testbot issue for the PHP 5.5 test failures? I'm getting the same test results in #2815221: Add ability to use Quick Edit to the latest_revision route.

We discussed this with @Bojhan, @yoroy, @Dries, and @Gábor today on the Ideas Queue review meeting, and were infinitely ecstatic about this patch going in. :) It fits a great need, and the implementation works as advertised.

Our recommendations were:

1) Adding more layouts to choose from (this is handled by #2840832: Add layout templates based on Panels layouts to core, and has become a "must-have" in the meta issue). This should go in *after* this patch, however, since it could be prone to bikeshedding. :P And unlike this patch, it can go in any time before RC.