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

  1. Rename EntityDisplay into EntityViewDisplay
  2. Change the entity type id of entity view displays from entity_display into entity_view_display
  3. Change the entity type label of entity view displays from Entity display into Entity view display
  4. Change the config prefix of entity view displays from entity.display into entity.view_display.
  5. Rename hook_entity_display_alter() to hook_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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Issue summary: View changes

Fixed typos

otherwise, +1

tstoeckler’s picture

Issue summary: View changes
Status: Active » Needs review
Related issues: +#2031725: Move all entity display interfaces to the core component
FileSize
54.36 KB

Here we go, let's see where we're at.

tstoeckler’s picture

Oh, 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.

yched’s picture

#3: yup, makes sense

Status: Needs review » Needs work

The last submitted patch, 2: 2165835-1.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.83 KB
53.08 KB

So type-hinting a stricter interface in a protected child method apparently does not work.

Status: Needs review » Needs work

The last submitted patch, 6: 2165835-6.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
33.61 KB

So if I change the config prefix, I should really also change the config prefix... *slapsforehead*

tstoeckler’s picture

FileSize
502 bytes
33.74 KB

Found one nitpick in self-review.

Status: Needs review » Needs work

The last submitted patch, 9: 2165835-9.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
33.74 KB

Weirdo, random test failure.

yched’s picture

11: 2165835-9.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: 2165835-9.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
59.76 KB

rerolled

Status: Needs review » Needs work

The last submitted patch, 14: 2165835-14.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
33.87 KB
1.74 KB

The last submitted patch, 16: 2165835-16.patch, failed testing.

tstoeckler’s picture

FileSize
819 bytes
33.95 KB

This fixes another one introduced in the base field patch.

tstoeckler’s picture

Alright, anyone want to RTBC :-)

Also thanks @swentel for the re-roll/fixes!

dawehner’s picture

Status: Needs review » Needs work

I am sorry :(

  1. +++ b/core/includes/entity.inc
    @@ -571,19 +571,19 @@ function entity_view_multiple(array $entities, $view_mode, $langcode = NULL, $re
    + *   The view display object associated to the view mode.
    ...
    - *   The display object associated to the view mode.
    
    @@ -614,7 +614,7 @@ function entity_get_display($entity_type, $bundle, $view_mode) {
    + *   The view display object that should be used to render the entity.
    ...
    - *   The display object that should be used to render the entity.
    

    Given that views has also a display object it seems better to use 'The entity view display object' like before.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilderInterface.php
    @@ -17,8 +17,8 @@
    +   * @param \Drupal\Core\Entity\EntityViewDisplayInterface[] $displays
    

    This new typehint is just wonderfull.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
5.05 KB
64.89 KB

Yeah, 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.

yched’s picture

it would be pretty wicked if Views displays were *actual* EntityViewDisplays

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

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplay.php
@@ -0,0 +1,355 @@
+ * Base class for config entity types that store configuration for entity forms
+ * and displays.

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

tstoeckler’s picture

FileSize
627 bytes
64.86 KB

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

yched’s picture

Status: Needs review » Reviewed & tested by the community

Well, 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
error: core/modules/entity/entity.install: does not exist in index
error: patch failed: core/modules/forum/config/entity.display.taxonomy_term.forums.default.yml:1
error: core/modules/forum/config/entity.display.taxonomy_term.forums.default.yml: patch does not apply
error: patch failed: core/profiles/standard/config/entity.display.node.article.default.yml:1
error: core/profiles/standard/config/entity.display.node.article.default.yml: patch does not apply
error: patch failed: core/profiles/standard/config/entity.display.node.article.teaser.yml:1
error: core/profiles/standard/config/entity.display.node.article.teaser.yml: patch does not apply
amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
36.12 KB

Rerolled.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2165835-26-entity-display-rename.patch, failed testing.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
36.63 KB
522 bytes

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

yched’s picture

Right. RTBC +1.

yched’s picture

Side 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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'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

# Overview configuration information for view mode or form mode displays.
entity.view_display.*.*.*:
#...
    content:
      type: sequence
      label: 'Field display formatters'
      sequence:
        - type: entity_display.field.[type]

I think the type needs to be updated to entity_view_display.field.[type] to match entity_form_display.field.[type]

# Schema for the base of the view mode display format settings.
entity_field_display_base:

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.

/**
 * @file
 * Contains \Drupal\entity\EntityDisplayBase.
 */

Nope. It contains EntityDisplay now :) However, as discussed at the top of this comment I'm not convinced about this.

yched’s picture

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

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
7.36 KB
39.55 KB

Yeah, I'm fine with EntityDisplayBase. Here we go. Also changed a bunch of documentation to not refer to the class name directly.

yched’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
52.74 KB
13.47 KB

Reroll for fuzz, + done the schema renames mentioned in #31 / #32

This adresses @alexpott's remarks, so back to RTBC.

tstoeckler’s picture

Oops, missed the schema changes. Thanks @yched! RTBC++

yched’s picture

Title: Rename EntityDisplay to EntityViewDisplay and EntityDisplayBase to EntityDisplay in accordance with their interfaces » Rename EntityDisplay to EntityViewDisplay in accordance with its interface
Issue summary: View changes

Removed the "rename EntityDisplayBase to EntityDisplay" part from the issue title and summary.

yched’s picture

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

alexpott’s picture

Title: Rename EntityDisplay to EntityViewDisplay in accordance with its interface » Change record: Rename EntityDisplay to EntityViewDisplay in accordance with its interface
Status: Reviewed & tested by the community » Active
Issue tags: -Avoid commit conflicts +Missing change record

Committed 655f5a0 and pushed to 8.x. Thanks!

Looks like https://drupal.org/node/1875952 could do with updating.

yched’s picture

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

yched’s picture

Title: Change record: Rename EntityDisplay to EntityViewDisplay in accordance with its interface » Rename EntityDisplay to EntityViewDisplay in accordance with its interface
Status: Active » Fixed
Issue tags: -Missing change record

Done

andypost’s picture

I changed title https://drupal.org/node/1907792 with a new class

EDIT Also linked all 3 change notices with the issue

Status: Fixed » Closed (fixed)

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