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
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 |
Comment | File | Size | Author |
---|---|---|---|
#43 | 2094499-42.patch | 59.81 KB | amateescu |
#40 | interdiff.txt | 1.94 KB | amateescu |
#40 | 2094499-40.patch | 59.82 KB | amateescu |
Comments
Comment #1
swentel CreditAttribution: swentel commentedAssigning to me
Comment #2
andypostAwesome idea! +1
Comment #3
andypostSuppose better to make field_ui module to inject form controller into
EntityDisplay
andEntityFormDisplay
likemenu_entity_info()
does for menu config entityThis should allow to use
$this->entity
and native entity form, just pass additional arguments via routesComment #4
yched CreditAttribution: yched commentedMoving forward on this might make #1875974: Abstract 'component type' specific code out of EntityDisplay easier.
@swentel, do you still intend to work on this ?
Comment #5
swentel CreditAttribution: swentel commentedOoh crap, completely forgot about this one. I can't work on this before next monday or so.
Comment #6
yched CreditAttribution: yched commentedOK - unassigning for now then ?
Comment #7
andypostI think better to postpone this one on #1963340: Change field UI so that adding a field is a separate task
Comment #8
andypostSuppose 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
fromDisplayOverviewBase
Comment #9
yched CreditAttribution: yched commentedFrom the OP:
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.
Comment #10
amateescu CreditAttribution: amateescu commented#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.
Comment #11
amateescu CreditAttribution: amateescu commentedAfter #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.
Comment #12
amateescu CreditAttribution: amateescu commentedSmall self-review :)
Comment #15
amateescu CreditAttribution: amateescu commentedIt seems we cannot retire the
getEntityDisplay()
helper because of the "a display is never guaranteed to exist" architecture.Comment #16
yched CreditAttribution: yched commentedAwesome ! @amateescu++ !
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)
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
The descriptions of $entityManager and $configFactory are no longer needed here, as they are defined by the parent.
- 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 ?
See remark above
- 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)
Looks like it's only needed in EntityDisplayFormBase, not in the child classes
YAY :-)
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 ?
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 ?
Comment #17
amateescu CreditAttribution: amateescu commentedThanks for the review!
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.
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...
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
orentity.node_type.delete_form
from node.routing.yml as examples.Comment #18
andypostAbout #18.9 - entity routes should start from
entity.
see referenced issues and #2285413: [Meta] Standardize entity route namesSuppose t('- Hidden -') otherwise no way to translate
Comment #19
yched CreditAttribution: yched commentedGreat cleanup, thanks @amateescu.
So, the last issue is EntityDisplayFormBase::save() and the weird need for enforceIsNew() ?
Comment #20
amateescu CreditAttribution: amateescu commentedYes, and I think I just found the culprit.
ConfigEntityBase::isNew()
looks like this: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?
Comment #21
tstoecklerSeems we could add
enforceIsNew
to the list of keys inEntityFormDisplay::__sleep()
, no?Comment #22
yched CreditAttribution: yched commentedHm - 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 ?
Comment #23
amateescu CreditAttribution: amateescu commented@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 :(
Comment #24
yched CreditAttribution: yched commentedAwww - 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 !
Comment #25
amateescu CreditAttribution: amateescu commentedRerolled for #2405127: Move EntityFormDisplay::_sleep() up to EntityDisplayBase.
Comment #26
yched CreditAttribution: yched commentedAlmost crossposted with the same reroll :-)
I was wondering this though :
Shouldn't this rather be $this->enforceIsNew($values['enforceIsNew']) ?
Comment #27
yched CreditAttribution: yched commented(also, didn't intend to "remove" the files from #25, I though I was removing mines, sorry :-/)
Comment #28
amateescu CreditAttribution: amateescu commentedIt can't use $values because of the line just above:
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 :(
Comment #30
yched CreditAttribution: yched commentedHm.
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...
Comment #31
yched CreditAttribution: yched commentedOpened #2406081: Entries present at the top-level of the config schema but absent from runtime ConfigEntities appear as 'null' in the YAML
Comment #32
amateescu CreditAttribution: amateescu commentedYup, that works. Thanks!
Comment #34
yched CreditAttribution: yched commentedLooks like it also needs to be removed from ymls shipped in default config
Comment #35
amateescu CreditAttribution: amateescu commentedThat's right.
Comment #36
amateescu CreditAttribution: amateescu commentedStarted rerolling this for #2281645: Make entity annotations use link templates instead of route names but I have to go for a few hours.
Comment #37
amateescu CreditAttribution: amateescu commentedFinished the reroll sooner :)
Comment #38
amateescu CreditAttribution: amateescu commentedAdded a beta evaluation to the IS.
Comment #40
amateescu CreditAttribution: amateescu commented#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.
Comment #41
yched CreditAttribution: yched commentedThanks @amateescu !
Comment #42
andypost+1 RTBC!!!
@amateescu
It is #2346883: Standardize field_ui entity route names to fix naming now
Comment #43
amateescu CreditAttribution: amateescu commentedRerolled 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 :)
Comment #44
alexpottThese 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.
Comment #46
yched CreditAttribution: yched commentedYay, @amateescu++ !
Ooooh yeah. Field UI routing currently confuses the hell out of me :-/
Comment #47
yched CreditAttribution: yched commentedFYI : #2413841: EntityDisplayBase::__wakeup() should avoid calling toArray()
Was not introduced by this patch here, but pinging the right persons :-)
Comment #49
amateescu CreditAttribution: amateescu commentedThis is the issue that tries to fix it: #2428035: Bring some sanity into Field UI routes and forms
Comment #50
amateescu CreditAttribution: amateescu commentedFor anyone interested, I just posted a sister issue: #2446869: Convert the "Field storage edit" form to an actual entity form
Comment #51
jibranThere 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.