Updated: Comment #35

Problem/Motivation

Right now EntityRenderController::viewMultiple() calls entity_get_render_display() once per bundle, resulting in separate entity loads.
We should optimize this, collect all involved bundles and then load the corresponding displays in a single load.

Proposed resolution

- entity_get_render_display($entity, $view_mode) --> static EntityDisplay::collectRenderDisplays($entities, $view_mode)
(a "single-mode" collectRenderDisplay($entity, $view_mode) shortcut is provided)
- entity_get_render_form_display($entity, $view_mode) --> static EntityFormDisplay::collectRenderDisplay($entity, $view_mode)
(no need for multiple rendering on the form side, but the code is moved similarly for consistency, and also optimized a bit on its own)

Remaining tasks

Reviews :-)

User interface changes

None

API changes

See above.
Note that entity_get_render_display() / entity_get_render_form_display() are only used by code that actually performs entity rendering / entity form building, so very little code should actually be impacted.

CommentFileSizeAuthor
#74 2090509-follow-up.patch1.62 KByched
#72 2090509-follow-up.patch2.55 KByched
#70 2090509-follow-up.patch2.72 KByched
#64 2090509-follow-up.patch2.72 KByched
#63 2090509-follow-up.patch2.35 KByched
#60 2090509-follow-up.patch1.89 KBandypost
#18 interdiff.txt643 bytesyched
#18 get_render_display_multiple-2090509-18.patch29.55 KByched
#16 interdiff.txt3.27 KByched
#16 get_render_display_multiple-2090509-16.patch29.54 KByched
#13 interdiff.txt9.56 KByched
#13 get_render_display_multiple-2090509-13.patch29.46 KByched
#12 interdiff.txt7.6 KByched
#12 get_render_display_multiple-2090509-12.patch23.66 KByched
#9 interdiff.txt2.74 KByched
#9 get_render_display_multiple-2090509-9.patch18.01 KByched
#7 get_render_display_multiple-2090509-7.patch17.37 KByched
#5 interdiff.txt2.75 KBswentel
#5 get_render_display_multiple-2090509-5.patch18.78 KBswentel
#3 interdiff.txt998 bytesyched
#3 get_render_display_multiple-2090509-3.patch16.03 KByched
#1 get_render_display_multiple-2090509-1.patch15.05 KByched
#53 get_render_display_multiple-2090509-53.patch19.33 KByched
#49 get_render_display_multiple-2090509-49.patch19.33 KByched
#45 interdiff.txt1.29 KByched
#45 get_render_display_multiple-2090509-45.patch19.13 KByched
#40 interdiff.txt3 KByched
#40 get_render_display_multiple-2090509-40.patch19.13 KByched
#35 interdiff.txt12.92 KByched
#35 get_render_display_multiple-2090509-35.patch18.7 KByched
#34 get_render_display_multiple-2090509-34.patch10.05 KByched
#32 get_render_display_multiple-2090509-32.patch10.07 KByched
#31 get_render_display_multiple-2090509-31.patch10.07 KByched
#24 get_render_display_multiple-2090509-24.patch30.04 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
15.05 KB

Initial patch.
I guess we should do something similar for entity_get_render_form_display() (move it to EntityFormController, but probably no need for making it "multiple"), but I might have no time before I leave on vacation. Posting what I have.

Status: Needs review » Needs work

The last submitted patch, get_render_display_multiple-2090509-1.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
998 bytes
16.03 KB

Fix BlockRenderController, which didn't extend the base EntityRenderController class so far.
Checked with @tim.plunkett, this shouldn't be needed anymore.

Status: Needs review » Needs work

The last submitted patch, get_render_display_multiple-2090509-3.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
18.78 KB

Should fix some tests

The last submitted patch, get_render_display_multiple-2090509-5.patch, failed testing.

yched’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
17.37 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, 7: get_render_display_multiple-2090509-7.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
18.01 KB
2.74 KB

A couple fixes

amateescu’s picture

The patch looks very good, should we do the same for form displays?

Edit: even if not for performance, as we don't usually display more than one entity form on a page, at least for consistency and getting rid of entity_get_render_form_display().

yched’s picture

@amateescu: Yes, the patch needs to do the same on the forms side too. There is no "multi-entities" processing needed for forms, but we should definitely maintain consistency.

yched’s picture

Reroll, + switch functional calls to service code.
Still @todo : form_display

yched’s picture

Similar for form_display

The last submitted patch, 13: get_render_display_multiple-2090509-13.patch, failed testing.

The last submitted patch, 13: get_render_display_multiple-2090509-13.patch, failed testing.

yched’s picture

Last night was the night of silly mistakes...

The last submitted patch, 16: get_render_display_multiple-2090509-16.patch, failed testing.

yched’s picture

Silly is not over it seems.

The last submitted patch, 18: get_render_display_multiple-2090509-18.patch, failed testing.

yched’s picture

Hm. Remaining fails, of course, are because patch replaces entity_get_render_form_display() by EntityFormController::collectRenderDisplay() without converting the remaining calls in edit.module...

This in fact shows that, while EntittyViewBuilder is actually a perfect place to move entity_get_render_display() on the view side, the same is not true for EntityFormController / entity_get_render_form_display():
- EntittyViewBuilder is just a "helper object holding logic for viewing entities", and any code can fetch it through the EntityManager, and call one of its methods. (and #2134887: move field_view_field() / field_view_value() to methods is moving the last remaining external calls to entity_get_render_display()/EVB::collectRenderDisplay() to EVB itself, so this makes even more sense)
- But EntityFormController is not a "helper object", it's the actual entity form, and it makes no sense to create that object if it's not to build that actual form. So edit.module, that currently calls entity_get_render_form_display() to build a one-off form for one single field, cannot call collectRenderDisplay() if it's placed on EFC...

Not sure where those methods would best go then :-/
Maybe the EntityManager ? but then again they only make sense for ContentEntities...
Or maybe static methods on EntityDisplay / EntityFormDisplay, which would also be a good fit for the other functions (entity_get_display() / entity_get_form_display()) - but then again, I liked the idea of moving entity_get_display() and entity_get_render_display() apart from each other, because although they look similar, they are really about different use cases.

Well, unless someone has a better proposal, I'll probably scope back the patch to just the optimization part, keeping the code as sad old functions in entity.inc for now, and leave the question of how we can OO them to a separate discussion...

yched’s picture

Priority: Normal » Major

Also, raising to major, because the current code is pretty bad perf-wise.
(aside from not allowing multiple-load of displays across the multiple entities being views, it also currently does one entity_load too many even for a single view)

fago’s picture

So edit.module, that currently calls entity_get_render_form_display() to build a one-off form for one single field, cannot call collectRenderDisplay() if it's placed on EFC...

I think here lies the problem - if edit implements an entity form it should do so via the regular API for entity forms? I'd not mind an entity form having just a single field.

yched’s picture

In short - can edit.module's "single field" form extend EntityFormController somehow ?

I pinged @wim for feedback.

Wim Leers’s picture

Reroll now that #2031725: Move all entity display interfaces to the core component has landed. Several of the changes being made here were already introduced there. The only change I made is also getting rid of entity_get_render_form_display()'s docs, which wasn't being done yet in the latest patch.

Wim Leers’s picture

Now, the Edit module questions.

  1. We've kind of already had this discussion earlier, in Prague. For starters, we don't want random form alters to affect in-place editing forms. Imagine that the metatags functionality would be altered in, then that only makes sense while editing the full node, not when editing one field of a node.
    So: the form ID must be different.
  2. The long story is at [#7886401-97], in the "Conclusions from "hard meetings" on September 21, 2013 at DrupalCon Prague" section of that comment. Copy-pasted here verbatim for your ease of reading:

    Conclusions from "hard meetings" on September 21, 2013 at DrupalCon Prague

    Gábor and I together with the Field API folks have been looking into getting rid of EditFieldForm and getting the edit form for the Entity instead, and dynamically modifying the EntityFormDisplay, removing (=hiding) all components except the one for the one field that is being in-place edited. The benefit would be: all the filtering+mapping of validation errors that Gábor put together and explained in #89 and #91 would just go away — because EntityFormController would handle it for us. Hurray! Byebye, field_attach_*() & EditFieldForm!
    This is the conclusion we settled on yesterday, in one of the "hard meetings" at DrupalCon Prague.

    Today, I worked on making that happen. And quickly ran into a wall. The problem: EntityFormDisplay only applies to configurable fields, not to base fields (e.g. promoted, revision log message, URL alias…). I could unset() all of those. Which is very inefficient, but it could work. But then it is also still possible for contrib modules to alter the entity form and make changes, which can break that. That can happen because getting rid of EditFieldForm in favor of a "default entity edit form with a dynamically manipulated form mode" still results in a form with the same form ID; we want Edit's in-place editing forms to have different form IDs.
    Everything in the above paragraph (starting at "today") was the conclusion of a discussion with amateescu and yched.

    This is what amateescu and yched decided should happen:

    1. Edit must unfortunately continue to use field_attach_*() for now.
    2. Edit should generate the widget for the specific field that's being in-place edited, but doing that directly would cause problems if we'd do it today, because hook_field_attach_form() (which is called for each widget that gets generated) wouldn't be called anymore.
    3. So, we have to wait until we have something like field_attach_form(), but simpler/cleaner/better/… (I think that will happen in #2095195: Remove deprecated field_attach_form_*()), then use that, and use $entity->validate().

    That means that for now we're condemned to use validation error filtering+mapping.

  3. Conclusion: unless you guys see a clever way out of that, I don't see how we can get rid of EditFieldForm.
  4. BUT please know that the only reason that EditFieldForm::init() calls entity_get_render_form_display() is because of an unimplemented @todo: the idea is that you could configure Edit module to automatically use those field widgets that are configured in a specific form view mode — but that's currently not used at all!

Status: Needs review » Needs work

The last submitted patch, 24: get_render_display_multiple-2090509-24.patch, failed testing.

yched’s picture

Right. Thanks for the reminder, @Wim.

EntityFormDisplay only applies to configurable fields, not to base fields (e.g. promoted, revision log message, URL alias…). I could unset() all of those. Which is very inefficient, but it could work. But then it is also still possible for contrib modules to alter the entity form and make changes, which can break that.

Well, on the 'view' side, modules that add stuff in rendered entities using hook_entity_view() need to check the view mode or the EntityDisplay to see if they really should. I guess the same could apply on the 'form' side. "If form_mode is 'in_place_edit', don't pollute it".
But agreed that that would be probably easy to overlook, and thus would make "edit in place" very brittle.

[edit: moved the rest in the next comment for clarity]

yched’s picture

Actually, I think we have a choice whether:

a) we try to push for treating EditFieldForm as a subcase of EntityFormController (aka an official entity form), and struggle with the quirks like the one above.

b) we treat EditFieldForm as "one of those forms that are not 'official entity forms'", but need to include widgets for all / some fields" - i.e. those that currently use field_attach_form_*() directly, and deal with processing the submits themselves. And we know this is something we need to keep supporting for "inline entity forms".

So my feeling is we might as well go with b), since it provides a core case for the "inline entity forms"-like case.

Meaning, yes, we need to find the right replacements for field_attach_form(), in a place that's not only accessible if you are an EntityFormController. That might mean an EntityFormBuilder - a helper "handler" class like EntityViewBuilder, that is *not* a form, but that the actual forms (EntityFormControiller, EditFieldForm, InlineEntityForm...) would use.

yched’s picture

Wim Leers’s picture

#28: that sounds sensible :)

yched’s picture

Reboot after the discussions in
#2167267: Remove deprecated field_attach_*_view()
#2095195: Remove deprecated field_attach_form_*()

entity_get_render_display() -> static method on EntityDisplay.
We'll see for forms later, just posting for the testbot for now.

yched’s picture

Status: Needs work » Needs review
FileSize
10.07 KB

Silly refactor typo, good thing I forgot to set to "needs review" :-p

Status: Needs review » Needs work

The last submitted patch, 32: get_render_display_multiple-2090509-32.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
10.05 KB

Not done with the typos...

yched’s picture

- Optimize the case of "no display exists in config at all" :
do not call entity_get_display(), which tries to do an additional and useless config load, but create the runtime object directly.

- Streamlined code a bit.

- Applied similar treatment on the form side:
entity_get_render_form_display() -> EntityFormDisplay::collectRenderDisplay()

I'll be leaving entity_get_display() alone in the issue.
Can be discussed separately, and including that here would make the patch size explode.

yched’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

+++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityDisplay.php
@@ -36,6 +37,119 @@ class EntityDisplay extends EntityDisplayBase implements EntityViewDisplayInterf
+    $load_ids = array();
...
+    $displays = $storage->loadMultiple($load_ids);
...
+        $display = $displays[$load_ids[$bundle]];

Display could not exists...

when no IDs found this will load all displays

yched’s picture

@andypost: Nope, it's only loadMultiple(NULL) that loads all entities, loadMultiple(array()) is just a no-op (furtunately - that would be a dangerous WTF)

yched’s picture

Also, the EFQ step guarantees that all $load_ids correspond to existing config entries, so no need to check for existence after loading.

yched’s picture

Filled in missing phpdocs.

I think this should be ready.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -entity API +Entity Field API

The patch looks great to me. There's only minor nitpick that can totally be fixed at commit-time :)

+++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityDisplay.php
@@ -36,6 +37,131 @@ class EntityDisplay extends EntityDisplayBase implements EntityViewDisplayInterf
+   * entity_get_display() should be used instead, in order to avoid inadvertently

Over 80 chars.

amateescu’s picture

Actually, one other thing: shouldn't we add (at least) collectRenderDisplay() to the interface?

The last submitted patch, 31: get_render_display_multiple-2090509-31.patch, failed testing.

yched’s picture

Interface : not sure, I would think not The point is that you call collectRenderDisplay() on the EntityDisplay class, specifically, not on a different one. It's not part of the interface.

yched’s picture

Fix the wrapping error mentioned in #41.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 45: get_render_display_multiple-2090509-45.patch, failed testing.

yched’s picture

No way, bot, interdiff is a comment change...

45: get_render_display_multiple-2090509-45.patch queued for re-testing.

yched’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC

yched’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: get_render_display_multiple-2090509-49.patch, failed testing.

yched’s picture

Status: Needs work » Reviewed & tested by the community
yched’s picture

Status: Reviewed & tested by the community » Needs work

Er - oops.

yched’s picture

Status: Needs work » Needs review
FileSize
19.33 KB

Forgot a couple 'entity_display' / 'entity_view_display' renames.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Green, back to RTBC

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityFormDisplay.php
    @@ -36,6 +36,83 @@ class EntityFormDisplay extends EntityDisplayBase implements EntityFormDisplayIn
    +    $results = \Drupal::entityQuery('entity_form_display')
    

    This looks like it's going to be more expensive than the entity_load_multiple() it replaces. Is this going to run whenever a form is rendered?

  2. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityViewDisplay.php
    @@ -36,6 +37,131 @@ class EntityViewDisplay extends EntityDisplayBase implements EntityViewDisplayIn
    +    $results = \Drupal::entityQuery('entity_view_display')
    

    Same question here.

So I'm not sure without profiling data whether this results in a performance improvement or not - we're replacing individual loads with the EntityQuery on config entities.

yched’s picture

This looks like it's going to be more expensive than the entity_load_multiple() it replaces.

No, that was the point of #2163919: Optimise Config EntityQueries in case there are conditions on the 'id'.

For N bundles, we need to look at Nx2 candidate display entities, and select N entities depending on the 'status' property on the candidates.

Doing so by :
- reading Nx2 config records and checking their 'status' (this is what the EFQ does after #2163919: Optimise Config EntityQueries in case there are conditions on the 'id')
- entity_load_multiple(the N selected entities), for which the config records have already been loaded / statically cached by the EFQ

is going to be more performant than:
- entity_load_multiple(Nx2 candidate entities)
- ditch the ones we don't need

catch’s picture

Status: Needs review » Reviewed & tested by the community

Oh nice! I missed that issue entirely...

catch’s picture

Title: optimize entity_get_render_display() for the case of "multiple entity view" » Change notice: optimize entity_get_render_display() for the case of "multiple entity view"
Status: Reviewed & tested by the community » Active
Issue tags: +Missing change record

Committed/pushed to 8.x, thanks!

Needs a change notice.

andypost’s picture

+++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityFormDisplay.php
@@ -36,6 +36,83 @@ class EntityFormDisplay extends EntityDisplayBase implements EntityFormDisplayIn
+   * @param \Drupal\Core\Entity\EntityInterface $entity
...
+  public static function collectRenderDisplay($entity, $form_mode) {

+++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityViewDisplay.php
@@ -36,6 +37,131 @@ class EntityViewDisplay extends EntityDisplayBase implements EntityViewDisplayIn
+   * @param \Drupal\Core\Entity\EntityInterface $entity
...
+  public static function collectRenderDisplay($entity, $view_mode) {

needs follow-up to fix type-hint

amateescu’s picture

Shouldn't we tipe-hint with ContentEntityInterface?

andypost’s picture

ContentEntityInterface makes sense, Berdir pointer about that somewhere but I'm not sure

yched’s picture

FileSize
2.35 KB

Oops, sorry for the typehints - I can't even pretend that's because they were absent from the old functions, they were not :-/

re: ContentEntityInterafce - yes, @Berdir asked for that in #2167267-78: Remove deprecated field_attach_*_view(), and he's right.

yched’s picture

FileSize
2.72 KB

Sigh. Forgot one. Tired :-/

yched’s picture

Title: Change notice: optimize entity_get_render_display() for the case of "multiple entity view" » [Followup] optimize entity_get_render_display() for the case of "multiple entity view"
Issue tags: -Missing change record

Status: Needs review » Needs work

The last submitted patch, 64: 2090509-follow-up.patch, failed testing.

The last submitted patch, 63: 2090509-follow-up.patch, failed testing.

The last submitted patch, 63: 2090509-follow-up.patch, failed testing.

Berdir’s picture

yched’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

Hm, yes, it's actually too early for that typehint on the Form side, EntityFormController still tries to load an EntityFormDisplay for non-Content entity types.
That is being cleaned up in #2095195: Remove deprecated field_attach_form_*(), but meanwhile best we can do is typehint on ContentEntityInterface only on the "view" side.

Status: Needs review » Needs work

The last submitted patch, 70: 2090509-follow-up.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
2.55 KB

Yeah, I was really tired. #70 was just the same patch as #64.

amateescu’s picture

That is being cleaned up in #2095195: Remove deprecated field_attach_form_*(), but meanwhile best we can do is typehint on ContentEntityInterface only on the "view" side.

Then why don't we just update EntityViewDisplay and leave EntityFormDisplay to #2095195: Remove deprecated field_attach_form_*()? :)

yched’s picture

FileSize
1.62 KB

Right, updated patch in #2095195: Remove deprecated field_attach_form_*() now adds the right typehint to EntityFormDisplay, so we might as well leave it alone here.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Great!

webchick’s picture

Title: [Followup] optimize entity_get_render_display() for the case of "multiple entity view" » Optimize entity_get_render_display() for the case of "multiple entity view"
Status: Reviewed & tested by the community » Fixed

Well that was confusing. :) However, committed/pushed that docs clean-up to 8.x.

Status: Fixed » Closed (fixed)

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