Problem/Motivation

Expand the Field UI to switch between two layouts: one column and two column (left/right)
Since this issue depends on #2296423: Implement layout plugin type in core, modules and themes will have the ability to provide new layouts.

Proposed resolution

Add an experimental module named Field Layout.
It enhances Entity Display entities to store a layout, and enhances the Field UI to expose those settings.

Remaining tasks

  1. #175
  2. xjm's question about the layout machine name mismatch in #180. (It might be fine.)
  3. A change record.
  4. Final framework management signoff.
  5. Anything that comes up in fixing #175 or in final framework manager review.
  6. Agree on whether it's okay to discuss whether this code will eventually belong to entity displays in a followup, and add an item to the roadmap if so.

User interface changes

Previously the Field UI showed all fields either in a "Content" region, or as "Disabled".
This will remain unchanged by default, additional layouts are opt-in.
There is an details element at the bottom to allow switching between layouts.

API changes

N/A

Data model changes

N/A, field_layout uses third party settings to store the data

CommentFileSizeAuthor
#193 2796173-field_layout-193.patch71.07 KBtim.plunkett
#193 reorganize-tests.txt8.19 KBtim.plunkett
#193 2796173-field_layout-193-interdiff.txt9.07 KBtim.plunkett
#191 2796173-field_layout-191-interdiff.txt5.06 KBtim.plunkett
#191 2796173-field_layout-191.patch69.44 KBtim.plunkett
#190 2796173-field_layout-190-interdiff.txt3.63 KBtim.plunkett
#190 2796173-field_layout-190.patch68 KBtim.plunkett
#189 2796173-field_layout-189-interdiff.txt2.83 KBtim.plunkett
#189 2796173-field_layout-189.patch67.07 KBtim.plunkett
#187 2796173-field_layout-187.patch66.98 KBtim.plunkett
#187 2796173-field_layout-187-interdiff-from-127.txt34.96 KBtim.plunkett
#181 2796173-field_layout-181-interdiff.txt6.44 KBtim.plunkett
#181 2796173-field_layout-181-PASS.patch66.98 KBtim.plunkett
#181 2796173-field_layout-181-FAIL.patch67.23 KBtim.plunkett
#159 2796173-field_layout-159-interdiff.txt877 bytestim.plunkett
#159 2796173-field_layout-159.patch66.95 KBtim.plunkett
#157 2796173-field_layout-157.patch66.96 KBtim.plunkett
#157 2796173-field_layout-157-interdiff.txt16.45 KBtim.plunkett
#154 2796173-field_layout-154-interdiff.txt3.47 KBtim.plunkett
#154 2796173-field_layout-154-PASS.patch67.34 KBtim.plunkett
#154 2796173-field_layout-154-FAIL.patch67.04 KBtim.plunkett
#150 2796173-field_layout-150.patch66.09 KBtim.plunkett
#150 2796173-field_layout-150-interdiff.txt15.55 KBtim.plunkett
#127 2796173-field_layout-127.patch65.35 KBtim.plunkett
#127 2796173-field_layout-127-interdiff.txt9.88 KBtim.plunkett
#116 2796173-field_layout-116.patch64.44 KBtim.plunkett
#116 2796173-field_layout-116-interdiff.txt1.25 KBtim.plunkett
#113 assign_region.png115.43 KBxjm
#113 select_layout.png116.82 KBxjm
#111 2796173-field_layout-111.patch64.43 KBtim.plunkett
#111 2796173-field_layout-111-interdiff.txt1.69 KBtim.plunkett
#111 Screen Shot 2016-12-12 at 9.59.33 AM.png358.1 KBtim.plunkett
#111 Screen Shot 2016-12-12 at 9.59.13 AM.png344.19 KBtim.plunkett
#111 Screen Shot 2016-12-12 at 9.58.34 AM.png381.06 KBtim.plunkett
#111 Screen Shot 2016-12-12 at 9.58.11 AM.png340.6 KBtim.plunkett
#105 2796173-field_layout-105.patch64.38 KBtim.plunkett
#105 2796173-field_layout-105-interdiff.txt10.02 KBtim.plunkett
#94 screenshot-d8.dev 2016-12-10 03-17-41.png127.09 KBjibran
#94 screenshot-d8.dev 2016-12-10 03-17-21.png438.15 KBjibran
#93 2796173-field_layout-93.patch64.44 KBtim.plunkett
#92 2796173-field_layout-92-interdiff.txt12.1 KBtim.plunkett
#92 2796173-field_layout-92-do-not-test.patch64.44 KBtim.plunkett
#92 2796173-field_layout-92-combined.patch129.48 KBtim.plunkett
#87 2796173-field_layout-87-interdiff.txt6.74 KBtim.plunkett
#87 2796173-field_layout-87-combined.patch119.24 KBtim.plunkett
#87 2796173-field_layout-87-do-not-test.patch55.93 KBtim.plunkett
#84 2796173-field_layout-84-combined.patch119.32 KBtim.plunkett
#84 2796173-field_layout-84-do-not-test.patch56.01 KBtim.plunkett
#81 2796173-field_layout-81-do-not-test.patch55.32 KBtim.plunkett
#81 2796173-field_layout-81-combined.patch198.16 KBtim.plunkett
#78 2796173-field_layout-78-interdiff.txt17.49 KBtim.plunkett
#78 2796173-field_layout-78-combined.patch190.14 KBtim.plunkett
#78 2796173-field_layout-78-do-not-test.patch55.18 KBtim.plunkett
#74 2796173-field_layout-74.patch79.53 KBtim.plunkett
#74 2796173-field_layout-74-interdiff.txt1.95 KBtim.plunkett
#72 2796173-field_layout-72.patch66.61 KBtim.plunkett
#72 2796173-field_layout-72-interdiff.txt31.69 KBtim.plunkett
#71 2796173-field_layout-71-interdiff.txt24.81 KBtim.plunkett
#71 2796173-field_layout-71.patch58.38 KBtim.plunkett
#70 2796173-field_layout-70.patch57.58 KBtim.plunkett
#70 2796173-field_layout-70-interdiff.txt1.42 KBtim.plunkett
#68 2796173-field_layout-68.patch57.58 KBtim.plunkett
#68 2796173-field_layout-68-interdiff.txt17.59 KBtim.plunkett
#66 2796173-field_layout-66.patch47.26 KBtim.plunkett
#66 2796173-field_layout-66-interdiff.txt3.72 KBtim.plunkett
#64 interdiff-2796173-61-64.txt435 bytessamuel.mortenson
#64 2796173-field_layout-64.patch46.75 KBsamuel.mortenson
#61 2796173-field_layout-61.patch46.62 KBtim.plunkett
#61 2796173-field_layout-61-interdiff.txt8.81 KBtim.plunkett
#57 2796173-field_layout-57.patch46.74 KBtim.plunkett
#57 2796173-field_layout-57-interdiff.txt5.35 KBtim.plunkett
#54 2796173-field_layout-54-interdiff.txt1.63 KBtim.plunkett
#54 2796173-field_layout-54.patch45.39 KBtim.plunkett
#52 2796173-field_layout-52.patch45.23 KBtim.plunkett
#52 2796173-field_layout-52-interdiff.txt4.79 KBtim.plunkett
#51 2796173-field_layout-51.patch46 KBtim.plunkett
#51 2796173-field_layout-51-interdiff.txt10.52 KBtim.plunkett
#44 2796173-field_layout-44-interdiff.txt3.5 KBtim.plunkett
#44 2796173-field_layout-44.patch41.96 KBtim.plunkett
#42 2796173-field_layout-42.patch41.44 KBtim.plunkett
#42 2796173-field_layout-42-interdiff.txt11.13 KBtim.plunkett
#40 2796173-field_layout-40-do-not-test.patch41.05 KBtim.plunkett
#40 2796173-field_layout-40-combined.patch110.04 KBtim.plunkett
#40 2796173-field_layout-40-interdiff.txt7.8 KBtim.plunkett
#39 2796173-field_layout-39-interdiff.txt17.87 KBtim.plunkett
#39 2796173-field_layout-39-do-not-test.patch42.16 KBtim.plunkett
#39 2796173-field_layout-39-combined.patch111.15 KBtim.plunkett
#37 interdiff-2796173.txt3.32 KBtim.plunkett
#37 2796173-field_layout-37-combined.patch110.06 KBtim.plunkett
#37 2796173-field_layout-37-do-not-test.patch39.73 KBtim.plunkett
#36 2796173-field_layout-36-do-not-test.patch39.05 KBtim.plunkett
#36 2796173-field_layout-36-combined.patch104.75 KBtim.plunkett
#36 interdiff-2796173.txt1010 bytestim.plunkett
#33 2796173-field_layout-33-do-not-test.patch39.05 KBtim.plunkett
#33 2796173-field_layout-33-combined.patch104.73 KBtim.plunkett
#30 interdiff-2796173.txt11.79 KBtim.plunkett
#30 2796173-field_layout-30.patch43.04 KBtim.plunkett
#28 interdiff-2796173.txt6.1 KBtim.plunkett
#28 2796173-field_layout-28.patch40.66 KBtim.plunkett
#27 interdiff-2796173.txt9.37 KBtim.plunkett
#27 2796173-field_layout-27.patch37.84 KBtim.plunkett
#25 2796173-field_layout-25.patch33.21 KBtim.plunkett
#25 interdiff-2796173.txt4.94 KBtim.plunkett
#22 2796173-field_layout-22.patch35.22 KBtim.plunkett
#22 interdiff-2796173.txt4.43 KBtim.plunkett
#18 interdiff-2796173.txt5.62 KBtim.plunkett
#18 2796173-field_layout-17.patch34.41 KBtim.plunkett
#15 2796173-field_layout-15.patch31.28 KBtim.plunkett
#15 interdiff-2796173.txt7.64 KBtim.plunkett
#12 interdiff-2796173.txt1.35 KBtim.plunkett
#12 2796173-field_layout-12.patch30.51 KBtim.plunkett
#9 interdiff-2796173.txt16.86 KBtim.plunkett
#9 2796173-field_layout-9.patch30.29 KBtim.plunkett
#6 interdiff-2796173.txt11.54 KBtim.plunkett
#6 2796173-field_layout-6.patch19.7 KBtim.plunkett
#5 interdiff-2796173.txt18.73 KBtim.plunkett
#5 2796173-field_layout-5.patch17.79 KBtim.plunkett
#4 interdiff-2796173.txt9.7 KBtim.plunkett
#4 2796173-field_layout-4.patch13.31 KBtim.plunkett
#2 2796173-field_layout-2.patch10.44 KBtim.plunkett
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

This does not let you switch between layouts, but it changes it from a single "Content" region to having "Top" and "Bottom".

More to come.

larowlan’s picture

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?

tim.plunkett’s picture

1) Honestly, it's much less code than I thought, but it's also not done. It really depends on the scoping of UX/workflow considerations, will bring it up with the UX team.

2) I think we'll leave it pretty close to this until #2296423: Implement layout plugin type in core, I'd rather only introduce one API. Another reason to make this experimental.

3) Could be, but this is just while it's experimental. Once it moves into Field UI, we can just add it directly where it belongs, no slicing or splicing needed.

Manage display should be working fine now. Going to start on Manage form display next, and also tests.

tim.plunkett’s picture

swentel’s picture

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)

tim.plunkett’s picture

A lot of this code was inspired by DS, but I did take into account how Panels does things too, as I'm more used to that. The code should look familiar to both camps though.

As this is the fifth experimental module I've worked on (of the ten that exist), I'm getting pretty comfortable with the speed of iteration and ability to make changes during the experimental phase. Also, I've learned not to try to do too much in the first pass. This scope is not aggressive, but I think it is efficient.

1) Yes, this absolutely needs to change before commit.
2) I needed to loop over everything in order to handle real fields and extra fields. There may be a better way to do this, I need to play around with it once there is test coverage.

I picked "Field Layout" because I needed something to get started. I also remember the discussion about renaming Field UI to Entity UI (or Entity Field UI?), not 100% sure that "Entity Layout" is a better name, but I'm absolutely open to changing it.

Thanks for the review!

tim.plunkett’s picture

Status: Active » Needs review
FileSize
30.29 KB
16.86 KB

I ended up adding a service like @larowlan suggested, while waiting for layout plugin to be integrated the list can now be swapped out.
Spent most of the day learning JavascriptTestBase. Turns out that if everything is broken, just add $this->assertSession()->assertWaitOnAjaxRequest(); and it will all start working...

swentel’s picture

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)

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
30.51 KB
1.35 KB

Integrating this with layout_plugin is the number 1 priority. But without something in core (like this) to use it, adding layout_plugin by itself probably won't happen. So hopefully this makes that happen sooner.

Fixing the hook_help. Not making a d.o handbook page until we settle on a name.

Copying this from the parent issue:

In the child issue, I've started on an experimental module called "Field Layout", which is just what I picked to get started.

@swentel suggested "Entity Layout", since we're not configuring the layout of the fields, but of the entity.
@Wim Leers further pointed out that we're dealing with entity display objects, so "Entity Display Layout" would be more accurate.

Any objection to calling the experimental module "Entity Display Layout"? Note that this name is just for the experimental phase, it would ideally be subsumed by the Entity Display system and Field UI once stable, there would not be a stable standalone module.

jibran’s picture

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.

tim.plunkett’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update

Need to expand the issue summary, but I've filled in the remaining tasks.

tim.plunkett’s picture

Fixed a bug found while writing tests (yay!) and removed the dependency on field_ui.

swentel’s picture

+++ 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.

tim.plunkett’s picture

Issue summary: View changes

Wrote a hook_install, as well as a hook_entity_presave() so that newly added entity displays get default values.

tim.plunkett’s picture

Attaching files is good.

dawehner’s picture

  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 :)

tim.plunkett’s picture

1) Yes, it does. Looking into how to better accomplish this.

2) Hah it does feel that way. But that's how recently committed traits have been documented, since they cannot use {@inheritdoc}.

Working on the test fail, \Drupal\config\Tests\ConfigImportAllTest is the worst.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
35.22 KB

Posting a new patch with an attempt at hook_uninstall. Not sure if that fixes it, having issues running CIAT locally.

Also cleaned up some of the rendering code, and added assertions to prove it all works after you uninstall the Field UI.

swentel’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.94 KB
33.21 KB

After countless 15 minute local test runs of ConfigImportAllTest, I figured it out.
a) hook_uninstall was not needed.
b) The $entity->isNew() check was not sufficient. Something must have been messing with that elsewhere. Switching to check for the presence of our settings works.
c) The code to find all the fields was WAY overkill. Just stealing the approach from hook_install, works just as well.

Additionally, I realized that I was writing hook_install like it was hook_update_N. But the Entity API is allowed to be used in hook_install! So it becomes much simpler.

tim.plunkett’s picture

Issue summary: View changes

@jibran clarified on IRC that he misunderstood what I was dragging. Since this patch only touches the custom tabledrag of Regions, and not of Weight, not adding test coverage for that.

tim.plunkett’s picture

Updated the test to use entity_type where possible.
Also added coverage for entity form displays.

tim.plunkett’s picture

Finished the tests.

tim.plunkett’s picture

Removed the pointless 1col_stacked layout.
Renamed 2col to twocol, since Html::cleanCssIdentifier() strips the leading number.
Allowed layouts to provide a library, and added a basic CSS file for twocol.
Wrapped the regions in a layout template.

yoroy’s picture

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!

tim.plunkett’s picture

This is now hard blocked by #2796581: Fields must store their region in entity displays.
It incorporates the patch from that issue, which is why it's so big.

@yoroy I think webchick is working on a separate UI issue, it will be linked from here. Glad to hear excitement!

tim.plunkett’s picture

tim.plunkett’s picture

#2796581: Fields must store their region in entity displays is now RTBC.

This fixes field_layout.install, and addresses @dawehner's #20.1 by using #group for forms.

tim.plunkett’s picture

Reroll and fixed bugs.

  • Reintroduce EntityDisplayWithLayoutInterface but as experimental
  • Swapping out Entity Display classes is not dependent on Field UI.
  • Regions need group processing code.
  • Thanks PHP <7.0.5
tim.plunkett’s picture

Moved to a single template file instead of nesting region templates in a layout. This was requested by themers, is more similar to Display Suite, and is what Layout Plugin would also expect.

tim.plunkett’s picture

Blocker was committed!
Moving the code more towards layout_plugin-esque code.

tim.plunkett’s picture

Unfortunately, by switching from nested #theme_wrappers to a dedicated single #theme, we're now hitting #953034: [meta] Themes improperly check renderable arrays when determining visibility.
Adding a workaround for now.

570f95a Restore proper 'default' handling
365c466 Restore markup to default template.
60c5a51 Add workaround for #953034

tim.plunkett’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
10.52 KB
46 KB

Fixed the bugs, and allow templates to be placed arbitrarily

tim.plunkett’s picture

Needed to change getLayout() to remove the default value handling, there are other cases where we need to be able to check if something has a layout or not.

I think this is very close. Code reviews would be very welcome!

dawehner’s picture

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?

tim.plunkett’s picture

1) Very true :)
2) @swentel had the same thought in #16 but asked that it be a follow-up.
3) I also considered giving it support for multiple, but you could just write one library with multiple dependencies. Also this is how layout_plugin does it.
4) Non-configurable fields should not be moved, as they are usually handled by something like preprocess (see template_preprocess_node moving everything around for submitted by).
5) Once they become plugins, they will automatically be assigned a 'provider' by the plugin system, so I'm hardcoding it to mimic that.
6) Well we just do $definition['template'] = strtr($definition['theme'], '_', '-'); if you provide 'theme' but no 'template', so I'm not sure it matters.
7) Yes it is. Adding an @todo.
8) It doesn't need it, but should I make a separate BTB just for this method?

dawehner’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.35 KB
46.74 KB

array_walk fails because it takes the array by reference.
I realize array_map is slightly less semantically correct, but the docs do say

Applies the callback to the elements of the given arrays

and that's precisely what I'm doing. I don't think the fact that we don't care about the return value makes its usage any less valid.

dawehner’s picture

Interesting. Oh PHP

tim.plunkett’s picture

Issue summary: View changes

Dries used the name "Field Layout" in his keynote, and nothing came of the other suggestions in the parent issue.

samuel.mortenson’s picture

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.

tim.plunkett’s picture

1) Done. That was copied from DS.
2) Done.
3) I copied the CSS from D8 Panels (it uses flexbox). I can go back to the D7 float/width approach.
4) That's just Field UI being Field UI. It's nothing we're doing here.
5+6) These suggestions are opposites :)
5 is saying "instead of altering, swap out the entity handler"
6 is saying "instead of swapping out the entity handler, use an alter"

I'm quite happy with how the class/form swaps are working in this case. And I think swapping out EVERY entity view builder is a mistake, since only one module can do that...

samuel.mortenson’s picture

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.

tim.plunkett’s picture

samuel.mortenson’s picture

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).

tedbow’s picture

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

tim.plunkett’s picture

Thanks @samuel.mortenson!

#65
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.
2) I did clean this logic up, thanks. Feels good to stop cheating with get()
3) Good point!

dawehner’s picture

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.

tim.plunkett’s picture

Fair enough. Converted, wrote unit test, refactored.
Now with the exception of _field_layout_entity_display_presave(), all of field_layout.module is hook implementations

dawehner’s picture

Nice!

tim.plunkett’s picture

Just fixing two coding standards nits that were bugging me.

tim.plunkett’s picture

Started in on the conversion for #2296423: Implement layout plugin type in core, and found a couple changes that can be made now to minimize future changes.

tim.plunkett’s picture

Okay the conversion to layout_plugin was actually very easy and not as big as I'd thought.
After discussion with @tedbow and @xjm, changing tack.

Now that we know this code will work for field_layout, we'll try to get layout_plugin in on its own first. Any changes to that code will be reflected here to ensure it continues working.

I will also run Panels and DS tests locally with this code to ensure it continues to work.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.95 KB
79.53 KB
dsnopek’s picture

+++ 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!

tim.plunkett’s picture

#2796581: Fields must store their region in entity displays was reverted, so all of this is postponed.

tim.plunkett’s picture

Status: Needs review » Postponed
tim.plunkett’s picture

Posting a combined patch of this, #2796581: Fields must store their region in entity displays, and #2296423: Implement layout plugin type in core.
Interdiff only includes changes made to field_layout code, not the updates from the other issue, so it's not a true interdiff.
Will mark postponed again after the tests pass (or don't?!)

tim.plunkett’s picture

Status: Needs review » Postponed
Issue tags: +Needs issue summary update

Great

tim.plunkett’s picture

Title: Add ability for entity view modes to switch between two hard-coded layouts » Add experimental Field Layout module to allow entity view/form modes to switch between layouts
Issue summary: View changes
Issue tags: -Needs issue summary update
tim.plunkett’s picture

Posted an updated patch. No explicit changes to field_layout, just updating based on changes in the blocking issues.

tim.plunkett’s picture

Status: Needs review » Postponed

Working on the fails in a test-only issue.

tim.plunkett’s picture

One blocker down, just layout_plugin left. The last fail was in layout_plugin.

tim.plunkett’s picture

Status: Needs review » Postponed
tedbow’s picture

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

tim.plunkett’s picture

Thanks for the review!

1) It's "per view mode" elsewhere in core, going with that.
2) +1
3) Done

1) Fixed
2+3) Hah, those were copy/paste from HEAD :) fixed

Unnumbered) Fixed

dawehner’s picture

  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

tim.plunkett’s picture

#88.1, do you mean separate classes for form and view?

tacituseu’s picture

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 ?

tim.plunkett’s picture

1) Yes, every module is responsible for namespacing their plugin IDs, neither Panels nor this experimental module do that yet.

2) I'll have to look at that, the dependency system is supposed to prevent that from happening.

tim.plunkett’s picture

Still postponed, but updating for recent changes to the layout API.
This should address #90.2

tim.plunkett’s picture

We're now unblocked! Layout API is now in core.

jibran’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
438.15 KB
127.09 KB

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

xjm’s picture

Priority: Normal » Major
xjm’s picture

@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.

xjm’s picture

Category: Task » Feature request

Feature request really, but definitely a major one.

xjm’s picture

Thanks @jibran for the screenshots. That is great.

swentel’s picture

+++ 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 ?

tim.plunkett’s picture

One line down from that is

  if ($form_object instanceof ContentEntityFormInterface && $display = $form_object->getFormDisplay($form_state)) {

It's the first condition in order to filter out that as soon as possible.
There is no mechanism for finding and altering all content entity forms.

Base form IDs for entity forms usually contain the entity type ID (like node, user) but can also be completely arbitrary in custom code.

jibran’s picture

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.
aspilicious’s picture

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)

tim.plunkett’s picture

They work the same as they do before. So node works, but user doesn't (because it doesn't print title_prefix/title_suffix).
This has no effect on any of that, because the layout represents the {{ content }} part of the entity template.

phenaproxima’s picture

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?

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.02 KB
64.38 KB

1) Fixed

2) Discussed this with @dawehner before, IMO makes more sense to leave it this way.

3) form.api.php defines this as function hook_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {, so not adding a typehint.

5) This is actually just making a protected method into public one, the docs are copied from \Drupal\Core\Entity\EntityDisplayBase::getDefaultRegion()

6) Fixed

7)
setLayout() is a shortcut method:

  public function setLayout(LayoutInterface $layout) {
    $this->setLayoutId($layout->getPluginId(), $layout->getConfiguration());
    return $this;
  }

setLayoutId() is used when you don't have a whole plugin object. For example,

  public function preSave(EntityStorageInterface $storage) {
    if (!$this->getLayoutId()) {
      $this->setLayoutId('onecol');
    }

8) Leftover from many many rounds of refactoring. Fixed.

9) It's a trait. EntityViewDisplay and EntityFormDisplay both have shared code via a base class, EntityDisplayBase. Due to how PHP inheritance works, we have to rely on a trait here. This code will eventually be moved into the base class.

10) The protected property is defined on the trait, it's safe enough.

11) These are standard callbacks. The docs standard says

They are documented without parameters or return value documentation, since the parameters and return value are standard for that type of callback.

That's why it starts with "Ajax callback / Submit handler" instead of the usual format.

12) They're config dependencies specified by a plugin. If you grep or google for it, you'd see it is documented on \Drupal\Core\Config\Entity\ConfigDependencyManager.

13) It's a test method, the @param is generally enough.

14) All PHPUnit assert methods are public and static.


NR for the method name change.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Back to RTBC with you!

xjm’s picture

Assigned: Unassigned » amateescu
Status: Reviewed & tested by the community » Needs review
  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!

xjm’s picture

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.

xjm’s picture

Issue tags: +Needs screenshots

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

swentel’s picture

@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 :)

tim.plunkett’s picture

#88

1) @dawehner and I discussed this on IRC, the shared logic of FieldLayoutBuilder outweighs the benefits of two classes IMO.
2) Changed
3) Expanded the comment
4) Thanks!

Included are some screenshots. First what's in HEAD, the selector, then with a 2 column layout with no settings, then with a test layout with settings.

xjm’s picture

Issue summary: View changes

Adding one of the UI screenshots to the summary.

xjm’s picture

A couple annotated screenshots to show how it works.

xjm’s picture

Issue summary: View changes
amateescu’s picture

Assigned: amateescu » Unassigned
Status: Needs review » Needs work
Issue tags: -Needs subsystem maintainer review

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?" :)

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
64.44 KB

1) Exactly, the overridden entity class has this:

  public function preSave(EntityStorageInterface $storage) {
    if (!$this->getLayoutId()) {
      $this->setLayoutId('onecol');
    }

Because the storage mechanism is third party settings, this is cleaned up automatically on uninstall by \Drupal\Core\Config\Entity\ConfigEntityBase::onDependencyRemoval()
We have coverage of this from the beloved \Drupal\config\Tests\ConfigImportAllTest

2) Upon rereading that code, I agree it is confusing.
Hopefully renaming the variable and moving the comment to where the decision is made will improve clarity.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

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

tim.plunkett’s picture

Random fail in Drupal\system\Tests\Update\SiteBrandingConvertedIntoBlockUpdateTest. Requeued, and re-RTBC'd

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
xjm’s picture

Status: Reviewed & tested by the community » Needs review

Thanks @amateescu and @tim.plunkett.

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

xjm’s picture

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.

tim.plunkett’s picture

When I went to address #108 I realized how many places had 'onecol' hardcoded. All of them were just to work around the layout not being set in time for init(), so I shared the logic from preSave() to cover it.
Then I prefixed the plugin IDs.
Also changed the arbitrary test IDs to be different from the real ones to be less confusing.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

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

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

Testbot fluke

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
tim.plunkett’s picture

Status: Needs work » Needs review

This is still really RTBC, but the php 5.5 bots are nonfunctional. Queued it with 5.6 to prove it passed.
Leaving a NR to stop the false fails.

samuel.mortenson’s picture

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.

tim.plunkett’s picture

Hoping testbot is better now?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

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.

2) Following up from #1, improving the selection of layouts so people have some idea (ideally a visual representation) of what "3 column stacked" vs. "2 column blocks" means.

Removing the product manager review tag. SHIP IT! :D

xjm’s picture

And unlike this patch, it can go in any time before RC.

To clarify, once the experimental module is added as alpha, it can have feature additions during any phase of the release cycle (including RC and patch releases), until it reaches beta stability.

tim.plunkett’s picture

#2660124: Dynamically build layout icons based on well formed config is intended to address #138.2, and is listed on and linked from the plan issue

xjm’s picture

I scanned the patch, not including the test coverage. A few questions and observations. I tried to clarify whether they're in scope or not.

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

    UX gate followup: I think this label could probably be improved. Also, non-blocking nit: I believe this should be "Per-view-mode field layout settings".

  2. +++ b/core/modules/field_layout/field_layout.info.yml
    @@ -0,0 +1,8 @@
    +description: 'Adds layout capabilities to the Field UI.'
    

    UX gate followup: Review/improve the module description.

  3. +++ 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/field_layout']) . '</p>';
    

    Docs gate followup: This handbook page needs to not be totally empty. ;)

  4. +++ b/core/modules/field_layout/layouts/onecol/field-layout--onecol.html.twig
    @@ -0,0 +1,24 @@
    + * Default theme implementation to display a one column layout.
    
    +++ b/core/modules/field_layout/layouts/twocol/field-layout--twocol.html.twig
    @@ -0,0 +1,28 @@
    + * Default theme implementation to display a two column layout.
    

    Non-blocking nit: "one-column layout", "two-column layout".

  5. +++ b/core/modules/field_layout/layouts/onecol/field-layout--onecol.html.twig
    --- /dev/null
    +++ b/core/modules/field_layout/layouts/twocol/field-layout--twocol.html.twig
    
    +++ b/core/modules/field_layout/layouts/twocol/field-layout--twocol.html.twig
    --- /dev/null
    +++ b/core/modules/field_layout/layouts/twocol/twocol.layout.css
    

    Followup: If we don't get a frontend framework review before commit, then it should probably be a followup on the roadmap specifically for this module, the same way we have one for the overall theme implementation for the layout API itself.

  6. +++ b/core/modules/field_layout/src/Entity/FieldLayoutEntityDisplayTrait.php
    @@ -0,0 +1,132 @@
    + * Provides shared code for entity displays.
    
    +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityDisplayFormTrait.php
    @@ -0,0 +1,205 @@
    + * Provides shared code for entity display forms.
    

    Non-blocking nit: What kind of shared code?

  7. +++ b/core/modules/field_layout/src/Entity/FieldLayoutEntityDisplayTrait.php
    @@ -0,0 +1,132 @@
    +trait FieldLayoutEntityDisplayTrait {
    
    +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityDisplayFormTrait.php
    @@ -0,0 +1,205 @@
    +trait FieldLayoutEntityDisplayFormTrait {
    
    +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityFormDisplayEditForm.php
    @@ -0,0 +1,44 @@
    +class FieldLayoutEntityFormDisplayEditForm extends EntityFormDisplayEditForm {
    +
    +  use FieldLayoutEntityDisplayFormTrait;
    
    +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityViewDisplayEditForm.php
    @@ -0,0 +1,44 @@
    +class FieldLayoutEntityViewDisplayEditForm extends EntityViewDisplayEditForm {
    +
    +  use FieldLayoutEntityDisplayFormTrait;
    

    So these traits are pretty massive, especially the form one. What's the reason that they are traits? I guess so that the forms can extend both them and the base class? What's the case for reusing the traits though? (The answer to this might be in the patch somewhere; just struck me as a little unexpected.)

  8. +++ b/core/modules/field_layout/src/Entity/FieldLayoutEntityDisplayTrait.php
    @@ -0,0 +1,132 @@
    +  /**
    +   * Overrides \Drupal\Core\Entity\EntityDisplayBase::init().
    +   */
    ...
    +  /**
    +   * Overrides \Drupal\Core\Entity\EntityDisplayBase::preSave().
    +   */
    

    Non-blocking: I had a kneejerk that these should be {@inheritdoc} but of course it's a trait so it's not inheriting anything. D'we have any examples of this documentation pattern elsewhere?

  9. +++ b/core/modules/field_layout/src/Entity/FieldLayoutEntityDisplayTrait.php
    @@ -0,0 +1,132 @@
    +   * @todo Remove once https://www.drupal.org/node/2821191 is resolved.
    

    Non-blocking nit: Per https://www.drupal.org/node/1354#order @todo should come after @see.

    I confirmed that this @todo is already a "Should" on the overall Layout API roadmap.

  10. +++ b/core/modules/field_layout/src/Entity/FieldLayoutEntityFormDisplay.php
    @@ -0,0 +1,25 @@
    +   * {@inheritdoc}
    +   *
    +   * This cannot be provided by the trait due to https://bugs.php.net/bug.php?id=71414
    +   * which is fixed in PHP 7.0.6.
    
    +++ b/core/modules/field_layout/src/Entity/FieldLayoutEntityViewDisplay.php
    @@ -0,0 +1,25 @@
    +   * {@inheritdoc}
    +   *
    +   * This cannot be provided by the trait due to https://bugs.php.net/bug.php?id=71414
    +   * which is fixed in PHP 7.0.6.
    

    So do we expect a followup to remove it once Drupal requires PHP 7? This might be an academic question, but D9 should have PHP 7 as a minimum requirement.

    Non-blocking nit: I am pretty sure {@inerhitdoc} can't be used with anything else still. There's some issue about this somewhere.

  11. +++ b/core/modules/field_layout/src/FieldLayoutBuilder.php
    @@ -0,0 +1,125 @@
    +   * FieldLayoutBuilder constructor.
    

    Non-blocking nit: "Constructs a blah" is normal I think, redundant and silly as that is.

  12. +++ b/core/modules/field_layout/src/FieldLayoutBuilder.php
    @@ -0,0 +1,125 @@
    +        // If this is a form, #group can be used to relocate the fields. This
    +        // avoids breaking hook_form_alter() implementations by not actually
    +        // moving the field in the form structure.
    

    Non-actionable observation: Neat.

  13. +++ b/core/modules/field_layout/src/FieldLayoutBuilder.php
    @@ -0,0 +1,125 @@
    +   * @param string $display_context
    +   *   The display context, either 'form' or 'view'.
    

    Non-blocking pontification: Should these be constants? Are they elsewhere in the Entity API? Come to think of it, are display contexts extensible?

All the stupid non-blocking docs stuff could be a "Should" have followup like "Clean up dumb docs nitpicks in blah," or incorporated in the patch, as per your preference.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Meant to do this. This is still ready for framework manager review, though.

effulgentsia’s picture

I've started reviewing this and will post more comments today. Just a quick question in the meantime...

+++ b/core/modules/field_layout/field_layout.layouts.yml
@@ -0,0 +1,21 @@
+field_layout_onecol:
+  label: 'One column'
+  category: 'Columns: 1'
+field_layout_twocol:
+  label: 'Two column'
+  category: 'Columns: 2'

Is there an already existing issue to discuss how these get organized? I can imagine various other modules and/or themes implementing layouts labeled "Two column" and in the same category. In which case, the site builder would see multiple options with the same labels, which is confusing. I don't know that #2660124: Dynamically build layout icons based on well formed config would resolve this either, since the icon previews might look very similar or identical as well.

The reason I bring this up during the "framework manager review" is that if these were being added to a non-experimental module, then we could reasonably tell contrib that it's their responsibility to not use the same labels as what's used in a core module. But I don't know if it's as reasonable for contrib authors to monitor experimental modules for that kind of clashing.

Related, why should "Field layout" be the module that owns the "Two column" label name? Seems like it should be possible for me to not enable field layouts, but enable some other kind of layout module (e.g., per-node layouts, or something else in the Panels ecosystem), and still have some layout on the site that's labeled "Two column". But if I do, and then enable field_layout.module, now I have two layouts with that label. Which makes me think that maybe these layouts belong in some other place than field_layout module, so that they can be available to the system even when field_layout module is not enabled? #2840832: Add layout templates based on Panels layouts to core says "Decide where they should live, currently in layout_discovery". Should we therefore move these to layout_discovery too?

tim.plunkett’s picture

Working on @xjm's feedback.

#143, I would fully expect field_layout to ship with zero layouts once stable.
This was discussed during the product manager review, and we chose not to block this issue on adding more layouts and in the correct location.
Another essential issue for this is #2822758: Introduce a way to distinguish between different types of layouts

xjm’s picture

Issue tags: +Needs change record

I think this probably needs a CR (both for the addition of the feature, and for how contrib/custom code might implement or extend it)?

effulgentsia’s picture

This was discussed during the product manager review, and we chose not to block this issue on adding more layouts and in the correct location.

Is there an issue comment about this explaining why? I understand not blocking on more layouts. But why not get the layouts proposed in this patch into the correct location? Moving them later probably also means renaming them from field_layout_* to something else, which means either writing an upgrade path or allowing sites that have this module enabled to break. I propose renaming them to layout_* (is core allowed to claim the "layout" namespace like that or would that break something already existing in contrib?) and moving them to layout_discovery. I could potentially see us wanting to move them from layout_discovery to either system, /core, or somewhere else, but so long as they're namespaced to layout_*, they should be movable without a rename or upgrade path, right?

effulgentsia’s picture

Status: Needs review » Needs work

When I uninstall the module, even though the UI tells me that the various view and form display configs will be updated such that all the data from this module will be lost, that does not actually happen. Those configs continue to have references to the uninstalled module. I suspect that would potentially break any place that checks schema, such as config translation, except I can't test that due to #2546212: [PP-1] Entity view/form mode formatter/widget settings have no translation UI. However, if I export the site config, then make some change to one of the displays on the site, then do an import and sync, I get errors like:
Configuration core.entity_view_display.node.article.teaser depends on the Field Layout module that will not be installed after import.

tim.plunkett’s picture

#146 It's my understanding that when adding this module, the changes should be limited to this module. Also, it's experimental, so I have no thought toward upgrade path if we rename whatever.

#147 This means that there is a generic core bug in the handling of third party settings, which I am surprised by.
\Drupal\Core\Config\Entity\ConfigEntityBase::onDependencyRemoval() is supposed to handle this.

effulgentsia’s picture

+++ b/core/modules/field_layout/src/FieldLayoutBuilder.php
@@ -0,0 +1,125 @@
+   * @param string $display_context
+   *   The display context, either 'form' or 'view'. If in a 'form' context, an
+   *   alternate method will be used to render fields in their regions.
+   */
+  public function build(array &$build, EntityDisplayWithLayoutInterface $display, $display_context) {

Not a blocker for committing this as an experimental module, but the implementation of this function seems very different for 'form' and 'view'. Given that, why make it a single function? Maybe better to have buildForm() and buildView() (or entityFormAlter()/entityViewAlter())?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
15.55 KB
66.09 KB

#141

1 Fixed
4 Fixed
6/7 Improved docs
8 See FormStateValuesTrait and RevisionLogEntityTrait for examples. Only appropriate for traits.
10 Fixed
11 Fixed
13 Removed due to #149

#147 This is covered by \Drupal\system\Tests\Module\InstallUninstallTest and \Drupal\config\Tests\ConfigImportAllTest.

#149 Addressed. This was brought up numerous times and considered okay to have, but I'm tired of defending it.

xjm’s picture

I added bullets for #141 1, 2, 3, and 5 to the summary of #2795833: [plan] Add layouts to entity displays (both form and view). The first part of 10 isn't actually addressed but I assume the answer is "Yes, but it's not worth filing an issue now for something so far off" so I added that as a bullet under "Could".

I think #147 and the upgrade path question probably merit further discussion.

Normally we do not require upgrade paths for alpha modules (and beta is best-effort) but we might choose to given that we also agreed to do so for #2833976: How Panels, Display suite, Layout Plugin, Layout Discovery and core get to a stable layout plugin in core because of the impact on contrib. We can discuss that in a followup and I don't think it needs to block commit for the prototype.

However, #147 could suggest a data integrity problem, which is one of the things that can block the initial commit of an experimental module, so I pinged @alexpott for feedback on it. Leaving at NR for that.

effulgentsia’s picture

It's my understanding that when adding this module, the changes should be limited to this module.

I don't think that's necessarily true. If the development of this module surfaces an architectural requirement, such as the existence of a layout that needs to be available even when this module is not enabled, then I think it's fine for this patch to include that layout in the location that makes most sense. Or, a child issue could be made for adding just that layout.

it's experimental, so I have no thought toward upgrade path if we rename

The problem is that I think a rename is quite likely for the reasons in #143. And not providing an upgrade path could put the site in a pretty bad state. For example, if the site has a global custom block on all admin pages (either in the admin theme, or the site chooses to use the default theme for the admin pages), then as soon as you deploy code with a renamed layout, then after a cache clear, visiting any admin page on your site results in a fatal "The website encountered an unexpected error. Please try again later." error. Which means you can't even uninstall the module via the UI anymore, so even following the instructions in https://www.drupal.org/core/experimental#alpha to uninstall and reinstall is difficult for users without Drush.

Normally we do not require upgrade paths for alpha modules (and beta is best-effort) but we might choose to...We can discuss that in a followup and I don't think it needs to block commit for the prototype.

What if we just rename them in this patch from field_layout_* to layout_* in order to not create avoidable future upgrade path work for ourselves? We can leave them in the field_layout module for now, and maybe add @todo comments to field_layout.layouts.yml explaining why they're not namespaced to the module and a link to the issue for discussing where to move them to. Or do we foresee problems with existing contrib modules if we start using the layout_* namespace for layouts?

tim.plunkett’s picture

the existence of a layout that needs to be available even when this module is not enabled

I don't think we have that need yet.

The product managers and UX team agreed to not block this on the "add more layouts" issue.
If you as a framework manager want to hard-block this issue on that, just do that. But I don't think we should be adding layouts to layout_discovery or purposefully misnamespacing them in this issue.

tim.plunkett’s picture

I finally reproduced #147.

On uninstall, config deletion takes place before the module's hooks stop firing. field_layout_entity_type_alter() is still in effect, which means our FieldLayoutEntityDisplayTrait::preSave() continues to fire, which ensures it has a layout.

Wrote a test for this, and a hacky fix.
A major problem with this fix: there's no way I can see to clean up that state value after we don't need it anymore. This would cause issues on reinstall.

tim.plunkett’s picture

+++ b/core/modules/field_layout/field_layout.module
@@ -0,0 +1,69 @@
+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);
+  }
+}

Ideally this wouldn't take affect during uninstall, but I'm guessing modules rely on such alters still running. And no idea of how to opt out of it (especially since it's cached!)

tim.plunkett’s picture

Okay came up with a more tenable fix.

field_layout_install() no longer depends on preSave() to magically do what we need, it's now handled explicitly.
preSave() can now be much simpler, and we'll let init() handle the rest of the cases.

Also just renamed according to #152, I think that's a reasonable compromise.

Everything since the last time it was RTBC has been addressed.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
66.95 KB
877 bytes

Whew, not a real failure, just missed a spot

effulgentsia’s picture

Status: Needs review » Needs work

I'm liking #159 a lot! Thank you. Uninstallation is much nicer now.

But one problem I'm still seeing is that when you install Standard, then install Field Layout, then uninstall Field Layout, then the node.article.default entity view display gets its comment component moved to hidden. If you then go to /admin/structure/types/manage/article/display to remake comments visible, then after another install/uninstall of Field Layout, it gets again reset to hidden.

I suspect the problem is somewhere within the ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() pipeline, and not within this patch's code. It's just that we have no other place in core that adds third party settings to EVD entities, and this patch is possibly surfacing a general bug with that. However, even if that's true, https://www.drupal.org/core/experimental#requirements lists "No known security or data integrity issues." as a requirement, so setting to NW for that, even if the only fault of this patch is that it's surfacing a data integrity issue in a hitherto unexercised API. Sorry.

effulgentsia’s picture

+++ b/core/modules/field_layout/field_layout.layouts.yml
@@ -0,0 +1,22 @@
+layout_onecol:
...
+layout_twocol:

Yay, thank you for these names :) While #160 is being sorted out, I'd love +1s here from maintainers of 8.x versions of any of the Panels/DS/Layout and related modules. I want to make sure the "layout" namespace for layout IDs is safe for core to claim. IMO, it's correct for core to claim it, because core is where Drupal\Core\Layout is, but if we have existing contrib that would conflict with that, I'd like to make sure we know about it.

effulgentsia’s picture

setting to NW for that, even if the only fault of this patch is that it's surfacing a data integrity issue in a hitherto unexercised API

I'll discuss with the other committers, but possibly the only hard-blocker for this issue might be to verify that it's a generic bug with uninstallation of a module that uses third_party settings, write a test that demonstrates that generic bug, and file a Critical issue with a patch with that failing test. Then potentially, if the release managers agree, fixing that Critical might not need to block this module from being committed.

tim.plunkett’s picture

#161, pinged other people, but there is no Layout module right now (nor plans for one), so this namespace is ours for the taking.

#160/162, the culprit is \Drupal\Core\Entity\EntityDisplayBase::onDependencyRemoval(). Since the comment display is tied to the output of core.entity_view_display.comment.comment.default, it's incorrectly determining that comment is being removed (which isn't true). Looking into how bad the logic bug is, but at least this is isolated within the entity display system.

tim.plunkett’s picture

Status: Needs work » Needs review

Opened #2843074: Stale dependencies passed to onDependencyRemoval() result in data loss on uninstallation. Seems at this time to be isolated to comment fields, specifically when using CommentDefaultFormatter.

japerry’s picture

Re: layout namespace: from the panels side I don't have any issue with this. I don't think panelizer is affected either.

Its possible some sites may have used the namespace to create layout features? Not sure we want to really accommodate for that edge case though.

tim.plunkett’s picture

Turns out that other issue is a pre-existing bug, and has been marked critical.

Then potentially, if the release managers agree, fixing that Critical might not need to block this module from being committed.

Hopefully!

xjm’s picture

#2843074: Stale dependencies passed to onDependencyRemoval() result in data loss on uninstallation does still need to be a blocker, because this module will expose it more. But fortunately it is RTBC so hopefully it will be in soon!

xjm’s picture

Its possible some sites may have used the namespace to create layout features? Not sure we want to really accommodate for that edge case though.

Yeah, there's always the risk of this when we add new code, until the day when core properly namespaces everything. But this is an acceptable risk; the best practice for custom code is also to namespace its stuff. We do need the CR though so that developers are informed (for this and everything).

Thanks @japerry!

xjm’s picture

xjm’s picture

Does #2843074: Stale dependencies passed to onDependencyRemoval() result in data loss on uninstallation address #147 as well as #160, or do we still need the "hacky fix" that was mentioned previously?

tim.plunkett’s picture

#157 removed the hacky fix, and is even better to what we had before that, so the latest patch is still what we want.

xjm’s picture

Thanks @tim.plunkett.

I tested manually and confirmed that the full config export is restored to its original state on uninstallation, reinstallation, and a second uninstallation.

However, I'm now encountering the following behavior:

  1. Apply #159 to 8.3.x HEAD.
  2. Install Standard and enable field_layout.
  3. Visit /admin/structure/types/manage/article/display, expand "Layout settings", and select the "Two column" layout. Save.
  4. Move the comments into the right column. Save.
  5. Switch back to one column layout and save. The comment field moves back to the single column as expected.
  6. Navigate to /admin/structure/types/manage/article/display/teaser. Select "Two column" and Save.
  7. The form is not updated and the second column does not become available.
  8. Go back to /admin/structure/types/manage/article/display and try to enable the two-column layout again. The form is again not updated.
xjm’s picture

Status: Needs review » Needs work

Whoa. And now when I uninstalled field_layout, all the fields except the comment field disappeared from my teaser display. Something is very wrong.

xjm’s picture

I noticed #2843707: Default config for entity displays does not match what is saved in the process of trying to debug this. Not sure if it's related or just an incidental and harmless config mismatch.

xjm’s picture

Ah, I've figured out what is happening in #173. When I switch the teaser layout to the two-column one, the region is updated to left instead of content. But when I uninstall field_layout, it does not repair the teaser to switch it back to the content region. So the display edit form no longer has a definition for the layout provided, and it appears there are no fields to configure since they are assigned to a region that does not exist.

The fields are still actually displayed on the teaser because they are falling back to the default region, but it's impossible to configure them anymore on the display. That's probably commit-blocking since the module does not uninstall completely and leaves the display in a broken state for the site builder.

xjm’s picture

Also, if you reinstall field_layout after #173, the content actually disappears from the display when it is viewed as well, which would be a critical issue for sure. To get your fields back at that point, you have to switch it back to the layout that was being used before the previous uninstallation (which is no longer recorded anywhere).

I think I also figured out what was happening in the first half of #172. There is some AJAX behavior that happens when you change the layout setting, with a throbber. It does not appear to change the visible form. If you hit the "Save" button before whatever the throbber is doing is complete, your change to the layout is not saved. That seems like a major usability bug, not necessarily blocking for the prototype/initial commit, but maybe blocking for it to be stable at least.

xjm’s picture

When we fix #173, we should also test the fix when the layout being removed is provided by a different module than field_layout. The regions in each entity display depend on the layout that is used with those regions. Layouts might be added and removed from many different places, and we need to figure out what the expected dependency management is when those layouts go away.

xjm’s picture

xjm’s picture

I added #172 / second half of #176 to the followup roadmap.

xjm’s picture

Issue summary: View changes

Reviewing this against https://www.drupal.org/core/experimental#requirements.

No known security or data integrity issues.

and

No disruption or regression to stable functionality (including being able to remove the experimental module without lasting disruption).

Functional test coverage for the primary usecase, with no test failures in HEAD.

Test coverage is excellent.

Basic API documentation for the minimum requirements, an initial change record draft for the addition of the module, and a plan for completing more comprehensive documentation in the codebase and handbook.

Still needs the CR. The rest is covered on #2795833: [plan] Add layouts to entity displays (both form and view).

Ideas/prototypes for user interfaces that are planned and minimally validated.

The UI is being added to our existing minimally validated field UI. :P The product/usability team signed off on it being sufficient for this experimental module.

Wherever possible, extension names, namespaces, plugin IDs, etc. should be valid and match the intended final names, to avoid unnecessary upgrade path issues.

This is what @effulgentsia was getting at in #152. The module provides third-party settings that simply map to the layout ID and plugin. It also provides two default layouts, named beginning with "layout", which an @todo to move them. I'm a little unsure about the mismatch between the layout machine names and the module name -- will that have any impact on owner/provider/dependency calculations? But it sounds like they're not going to be there very long in either case.

I think the module name is fine.

Resolution of other critical issues that maintainers identify as hard blockers.

Covered already above, barring any additional ones in @effulgentsia's further review or the fix for #175.

A separate module roadmap ("plan" issue) documenting followup steps for outstanding work, including:
Followups to meet the core gates.
Followups to address concerns with the experimental UI and architecture, and iterate on them as needed.
Any other issues identified during planning and review.

I've added a couple items for usability and accessibility testing. For what it's worth, I find the collapsed layout fieldset at the bottom of the form difficult to discover, especially since it's buried after the WTFy "Custom display settings" fieldset on the default display. So I added that to the "Should have" followups as well.

A timeframe for completing the remaining work (typically two minor releases), a plan for addressing followups, and roughly which followups must be completed to keep the module in core. Followups should be grouped according to whether they must, should, or could be a part of the module's initial stable release.

#2795833: [plan] Add layouts to entity displays (both form and view) is in good shape. There are a few items in it that need to be sorted/clarified; see #2795833-25: [plan] Add layouts to entity displays (both form and view).


So, for release management signoff, the outstanding things I think we need to address are:

  1. #175
  2. My question about the layout machine name mismatch. (It might be fine.)
  3. A change record.
  4. Address #2795833-25: [plan] Add layouts to entity displays (both form and view).
  5. Anything that comes up in fixing #175 or in final framework manager review.

I added these to the summary as remaining tasks. Thanks!

tim.plunkett’s picture

#180.1

#173 uncovers two bugs!
Once field_layout is stable, it's my intention to merge it directly into EntityDisplayBase and Field UI, and uninstall wouldn't be possible. If you wanted to stop using a layout, you'd just switch back to the one-column.

So that's exactly what field_layout_uninstall() needs to do!

But that still doesn't fix the regions for each field. Which confused me, since I know we have code to do that, AND coverage for it.

The problem is that I put that code in the submitForm(), making it UI only.
Updating the kernel test first makes everything fail, and then moving the logic directly into the API fixes it.

#180.2 This is fine. It's convention only, no code cares about this. It's why each plugin has a "provider" tracked for it.

#180.3 @todo for tomorrow

#180.4 @todo for tomorrow

#180.5 TBD

tim.plunkett’s picture

+++ b/core/modules/field_layout/tests/src/FunctionalJavascript/FieldLayoutTest.php
@@ -258,7 +258,7 @@ protected function getRegionTitles() {
-    $region_element = $this->getSession()->getPage()->find('css', ".field-layout-region--$region_name");
+    $region_element = $this->getSession()->getPage()->find('css', ".layout-region--$region_name");

Bad interdiff on my part, this was in the last patch!

xjm’s picture

Issue summary: View changes

Coolness, glad those bugs were straightforward to fix.

Once field_layout is stable, it's my intention to merge it directly into EntityDisplayBase and Field UI, and uninstall wouldn't be possible. If you wanted to stop using a layout, you'd just switch back to the one-column.

I can see how that would make sense, but there would still potentially be the need for a smooth upgrade path/process for that transition. It's similar to the questions we had to address for the Layout plugin itself; in fact it's also related, since the dependency for entity displays was one of the compelling reasons for the layout plugin API to be in core/lib. For this module the question is much simpler smaller in scope: we already plan to move the layouts to the parent module, and almost everything else is this patch is internal API. We should add a point for moving the code to the end of the field layout roadmap. For now I just added a note to the summary here.

tim.plunkett’s picture

xjm’s picture

I confirmed that #175 is fixed.

All the release management things seem to be squared away, so this is xjm, signing off!

tim.plunkett’s picture

The last RTBC patch was in #127, here's an interdiff from that point.

Patch still applied the same, but rebased anyway just in case.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

The all new changes look good. Nothing related to functionality. Just some suggestion and a question. Setting it to RTBC beuase it is ready.

  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/field_layout']) . '</p>';
    

    Is there a follow-up docs issue to write https://www.drupal.org/documentation/modules/field_layout page?

  2. +++ b/core/modules/field_layout/src/FieldLayoutBuilder.php
    @@ -0,0 +1,137 @@
    +      // Add the regions to the $build in the correct order.
    ...
    +      $regions = array_fill_keys($layout_definition->getRegionNames(), $fill);
    

    These should be together.

  3. +++ b/core/modules/field_layout/src/Form/FieldLayoutEntityDisplayFormTrait.php
    @@ -0,0 +1,176 @@
    +      $subform_state = SubformState::createForSubform($form['field_layouts']['settings_wrapper']['layout_settings'], $form, $form_state);
    +      $layout_plugin->validateConfigurationForm($form['field_layouts']['settings_wrapper']['layout_settings'], $subform_state);
    ...
    +      $layout_plugin->submitConfigurationForm($form['field_layouts']['settings_wrapper']['layout_settings'], SubformState::createForSubform($form['field_layouts']['settings_wrapper']['layout_settings'], $form, $form_state));
    

    Let's keep the symmetry.

  4. +++ b/core/modules/field_layout/tests/modules/field_layout_test/src/Plugin/Layout/FieldLayoutTestPlugin.php
    @@ -0,0 +1,48 @@
    + * @todo.
    

    Stray @todo.

tim.plunkett’s picture

#188

1) This is explicitly mentioned as a "Must Have" in the parent issue (#2795833: [plan] Add layouts to entity displays (both form and view)) but is not part of this issue

2) Fixed

3) Fixed

4) Fixed

Two docs changes, one code cut/paste, so leaving RTBC.
Thanks for the review!

tim.plunkett’s picture

Had an interesting discussion with @effulgentsia today, specifically about ways the form portion of this could conflict with unknowns.

First, if you go through the Field UI and add a field called "Layout", the generated machine name will be field_layout.
FieldLayoutBuilder::buildForm() then overwrites the field completely.
The workaround is to use $build['_field_layout'] to store our additions to the form.

Second, any alterations to the form structure using #group will cause conflicts.
The easy example is the node form. \Drupal\node\NodeForm::form() uses #group to split the node form into two columns.
We agreed that if a field had an explicit #group set, Field Layout should leave it untouched.
Once Field Layout is stable, we can remove core's usage of #group for entity forms, and use layouts directly.
I've opened #2845425: Replace hook_form_node_form_alter() implementations with configured field layouts to deal with this long-term.

Test coverage is added for both changes, and leaving at RTBC since it was discussed and approved in real-time, and because @effulgentsia indicated he's not finished his review yet anyway.

tim.plunkett’s picture

@effulgentsia noticed that aggregator_entity_extra_field_info() defines extra fields that have the same name as non-configurable base fields as defined by Feed::baseFieldDefinitions() and Item::baseFieldDefinitions()
This is because those fields do not have formatters available, so the field definitions cannot be marked as configurable. But extra fields are always configurable.
Therefore our logic in FieldLayoutBuilder::getFields() needs to change. I did that, and added coverage for it.

@effulgentsia was also concerned about the direct array manipulation in FieldLayoutBuilder::buildView(), which isn't needed in ::buildForm() due to #groups.
I opened #2846393: Investigate alternative approaches to moving fields in FieldLayoutBuilder::buildView(), added it as a "Should Have" to the parent issue, and included an explicit @todo.

Also from our discussion I opened #2846389: Ajax progress indicator from layout settings is confusing, also adding it as a "Should Have" to the parent issue.

Finally, I made a dedicated issue for @swentel's point in #16 about the module weight: #2846395: Increase module weight of field_layout
That is a "Could Have" on the parent issue.

effulgentsia’s picture

#191 addresses all remaining framework manager concerns I could think of, so removing that tag.

@tim.plunkett is implementing a couple more minor things I pinged him about, so holding off on commit until then.

If anyone else has any remaining concerns about this patch, now is a good time to mention them. Otherwise, I expect to commit this within the next 24 hours.

tim.plunkett’s picture

There are 3 changes in this patch.

First, I reorganized the one test. Before it would set up a giant $expected array mapping to the entire display entity, for every step.
After time I found it hard to tell what was changing.
So now it directly manipulates the same array each time, with comments, so you can clearly see what is happening.
I split that out to its own interdiff.

Second, in #181 I halfway introduced a new logic branch but didn't finish it.
When switching layouts we have to remap the regions for each field. Before, it would blindly overwrite everything with the default region of the new layout.
Now if the old layout and new layout share a region name, the field stays put. This finishes that logic, and adds coverage for it.

Thirdly, FieldLayoutEntityDisplayTrait::preSave() was written to mimic ConfigEntityBase::preSave().
It ensures that the configuration was set on the plugin, and the plugin was consulted for updated values (allowing for things like default configuration).
This code would go away in #2844302: Move Field Layout data model and API directly into \Drupal\Core\Entity\EntityDisplayBase, since it's only needed here because these are third-party settings.
Similarly, we have a ::calculateDependencies().
We don't need a __sleep() as we don't store the instantiated plugin as a property.
Finally there is ConfigEntityBase::set(), which we don't have a version of.
This adds code to setLayoutId() to approximate that, and adds test coverage.

The second and third change are in the main interdiff.

Since these were discussed with @effulgentsia, once again leaving RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 193: 2796173-field_layout-193.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Random failure in outside_in, retested and all is well.
CR was written already

effulgentsia’s picture

Ticking credit boxes for everyone! Thank you all for your insights that helped this issue.

  • effulgentsia committed e14a020 on 8.3.x
    Issue #2796173 by tim.plunkett, samuel.mortenson, xjm, jibran,...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.3.x and published the CR.

effulgentsia’s picture

Issue tags: +8.3.0 release notes
jibran’s picture

Yay! thanks @effulgentsia.

Gábor Hojtsy’s picture

xjm’s picture

First, if you go through the Field UI and add a field called "Layout", the generated machine name will be field_layout.

Good catch!

Thanks @Gábor Hojtsy for updating the experimental module list. Looks good to me.

  • effulgentsia committed e14a020 on 8.4.x
    Issue #2796173 by tim.plunkett, samuel.mortenson, xjm, jibran,...

  • effulgentsia committed e14a020 on 8.4.x
    Issue #2796173 by tim.plunkett, samuel.mortenson, xjm, jibran,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.