Problem/Motivation

While reviewing #3032287: Add the content moderation state widget to the layout builder entity form., @amateescu pointed out that the custom hook being implemented, hook_layout_builder_overrides_entity_form_display_alter has some overlap with with the existing hook hook_entity_form_display_alter.

Using the existing hook means:

  • Less APIs, less duplication of concerns.
  • Implementors would have to check they are altering the layout builder override display.

Firing and implementing the current hook means:

  • An explicit API for dealing with LB override displays, no conditions on displays or context.

Proposed resolution

Right now, for the LB code flow hook_entity_form_display_alter is not fired because EntityFormDisplay::collectRenderDisplay is never called, however efforts could be made to either:

  • Fire the hook again with the same params.
  • Add a $default_fallback = TRUE parameter to EntityFormDisplay::collectRenderDisplay() that allows it to to prevent loading the default display.

Remaining tasks

None.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

Release notes snippet

In 8.70-alpha1 and alpha2 only, the Layout Builder module provided hook_layout_builder_overrides_entity_form_display_alter() to modules extending the Layout Builder user interface. was has been removed in in favor of the generic hook_entity_form_display_alter() hook, so any modules depending on this hook during the alpha phase should be updated to use the new hook. Most sites and modules are not affected by this change.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

I think we could remove the hook before 8.7 beta if we came to a resolution on this issue.

Personally my take is that these things serve sufficiently unique circumstances (ie, the LB overrides display has a unique lifecycle compared to a standard display) that having two hooks makes sense. IMO it's a more discoverable and explicit way to deal with this component of LB.

I do however understand the duplication argument and don't feel very strongly about the previous point, so would be fine with making this adjustment if either of the proposed resolutions were feasible, low-risk changes (in terms of stability of existing features etc) and accepted by a committer in the alpha lifecycle.

Sam152 credited amateescu.

Sam152’s picture

Assigned: Unassigned » tim.plunkett

Crediting @amateescu for raising these points and shaping the discussion.

I think @tim.plunkett is also best placed to weigh in here. I wouldn't usually assign an issue, but there is some time sensitivity with this one.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Sam152’s picture

Status: Active » Needs review
FileSize
6.41 KB

Here is a first pass at an implementation of this.

The changes in here to ::collectRenderDisplay would also be useful for #2849279: Moderation control form should validate an entity before saving it, which follows a similar pattern to constructing the entity form.

amateescu’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -65,6 +65,9 @@ class EntityFormDisplay extends EntityDisplayBase implements EntityFormDisplayIn
    +   *   (Optional) If the render display should be instantiated without
    

    Optional -> optional.

  2. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -65,6 +65,9 @@ class EntityFormDisplay extends EntityDisplayBase implements EntityFormDisplayIn
    +   *   attempting to load any display from configuration.
    

    Defaults to FALSE. ;)

  3. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -72,28 +75,30 @@ class EntityFormDisplay extends EntityDisplayBase implements EntityFormDisplayIn
    +    if (!$transient) {
    

    How about returning early instead of nesting everything inside this check?

  4. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
    @@ -76,24 +76,16 @@ public function getBaseFormId() {
    +    $form_display = EntityFormDisplay::collectRenderDisplay($this->entity, $this->getOperation());
    

    Shouldn't we call it with $transient = TRUE? :)

Sam152’s picture

Whoops, forgot to actually use the new flag. Adding that in and an assert which would fail if the flag wasn't present.

Sam152’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
@@ -106,7 +106,7 @@ public static function collectRenderDisplay(FieldableEntityInterface $entity, $f
-        'mode' => $form_mode,
+        'mode' => $transient ? EntityDisplayBase::CUSTOM_MODE : $form_mode,

Whoops, cross posted with #7.

Maybe we should also introduce EntityDisplayBase::TRANSIENT_MODE or something?

Sam152’s picture

1. Done.
2. Done.
3. I couldn't come up with a better control flow, because we still want the latter part of the function to run, so the hook is called. Open to input here.
4. Addressed in #8, but I'm unsure it's the right solution.

amateescu’s picture

+++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
@@ -76,24 +76,15 @@ public function getBaseFormId() {
+    $this->setOperation('layout_builder');

This shouldn't be needed because the form operation is already defined as layout_builder in the entity definition.

Maybe we should also introduce EntityDisplayBase::TRANSIENT_MODE or something?

I don't think we need that IMO, CUSTOM_MODE describes it well and Views uses it for the same use case :)

The last submitted patch, 6: 3038442-6.patch, failed testing. View results

The last submitted patch, 8: 3038442-7.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 10: 3038442-10.patch, failed testing. View results

Sam152’s picture

Status: Needs work » Needs review
FileSize
3.71 KB
9.67 KB

I think this approach makes more sense. Also fixing the failing test.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
@@ -65,6 +65,9 @@ class EntityFormDisplay extends EntityDisplayBase implements EntityFormDisplayIn
+   * @param bool $transient
+   *   (optional) If the render display should be instantiated without
+   *   attempting to load any display from configuration. Defaults to FALSE.

@@ -103,6 +108,7 @@ public static function collectRenderDisplay(FieldableEntityInterface $entity, $f
+        'skipDisplayInitialization' => $transient,

The naming of transient here is coupling WHAT the code does with WHY someone would want to do it.

Also, this variable does multiple things:

  1. Prevents a saved display from being loaded
  2. Prevents the default/fallback from being used
  3. Expands the logic in EDB::init() from only working for custom modes to custom modes and those that set skipDisplayInitialization to TRUE

The changes within CM and LB look good, so I'm happy with the approach. Just concerned about communicating what this means, and subtly changing how important/necessary CUSTOM_MODE is

Sam152’s picture

Assigned: tim.plunkett » Unassigned

Yeah, the alternative would be to simply use _custom as the mode, but in doing so it's not obvious to identify the display actually belongs to the layout builder overrides form.

This was the only other thing I came up with to ensure the defaults aren't initialised, however it's problematic in the sense that the mode in the fired hook goes out of sync with the mode the display is instantiated, which is a lot weirder imo.

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
@@ -106,7 +106,7 @@ public static function collectRenderDisplay(FieldableEntityInterface $entity, $f
       $display = $storage->create([
         'targetEntityType' => $entity_type,
         'bundle' => $bundle,
-        'mode' => $form_mode,
+        'mode' => $transient ? EntityDisplayBase::CUSTOM_MODE : $form_mode,
         'status' => TRUE,
       ]);

Not sure if this could be addressed with more documentation or simply another approach.

Thanks for taking a look @tim.plunkett. Unassigning since I was the one who assigned this in #4. Feel free to assign it back if you're working on it.

tim.plunkett’s picture

I think it mostly needs naming/docs improvements, not necessarily a new implementation.

amateescu’s picture

Status: Needs review » Needs work

I think this is NW now based on Tim's feedback..

Sam152’s picture

Okay, I think the naming the only outstanding thing then. We could rename the variable to $transient_and_uninitialized or $empty_transient.

Alternatively we could add two flags instead of one:

public static function collectRenderDisplay(FieldableEntityInterface $entity, $form_mode, $initialized = TRUE, $transient = FALSE)

However that's kind of strange since the initialized flag would only actually apply if a new runtime object was being created, which would be quite a rare case.

amateescu’s picture

Status: Needs work » Needs review
FileSize
7.7 KB
5.37 KB

How about changing the variable name to $default_fallback (or $load_default?) and make it only about whether we should attempt to load the default display from configuration. This would allow people to save a layout_builder form display in config, and I think that's a feature that we shouldn't try to prevent..

Also, I don't think that using EntityDisplayBase::CUSTOM_MODE as the 'final' mode for the form display object is that problematic, developers have two ways of getting the 'initial' form mode that was requested: EntityDisplayInterface::getOriginalMode() and the $context['form_mode'] argument of hook_entity_form_display_alter().

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Blocks-Layouts, +beta target
+++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
@@ -82,7 +86,9 @@ public static function collectRenderDisplay(FieldableEntityInterface $entity, $f
-    $candidate_ids[] = $entity_type . '.' . $bundle . '.default';
+    if ($default_fallback) {
+      $candidate_ids[] = $entity_type . '.' . $bundle . '.default';
+    }

@@ -101,7 +107,7 @@ public static function collectRenderDisplay(FieldableEntityInterface $entity, $f
-        'mode' => $form_mode,
+        'mode' => $default_fallback ? $form_mode : static::CUSTOM_MODE,

Oh this is so much clearer!

Thanks, this would be great to land in 8.7 before that Layout Builder hook is in the wild.

tim.plunkett’s picture

Title: Investigate using hook_entity_form_display_alter instead of layout builders custom overrides display altering hook » Use hook_entity_form_display_alter instead of Layout Builder's custom overrides display altering hook
Sam152’s picture

developers have two ways of getting the 'initial' form mode that was requested

Ah, custom the '_custom' mode seems okay in this case. It does however still suffer from the feedback given in #16 imo, but if @tim.plunkett is fine with this, all good from me.

Edit: reading the feedback again, that isn't actually true! +1 RTBC.

tim.plunkett’s picture

Since this hook was in 8.7.0-alpha1 we need to write a CR and release note for the removal

amateescu’s picture

Issue summary: View changes
Issue tags: -Needs change record, -Needs release note

Updates the IS with a release note and wrote a CR: https://www.drupal.org/node/3042512

xjm’s picture

Issue summary: View changes

Making the release note a bit clearer about the limited scope of the BC break.

xjm’s picture

Title: Use hook_entity_form_display_alter instead of Layout Builder's custom overrides display altering hook » Use hook_entity_form_display_alter() instead of Layout Builder's custom overrides display altering hook
Issue tags: -beta target

Not a beta target, though. This needs to be committed before the beta phase begins. If it had not landed by today it would simply have needed to follow the deprecation process instead

  • xjm committed 8d1a7d7 on 8.8.x
    Issue #3038442 by Sam152, amateescu, tim.plunkett: Use...

  • xjm committed 21e5681 on 8.7.x
    Issue #3038442 by Sam152, amateescu, tim.plunkett: Use...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.8.x and cherry-picked to 8.7.x. Thanks!

xjm’s picture

Issue tags: +8.7.0 release notes

Tagging for the release notes. (We'll include this in the beta release notes, but probably will remove it for the 8.7.0 minor notes since the BC break does not affect updates from 8.6.)

xjm’s picture

Oops, also belatedly published the CR.

Status: Fixed » Closed (fixed)

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