Updated: Comment #0
Problem/Motivation
#2031725: Move all entity display interfaces to the core component renamed the interfaces for entity view and form modes. To limit the scope of the patch and the API change the classes themselves were not renamed in that issue. The current situation is the following:
EntityDisplay implements EntityViewDisplayInterface
EntityFormDisplay implements EntityFormDisplayInterface
Proposed resolution
- Rename
EntityDisplay
intoEntityViewDisplay
- Change the entity type id of entity view displays from
entity_display
intoentity_view_display
- Change the entity type label of entity view displays from Entity display into Entity view display
- Change the config prefix of entity view displays from
entity.display
intoentity.view_display
. - Rename
hook_entity_display_alter()
tohook_entity_view_display_alter()
Remaining tasks
Write patch.
User interface changes
For modules that expose the labels of entity types 4. above is a minor API change. In core this affects the entity type select list on the field settings form of an entity reference field, for example.
API changes
See 1., 2., 3., 5., 6. from above. The most impacting changes will be 3. and 6. All other changes are internal-ish.
Comment | File | Size | Author |
---|---|---|---|
#34 | interdiff.txt | 13.47 KB | yched |
#34 | 2165835-34-entity-display-rename.patch | 52.74 KB | yched |
#33 | 2165835-33-entity-display-rename.patch | 39.55 KB | tstoeckler |
Comments
Comment #1
yched CreditAttribution: yched commentedFixed typos
otherwise, +1
Comment #2
tstoecklerHere we go, let's see where we're at.
Comment #3
tstoecklerOh, yeah thanks for 1. I re-edited that because I changed my mind about the config prefixes. I originally wanted to propose
entity.display -> entity.display.view
entity.form_display -> entity.display.form
but the patch now contains
entity.display -> entity.view_display
entity.form_display (no change)
I originally thought the additional 'display' namespace would be neat, but I realized that dots in the config names are also used to separate different entity types and bundles. So I now think the extra dot would be a bit more confusing and it is not worth the API change for form modes.
Comment #4
yched CreditAttribution: yched commented#3: yup, makes sense
Comment #6
tstoecklerSo type-hinting a stricter interface in a protected child method apparently does not work.
Comment #8
tstoecklerSo if I change the config prefix, I should really also change the config prefix... *slapsforehead*
Comment #9
tstoecklerFound one nitpick in self-review.
Comment #11
tstoecklerWeirdo, random test failure.
Comment #12
yched CreditAttribution: yched commented11: 2165835-9.patch queued for re-testing.
Comment #14
swentel CreditAttribution: swentel commentedrerolled
Comment #16
swentel CreditAttribution: swentel commentedComment #18
tstoecklerThis fixes another one introduced in the base field patch.
Comment #19
tstoecklerAlright, anyone want to RTBC :-)
Also thanks @swentel for the re-roll/fixes!
Comment #20
dawehnerI am sorry :(
Given that views has also a display object it seems better to use 'The entity view display object' like before.
This new typehint is just wonderfull.
Comment #21
tstoecklerYeah, you're right that is confusing. Although in the long run it would be pretty wicked if Views displays were *actual* EntityViewDisplays. :-)
I adapted the patch to use "entity view display" consistently. I think the word "object" is superfluous, that's why I didn't go with "entity view display object". You know that it's an object from the typehint; and in general I think it's pretty clear.
For consistency I also updated a bunch of similar code to consistently say "entity form display" instead of entity_form_display of EntityFormDisplay. That's slightly out of scope, but it is sort of related as we're trying to improve consistency here.
Comment #22
yched CreditAttribution: yched commentedWell, Views displays have lots of other things (sorts, filters, arguments...). But yeah in the long run (D9+...) it would be nice to reduce the gap between an EntityViewDisplay and the 'fields" section of a Views display.
#1867518: Leverage entityDisplay to provide fast rendering for fields is somewhat about going in that direction already (at least internally).
"... for entity form and view displays" ?
Other than that, I tend to find that the fully expanded expressions ('the entity form display" / "the entity view display") make the actual sentences more confusing, but I don't have the heart to really bikeshed this :-)
RTBC after fixing the remark above.
Comment #23
tstoecklerHere we go. I shortened that line a bit so that it would fit into one line per our standards.
@yched: I don't really care, if you propose a different nomenclature feel free. This isn't really that hard to re-roll... Personally, I do think it's rather verbose but since it's e.g EntityViewDisplay(Interface) calling the thing "entity view display" makes sense to me.
Comment #24
yched CreditAttribution: yched commentedWell, in the context of a param that is clearly typehinted to EntityViewDisplayInterface or EntityFormDisplayInterface, in a function / method that's in a "view" or "form" domain logic, It would think just saying "the entity display" is unambiguous enough, and more parsable.
(no biggie for the entity_get*_display() functions, #2090509: Optimize entity_get_render_display() for the case of "multiple entity view" will move them to static methods in the Display classes themselves, which will make the context much clearer)
That's bikeshedding though. As said above, RTBC.
Comment #25
alexpottComment #26
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #28
amateescu CreditAttribution: amateescu commentedSo the previous patches were breaking the config schema for this entity type, but the new test added in #2167623: Add test for all default configuration to ensure schema exists and is correct catches it now, nice!
Comment #29
yched CreditAttribution: yched commentedRight. RTBC +1.
Comment #30
yched CreditAttribution: yched commentedSide note : the following change notices will need to be updated.
https://drupal.org/node/2104695
https://drupal.org/node/1875952
https://drupal.org/node/1907792
Comment #31
alexpottI'm not 100% convinced about changing EntityDisplayBase to EntityDisplay - we have 220 abstract classes in core (excluding vendor) and 185 of are called somethingBase. What is the rational here?
I like the rename of EntityDisplay to EntityViewDisplay that makes perfect sense.
Config schema look like they need some work to become consistent
I think the type needs to be updated to entity_view_display.field.[type] to match entity_form_display.field.[type]
Shouldn't this be entity_field_view_display_base so that it matches entity_field_form_display_base? In fact considering the comment above it looks like both these names could be improved. We could remove _field from both of those names.
Nope. It contains EntityDisplay now :) However, as discussed at the top of this comment I'm not convinced about this.
Comment #32
yched CreditAttribution: yched commented- Re: EntityDisplayBase --> EntityDisplay
I don't really remember the discussion that led to that rename- it's possible I might have approved / promoted this :-), but right now I tend to agree with @alexpott and see no real reason to remove the Base suffix here.
- Re: name of entries in config schemas
The situation is a bit confusing right now, because the current config schemas for Entity(View|Form)Displays are kind of bogus : they assume everything in EntityDisplay::$components is a "field widget/formatter + its settings".
That's wrong, there are different kinds of components, but until #1875974: Abstract 'component type' specific code out of EntityDisplay is in, there is no way to differenciate the different "component types" and properly describe their various entries.
We're just lucky that there are only two "kinds of components" in current core (fields / extra fields), and that the set of properties for the latter is currently a strict subset of those for the former, so entity_display.field.[type] works for both.
We'll only be able to clean this as part of or after #1875974: Abstract 'component type' specific code out of EntityDisplay.
But yeah, I guess for now just renaming
- entity_field_display_base -> entity_field_view_display_base
- entity_display.field.[type] -> entity_view_display.field.[type]
would be best for consistency with the form side.
Comment #33
tstoecklerYeah, I'm fine with EntityDisplayBase. Here we go. Also changed a bunch of documentation to not refer to the class name directly.
Comment #34
yched CreditAttribution: yched commentedReroll for fuzz, + done the schema renames mentioned in #31 / #32
This adresses @alexpott's remarks, so back to RTBC.
Comment #35
tstoecklerOops, missed the schema changes. Thanks @yched! RTBC++
Comment #36
yched CreditAttribution: yched commentedRemoved the "rename EntityDisplayBase to EntityDisplay" part from the issue title and summary.
Comment #37
yched CreditAttribution: yched commentedThe API change has been approved in #2031725: Move all entity display interfaces to the core component, and then pushed to this followup.
Also, adding "Avoid commit conflicts", since a lot of patches are doing work around EntityDisplays right now, and getting this in first would help.
Comment #38
alexpottCommitted 655f5a0 and pushed to 8.x. Thanks!
Looks like https://drupal.org/node/1875952 could do with updating.
Comment #39
yched CreditAttribution: yched commentedThanks @alexpott.
Yes, #30 had a list of change records to update :
https://drupal.org/node/2104695
https://drupal.org/node/1875952
https://drupal.org/node/1907792
Comment #40
yched CreditAttribution: yched commentedDone
Comment #41
andypostI changed title https://drupal.org/node/1907792 with a new class
EDIT Also linked all 3 change notices with the issue