Store entity display settings in EntityDisplay config objects

It looks like EntityDisplay could be used for entityRenderController

I'll try to give this another round of review soon. Do you guys still have pending code changes ?

And yeah, how this might tie to the render controller is one thing I want to explore.

Updated issue summary.

Also, just occurred to me:
- The yml file should list elements in the correct order. The save() method needs to order elements by weight, and put the 'content' array in the correct order before handing to parent::save().
- Field UI submit handler should renumber weights starting from 0, and going by increments of 1 (tabledrag sometimes produces fairly large weight values.
Will make git diffs on the display.yml files more friendly

I do plan to have a close look at the code, notably at the render side. Will try to do so this weekend.

Ahoy! EM stands for emphasis, not code. :P ;)

On the questions (both AFAIK):

1) We're preparing the view on multiple entities, for performance, and because of multidimensional extensions being involved; i.e., if you pass the full stack of entities being prepared for view, then an integrating module can go ahead and fetch all the data for all the entities/fields in one go, instead of issuing individual storage lookups for every single entity. The multiple processing is definitely preferred from a performance perspective.

2) entity.inc got moved into an entity.module in D8 before we had the PSR-0 and "frameworkification" master plan. Back then, the idea was: "It invokes hooks, so it should be a module." Later, we realized that entity.module is a pure, architectural service component and does not do anything on its own. Also, we had introduced entity classes and storage classes, which reduced its procedural aspect. Therefore, we moved it it "back", but not into includes/entity.inc, but Drupal\Core\Entity. :)

HTH.

Yeah, a quick str_replace from <em> to <code> would make that comment more readable :)

Oh. And yeah, that question on why Entity is no longer a module is great. I actually considered that we should do the same with Field API, once we're done with pluginification and configurationification, since, frankly, what's really left except of classes then?

@sun, re: field module -> component
Yeah, dunno, we still have a couple hooks in here. I guess we'll see when we get there.
Right now it's more the lack of an entity.module that's annoying :-) We're introducing config entities that really belong to the entity system here, not field module.

That's also why we are moving the display object now to the entity. Stalski is going to commit an update soon. The render controller will load the display then, and place it on the entity.

Tell me when I can review. I'd really want to validate the render approach before we go to far along.

More concise formulation for the title.

So, I gave the code a first round of review, and pushed a couple changes, that is mostly 89d1179:
- Renames _field_entity_display_load() to entity_get_display(),
- moves it to entity.inc,
- removes the "switch to 'default' mode" logic (this only happens at render time, in EntityRenderController; other than that, we need to be able to do stuff with the diisplay object for a view mode that's not currently "enabled").

Attached is an interdiff with my whole set of changes.

Other than that, I'm doing some refactoring on the render callstack, but I'm not fully settled on on the best approach yet, will try to come back with some code soonish.
Meanwhile, I created #1838034: [EntityDisplay] Get rid of field_set_display(), would be cool if you guys could look into this.

Also, merged current 8.x into the branch

I see you renamed display_settings to display_properties. I saw it more as settings, because it are the display settings for that field.
Should we keep display_properties, and use it then everywhere? Because now it's mixed.

@zuuperman #24:
I'm trying to settle on 'settings' being the settings for a specific formatter or widget (or more generally 'plugin').
So yeah, $display_properties for "the display parameters used by a given field". $display_options would work too, but the current code is semi-consistent on $display_properties / $widget_properties right now, so let's keep this.

Merged latest 8.x and pushed some important changes, most notably :
- 228752c Inject display objects as params in field_attach_[prepare]_view().
This solves the "how do we pass display objects down to field_invoke_method()" question.
Previous code placed the $display in the $entity (DisplayEntityBase), which is a hack :-). The entity is the thing we're displaying, we shouldn't use it as a not be a dumping ground for "temporary" information about how we are rendering it. D6 had the awful $node->build_mode property, we removed that in D7, let's not reintroduce a similar pattern.
This change also restores field_invoke_method_multiple() as a generic method not tied to a specific method, we'll need it for other ops (load) when doing "field types as plugin".
- fccc601 Remove DisplayEntityBase class, not needed anymore.
- 7794307 Remove $view_mode param from f_a_*view(), inject it in the Display objects.
We still need to know down the stack what was the original view mode that was requested, even if we're backing up to 'default'. But this information is carried in the Display object (at run time, this doesn't get saved in the CMI file)

Thus, new signatures are :
field_attach_view($entity_type, Entity $entity, EntityDisplay $display) - "display the fields in this entity using these display properties"
field_attach_prepare_view($entity_type, Entity $entities, array $displays) - "prepare those entities using these display properties"
Which looks pretty reasonable IMO.

Next, I'm working on removing FieldInstance::getFormatter() and moving it somewhere else. Given that the $instance doesn't hold the display properties anymore, it has no reason to be the intermediary for instanciating a formatter.

Yeah, $entity_type could definitely be dropped, but it needs to be done for all field_attach_*() functions, so not for that patch.

Per Stalski's request, created new subtasks, that can be worked on independently of the work on the rendering pipeline :
#1847130: [EntityDisplay] Write upgrade path
#1847132: [EntityDisplay] Convert view modes config to CMI
(that's in addition to #1838034: [EntityDisplay] Get rid of field_set_display())

Updated issue summary.

@yched

We got some caching issues in tests. When formatter settings are changed on runtime, the formatter doesn't get rebuild.
Example: LinkFieldTests
in a foreach, all the different format settings are tested.

http://drupalcode.org/sandbox/yched/1736366.git/commitdiff/0ed44374c5551...

I think we got 4 options (for now, as i thought you were planning to move getFormatter away from fieldInstance):
1) leave it the way i committed. When users change a formatter on runtime, they have to clear the old formatter themselves.
2) The code that's now in LinkFieldTests moves to EntityDisplay->save(). For every field in the content array, the formatter gets resetted
3) In EntityDisplay->save(), field_info_cache_clear is called. Clearing all the field cache.
4) Remove resetFormatter again, and users should call field_info_cache_clear themselves.

Quick run through testbot so we know what tests are left for conversion. Moving it back to sandbox after the test.

Aw, so this finally hit us.
Yeah, that's why persisting formatters in the instance is problematic.
Everything works in "regular" situations where the $display isn't changed during the request, but we definitely want to allow code like this :

$display = something
field_attach_view($entity, $display);
$display->change_settings_for_field_foo;
field_attach_view($entity, $display);

without people going "wtf why doesn't it work".

Persisting in one place based on definitions stored in another place is a asking for trouble. That's why formatters were persisted in the $instance so far, and that's why they should be persisted in the EntityDisplay now.

I have been working on a branch that does that. I'll merge with latest and see how it goes with tests, and I'll report back.

Oh, crap - I did not intend to set back to "D8 Field API", and it seems it wiped the test results :-/

I've been using #1848068: Helper issue for "EntityDisplay" to post patches for the bot today.
I reuploaded patch #32 over there : #1848068-4: Helper issue for "EntityDisplay" (comment #4)

Added subtasks

I pushed a couple important changes :

- 327da51 Persist formatters in the EntityDisplay, as per #30 / #34, so we can ditch them when the underlying definition changes.
The actual formatter creation still happens in $instance->getFormatter() (other code that needs to create formatters outside of the entity_view() callstack use that), but nothing is persisted there.
Thus, FieldInstance::resetFormatter() is removed, not needed anymore.

- 45ee059 Remove the $add_default parameter in EntityDisplay::getContent(). Providing defaults for 'hidden' fields was mainly done to ease the job of Field UI "display overview" code, but IMO that's Field UI's problem to decide how its form should behave for hidden fields.
If a field has no entry, getContent() returns NULL, and the calling code can act accordingly.

- e19207e Replace hook_field_display_alter() & hook_field_extra_fields_display_alter() by hook_entity_display_alter().
The new hook is called only once per entity_view_multiple(), while the others were called once per field & per entity. Thus, less hook invocations, and this simplifies the code flow quite a bit.

Tests are not too bad. Remaining failures are #1848858: [EntityDisplay] Add hidden extra fields to the display object so they are always stored, and a new fail introduced by #1292470: Convert user pictures to Image Field (probably only some test code that needs to be adapted, haven't looked into it so far).

I intend to run a couple perf tests tomorrow, and bring an initial patch to the issue queue. We need feedback from core committers see if they would take that as a post Dec 1st change, or if this needs to be done before feature freeze :-/.

Attached is the current patch against D8. No interdiff, the HEAD merges makes it difficult to generate.

Pushed a couple changes after doing benchmarks, and merged latest 8.x.
Preparing a post for the core queue :-)

Core issue was created at #1852966: Rework entity display settings around EntityDisplay config entity.
I think we can close this one now - the separate sub issues should still be useful though.

added subtask