What the Field UI does currently is still modeled after the D7 architecture:
- define routes for "Manage display [view_mode]" pages,
- use access callbacks to only show those where the view modes uses "custom settings"
- in the page callbacks, show a form collecting various stuff and save them where appropriate on submit.

The D8 situation is that those "Manage display" pages really are "where you edit an EntityDisplay entity".
So the EntityDisplay object should be loaded by the route as part of parameter upcasting (in a way that the upcast would fail if status = FALSE --> 404), and received as an argument in the form builder (instead of currently: the form builder receives the raw URL parts, and at some point goes "I'll load an EntityDisplay object because I'll do stuff with it").
(and same for form displays of course)

An additional step might be to make those forms be the actual, official "Entity forms" for the EntityDisplay entity types (in the Entity API sense), but that might be a bit harder since the entity types are provided by entity.module while the forms are provided by field_ui.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because the current forms are not broken.
Prioritized changes The main goal of this issue is converting two Field UI forms to the new API available in Drupal 8 (EntityForm). It also removes a lot of unneeded code in the process:
27 files changed, 180 insertions(+), 355 deletions(-)
Disruption Minor disruption for contrib modules which alter the "Manage form display" and "Manage display" Field UI forms
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Assigned: Unassigned » swentel

Assigning to me

andypost’s picture

Awesome idea! +1

andypost’s picture

Suppose better to make field_ui module to inject form controller into EntityDisplay and EntityFormDisplay like menu_entity_info() does for menu config entity

This should allow to use $this->entity and native entity form, just pass additional arguments via routes

yched’s picture

Moving forward on this might make #1875974: Abstract 'component type' specific code out of EntityDisplay easier.
@swentel, do you still intend to work on this ?

swentel’s picture

Ooh crap, completely forgot about this one. I can't work on this before next monday or so.

yched’s picture

Assigned: swentel » Unassigned
Issue summary: View changes

OK - unassigning for now then ?

andypost’s picture

andypost’s picture

Status: Postponed » Active

Suppose better to open the issue again, this one just need bits of #1963340: Change field UI so that adding a field is a separate task to decouple OverviewBase from DisplayOverviewBase

yched’s picture

From the OP:

An additional step might be to make those forms be the actual, official "Entity forms" for the EntityDisplay entity types (in the Entity API sense), but that might be a bit harder since the entity types are provided by entity.module while the forms are provided by field_ui

It would actually not be really new : Field UI already provides the "entity delete form" for FieldInstance config entities & the "list controller" for Field config entities.

IMO that's what we should shoot for here - make the "Manage Display" & "Manage Form" pages the actual entity forms for EntityViewDisplay & EntityFormDisplay config entties.

amateescu’s picture

Status: Active » Postponed

#1963340: Change field UI so that adding a field is a separate task is getting pretty close now, I'd like to keep this one postponed for a bit longer and I'll tackle it after.

amateescu’s picture

Title: "Manage (form) display" forms should directly receive an EntityDisplay object » Convert "Manage (form) display" forms to be the official entity edit forms for EntityView[Form]Display objects
Status: Postponed » Needs review
FileSize
46.87 KB

After #1963340: Change field UI so that adding a field is a separate task and #2399219: Allow entity form handlers to determine the entity object they need to work with, this was now pretty straightforward to implement.

I haven't done anything related to access checks because I'm not changing the URLs to include a {entity_view(form)_display} parameter. I tried it at some point but the local tasks couldn't work anymore, that's why I had to wait for the second issue mentioned above to be committed before finishing this one.

This patch will need a reroll if #2030607: Expand EntityDisplayBase with methods gets in first.

amateescu’s picture

FileSize
46.87 KB
1.42 KB

Small self-review :)

The last submitted patch, 11: 2094499.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: 2094499-12.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
47.25 KB
5.42 KB

It seems we cannot retire the getEntityDisplay() helper because of the "a display is never guaranteed to exist" architecture.

yched’s picture

Awesome ! @amateescu++ !

  1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -20,6 +20,11 @@
    + *   handlers = {
    + *     "form" = {
    + *       "edit" = "Drupal\field_ui\Form\EntityFormDisplayEditForm"
    + *     }
    + *   },
    
    +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
    @@ -21,6 +21,11 @@
    + *   handlers = {
    + *     "form" = {
    + *       "edit" = "Drupal\field_ui\Form\EntityViewDisplayEditForm"
    + *     }
    + *   },
    

    Since the form is provided by field_ui, shouldn't this be added in field_ui_entity_type_build() ?
    (which is also where the link templates for the various Field UI pages are added on the fieldable entity types)

  2. +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
    @@ -2,44 +2,46 @@
     /**
      * Field UI display overview base class.
      */
    -abstract class DisplayOverviewBase extends FormBase {
    +abstract class EntityDisplayFormBase extends EntityForm {
    

    The phpdocs for the EntityDisplayFormBase, EntityViewDisplayEditForm, EntityFormDisplayEditForm classes should be updated for their new identity : those are the edit forms for the EntityDisplay entities, the notion of "overview" is stale now

  3. +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
    @@ -92,7 +87,14 @@
       protected $configFactory;
    

    The descriptions of $entityManager and $configFactory are no longer needed here, as they are defined by the parent.

  4. +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
    @@ -113,13 +115,20 @@ public function __construct(EntityManagerInterface $entity_manager, FieldTypePlu
    + public function getEntityFromRouteMatch(RouteMatchInterface $route_match, $entity_type_id) {
    +    $route_paramateres = $route_match->getParameters()->all();
    +    $this->targetEntityTypeId = $route_paramateres['entity_type_id'];
    +
    +    if (isset($route_paramateres['bundle'])) {
    +      $this->targetBundle = $route_paramateres['bundle'];
    +    }
    +    else {
    +      $target_entity_type = $this->entityManager->getDefinition($route_paramateres['entity_type_id']);
    +      $this->bundleEntityTypeId = $target_entity_type->getBundleEntityType();
    +      $this->targetBundle = $route_paramateres[$this->bundleEntityTypeId]->id();
    +    }
    +
    +    return $this->getEntityDisplay($route_match->getParameter($this->displayContext . '_mode_name'));
    

    - Typo : paramateres :-)

    - So $this->targetEntityTypeId and $this->targetBundle are set both in getEntityFromRouteMatch() here and in form() below, that's a bit confusing (that's either redundant or conflicting ?)

    Is there a way we could avoid having those as "internal state of the object that only exist after some method has been called" ? Those forms work on a Display entity, so the moment we have been able to nail down a Display for $this->entity, any code that needs to know the target entity type & bundle can fetch them from the entity ?

  5. +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
    @@ -185,32 +194,23 @@ protected function getFieldDefinitions() {
      public function form(array $form, FormStateInterface $form_state) {
    +    $this->targetEntityTypeId = $this->entity->targetEntityType;
    +    $this->targetBundle = $this->entity->bundle;
    

    See remark above

  6. +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
    @@ -578,8 +574,61 @@ protected function buildExtraFieldRow($field_id, $extra_field, EntityDisplayInte
    +  public function save(array $form, FormStateInterface $form_state) {
    +    // Work around the fact that entity display are never guaranteed to be
    +    // present, not even the 'default' one.
    +    if (!$this->entityManager->getStorage($this->entity->getEntityTypeId())->load($this->entity->id())) {
    +      $this->entity->enforceIsNew();
    +    }
    +
    +    parent::save($form, $form_state);
    +  }
    

    - Minor : for logic, can we move that below copyFormValuesToEntity() ?

    - Also, not sure why this is needed to begin with - if entity_get_display() had to create the Display, then it should be "isNew" already ?

    - If that *is* really needed, would it make sense to do that in getEntityFromRouteMatch() instead ? (not sure, just thinking out loud)

  7. +++ b/core/modules/field_ui/src/Form/EntityFormDisplayEditForm.php
    @@ -2,27 +2,22 @@
    +use Drupal\Core\Routing\RouteMatchInterface;
    
    +++ b/core/modules/field_ui/src/Form/EntityViewDisplayEditForm.php
    @@ -2,28 +2,22 @@
    +use Drupal\Core\Routing\RouteMatchInterface;
    

    Looks like it's only needed in EntityDisplayFormBase, not in the child classes

  8. +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
    @@ -111,46 +111,48 @@ protected function alterRoutes(RouteCollection $collection) {
    -            '_form' => '\Drupal\field_ui\FormDisplayOverview',
    +            '_entity_form' => 'entity_form_display.edit',
    

    YAY :-)

  9. +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
    @@ -111,46 +111,48 @@ protected function alterRoutes(RouteCollection $collection) {
    +        $collection->add("entity.entity_form_display.$entity_type_id", $route);
    

    Not sure whether the new route names should be prefixed by 'entity.' - it's still field_ui that defines the routes, the form classes, and adds the "link templates" in the various entity type definitions (and the names of those templates are still 'field_ui-*')

    So shouldn't they stay in the field_ui.* space ?

  10. +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
    @@ -111,46 +111,48 @@ protected function alterRoutes(RouteCollection $collection) {
    +        $collection->add("entity.entity_form_display.form_mode_$entity_type_id", $route);
    

    Nitpick - so (regardless of the 'entity.' / 'field_ui.' prefix) we now have :
    xx.entity_view_display.[entity_type] : 'default' view mode
    xx.entity_view_display.view_mode_[entity_type] : other view modes
    xx.entity_form_display.[entity_type] : 'default' form mode
    xx.entity_form_display.form_mode_[entity_type] : other form modes

    Wondering whether :
    xx.entity_view_display.[entity_type].default
    xx.entity_view_display.[entity_type].view_mode
    xx.entity_form_display.[entity_type].default
    xx.entity_form_display.[entity_type].form_mode
    would be clearer ?

amateescu’s picture

FileSize
47.89 KB
23.85 KB

Thanks for the review!

  1. Good catch, fixed.
  2. Fixed.
  3. Fixed.
  4. Is there a way we could avoid having those as "internal state of the object that only exist after some method has been called" ?

    Yes, I had this in mind when I wrote the patch, but it only became possible after bringing back getEntityDisplay() in #15 and I guess I was too lazy to remember I could also do that now. Fixed.

  5. See answer above :P
  6. - Minor : for logic, can we move that below copyFormValuesToEntity() ?

    - Also, not sure why this is needed to begin with - if entity_get_display() had to create the Display, then it should be "isNew" already ?

    - If that *is* really needed, would it make sense to do that in getEntityFromRouteMatch() instead ? (not sure, just thinking out loud)

    I lost quite some time trying to figure out what's going on there and I couldn't. The problem was that the display was "not new" only in the form's save() method, it was new everywhere else. It could be just some static cache in the storage handler (or FieldUIRouteTest) but I never debugged simpletest with Xdebug so I couldn't think of anything else...

  7. Fixed.
  8. Double yay :)
  9. So shouldn't they stay in the field_ui.* space ?

    The current convention is to prefix entity forms with "entity.", not with the module that provides the entity type or the form. See entity.node_type.edit_form or entity.node_type.delete_form from node.routing.yml as examples.

  10. No preference whatsoever, changed to the proposed names.
andypost’s picture

About #18.9 - entity routes should start from entity. see referenced issues and #2285413: [Meta] Standardize entity route names

+++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
@@ -935,7 +917,7 @@ protected function getPluginOptions(FieldDefinitionInterface $field_definition)
+    return $applicable_options + array('hidden' => '- ' . $this->t('Hidden') . ' -');

Suppose t('- Hidden -') otherwise no way to translate

yched’s picture

Great cleanup, thanks @amateescu.

So, the last issue is EntityDisplayFormBase::save() and the weird need for enforceIsNew() ?

amateescu’s picture

So, the last issue is EntityDisplayFormBase::save() and the weird need for enforceIsNew() ?

Yes, and I think I just found the culprit. ConfigEntityBase::isNew() looks like this:

  public function isNew() {
    return !empty($this->enforceIsNew);
  }

Which means that as soon as soon as a configuration entity is serialized as part of the form state (via ConfigEntityBase::toArray()), it loses the value of the $enforceIsNew protected member, which in turn means that the next time its form is displayed, the entity will not be "new" anymore.

Not sure yet how we can fix this in a generic way.. any ideas?

tstoeckler’s picture

Seems we could add enforceIsNew to the list of keys in EntityFormDisplay::__sleep(), no?

yched’s picture

Hm - I'm confused, I was pretty sure that EntityDisplays had custom __sleep() code that, for example, avoided serializing the instantiated widget / formatter plugins contained in $this->plugins. But I can't seem to find it :-)

But then, if we don't have it and only use DependencySerializationTrait::__sleep() like any Entity, then I don't see why $enforceIsNew would be lost on serialize ?

amateescu’s picture

FileSize
48.98 KB
2.48 KB

Seems we could add enforceIsNew to the list of keys in EntityFormDisplay::__sleep(), no?

@tstoeckler, yes, that and some code to properly restore the value in EntityFormDisplay::__wakeup() did it!

@yched, only EntityFormDisplay has the custom _sleep() and __wakeup() code. Quite a bit of hair pulling until I figured it out, because I didn't read #21 correctly at first :(

yched’s picture

Status: Needs review » Reviewed & tested by the community

only EntityFormDisplay has the custom _sleep() and __wakeup() code

Awww - that's wicked :-)
Opened #2405127: Move EntityFormDisplay::_sleep() up to EntityDisplayBase

Meanwhile, yay for the fix and simplification ! This looks ready to me.
Thanks a ton, @amateescu !

amateescu’s picture

yched’s picture

Almost crossposted with the same reroll :-)

I was wondering this though :

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
@@ -408,7 +409,9 @@ public function __sleep() {
+    $is_new = $this->isNew();
     $this->__construct($values, $this->entityTypeId);
+    $this->enforceIsNew($is_new);

Shouldn't this rather be $this->enforceIsNew($values['enforceIsNew']) ?

yched’s picture

(also, didn't intend to "remove" the files from #25, I though I was removing mines, sorry :-/)

amateescu’s picture

Status: Reviewed & tested by the community » Needs review

Shouldn't this rather be $this->enforceIsNew($values['enforceIsNew']) ?

It can't use $values because of the line just above:

$values = array_intersect_key($this->toArray(), get_object_vars($this));

which removes 'enforceIsNew' from the array.

And since #2405127: Move EntityFormDisplay::_sleep() up to EntityDisplayBase, I'm now getting some exceptions locally when running FieldUIRouteTest :(

serialize(): "label" returned as member variable from __sleep() but does not exist

The last submitted patch, 25: 2094499-25.patch, failed testing.

yched’s picture

Hm.
The core.entity_view_display.*.yml files on my setup do have a 'label' entry, equal to null,
and core.entity.schema.yml / core.entity_view_display.*.*.* schema does have a 'label' entry

This 'label' is absent from core.entity_form_display.*.yml / core.entity_form_display.*.*.* schema

So that would explain it - in current HEAD, EVDs do not get serialized in the test suite, and they apparently can with the patch.
The fix would be to remove that incorrect entry from the schema.

What I find really surprising, though, is that ConfigEntity::toArray() outputs entries with NULL values for properties present in the schema but absent from the runtime object ? That sounds wrong...

yched’s picture

What I find really surprising, though, is that ConfigEntity::toArray() outputs entries with NULL values for properties present in the schema but absent from the runtime object ? That sounds wrong...

Opened #2406081: Entries present at the top-level of the config schema but absent from runtime ConfigEntities appear as 'null' in the YAML

amateescu’s picture

FileSize
49.39 KB
454 bytes

The fix would be to remove that incorrect entry from the schema.

Yup, that works. Thanks!

Status: Needs review » Needs work

The last submitted patch, 32: 2094499-32.patch, failed testing.

yched’s picture

Looks like it also needs to be removed from ymls shipped in default config

amateescu’s picture

Status: Needs work » Needs review
FileSize
57.88 KB
8.49 KB

That's right.

amateescu’s picture

Status: Needs review » Needs work

Started rerolling this for #2281645: Make entity annotations use link templates instead of route names but I have to go for a few hours.

amateescu’s picture

Status: Needs work » Needs review
FileSize
58.08 KB

Finished the reroll sooner :)

amateescu’s picture

Issue summary: View changes

Added a beta evaluation to the IS.

Status: Needs review » Needs work

The last submitted patch, 37: 2094499-37.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
59.82 KB
1.94 KB

#2281645: Make entity annotations use link templates instead of route names made such a mess for Field Ui that it took me a full day to figure out a way for this patch to work again :( We'll need a dedicated issue to properly clean up everything related to Field UI routes / local tasks / local actions.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @amateescu !

andypost’s picture

+1 RTBC!!!

@amateescu

We'll need a dedicated issue to properly clean up everything related to Field UI routes / local tasks / local actions.

It is #2346883: Standardize field_ui entity route names to fix naming now

amateescu’s picture

FileSize
59.81 KB

Rerolled for a small context change after #2401505: Add an entity collection template for lists .

@andypost, what I have in mind is bigger than just naming :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

These are entities so we should be using EntityForm - nice tidy up. Also less weirdness in the field_ui link templates is appreciated. Committing under the reduce fragility maintainer discretion. Committed b8378ac and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation.

  • alexpott committed b8378ac on 8.0.x
    Issue #2094499 by amateescu: Convert "Manage (form) display" forms to be...
yched’s picture

Yay, @amateescu++ !

[amateescu] We'll need a dedicated issue to properly clean up everything related to Field UI routes / local tasks / local actions.
[andypost] It is #2346883: Standardize field_ui entity route names
[amateescu] What I have in mind is bigger than just naming :)

Ooooh yeah. Field UI routing currently confuses the hell out of me :-/

yched’s picture

FYI : #2413841: EntityDisplayBase::__wakeup() should avoid calling toArray()
Was not introduced by this patch here, but pinging the right persons :-)

Status: Fixed » Closed (fixed)

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

amateescu’s picture

Ooooh yeah. Field UI routing currently confuses the hell out of me :-/

This is the issue that tries to fix it: #2428035: Bring some sanity into Field UI routes and forms

amateescu’s picture

For anyone interested, I just posted a sister issue: #2446869: Convert the "Field storage edit" form to an actual entity form

jibran’s picture

There is a change notice in draft queue https://www.drupal.org/node/2404475 for #1963340: Change field UI so that adding a field is a separate task which is not relevant anymore because of the changes here so I suggest we update the 2404475 for this issue. Or we can just delete it.