Rework entity display settings around EntityDisplay config entity

Patch attached,
plus a patch without the updates in test files, for better reviewability of the code changes.

re #2:
field_get_actual_view_mode() should move over to entity_* namespace eventually. It just stays in field.module for now because field module still handles "should this view mode use its own settings or simply reuse 'default'" config values. When moving those over to CMI (in a separate issue), this will move over to entity.inc.

On the actual need for the function, we need to be able to work with the display object for a given mode even if the mode is currently set to use 'default' at render time. Otherwise some code that intends to change 'rss' display might end up altering "default' (i.e. node/%nid)

This being said, there might be better ways in this area. Maybe this could just be an extra param on entity_get_display(). Mulling on this.

Did some benchmarks :

Setup: 2 node types, 10 text fields + 1 body field each
'full' displays all fields
'teaser' displays 5 text fields + the trimmed body
+ 3 other view modes with the same config as 'teaser'

ab runs :

A - node/1
HEAD: 86.565 ms
patch: 81.497 ms -> -5,85%

B - custom page rendering 10 teasers (5 of each type) with entity_view_multiple()
HEAD: 143,370 ms
patch: 143,774 ms -> +0,3%, difference is within std dev

Some xhprof investigation on this last case shows that :
entity_view_multiple() inclusive wall CPU drops by 7,5%
but node.tpl includes a user_view($author, 'compact') - that's 10 individual user_view(), so that's a series of 10 entity_load() on the $entity_display ConfigEntity for user compact mode.
config entity loads are not statically cached, neither are config() reads, so this eats all the performance gains

Adding a hacked-in static cache in entity_get_display() (see attached interdiff) produces :

Bbis - same as B with static cache on loaded entity displays
HEAD: 143,370 ms - (same run than B)
patch: 137,661 ms -> -4,24%, but MemUse +1,9% (peak +1,7) - cost of the static cache...
(memory use was a wash for A and B)

Independently of this patch, it seems we could really optimize node displays by finding a way to group all those individual user_view() in one single user_view_multiple() and then dispatch to the right nodes.

[edit: opened #1853378: Optimize user_view('compact') in template_preprocess_node()]

Oh, forgot to attach the static cache hack mentioned in #5

@swentel : yay at the upgrade path ! smartly done too, & with tests :-)

The upgrade should also remove the 'display' part of the field_bundle_settings_[entity_type]__[bundle] variables.
That means iterating on all entity types & bundles though, and I don't think we want to use entity_get_info() in update hooks.
So that could mean getting the global $conf array, and iterating on all entries that start with field_bundle_settings_* :-/

Pushed the code from #11 in the sandbox, and added a @todo for that.
Also, removed the lines pointed by @Berdir, they should not be needed now - let's see...

New patch attached, with an updated "without test changes" patch for reviewability.

(side note: it's really easier on me if code changes are done in the sandbox rather than in patches ;-) )

added link to the sandbox

Update on the benchmark side:

As pointed in #5 above, the user_view() added by #1292470: Convert user pictures to Image Field in each rendered node.tpl skews benchmarks a bit. This is dealt with in #1853378: Optimize user_view('compact') in template_preprocess_node().

So in order to only asses the impact of this patch on entity_view() / entity_view_multiple(), new benchmarks with the 'display user pic in nodes' theme setting toggled off:

Setup: 2 node types, 10 text fields + 1 body field each
- 'full' mode displays all fields
- 'teaser' mode displays 5 text fields + the trimmed body
+ 3 other view modes with the same config as 'teaser'

A - node/1 - single entity_view()
8.x : 81.519 ms
patch: 79.305 ms -> -2,7%

B - custom page rendering 10 teasers (5 of each type) - single entity_view_multiple()
8.x : 131.780 ms
patch: 125.971 ms -> -4,4%

Forgot to attach the patch in #14...

added more specific perf numbers

add section for performance

headers

Pushed a couple changes:

- 993eea4 Completed upgrade path: handle the 'display' entry in field_bundle_settings_* variables (+ added more tests)

- 8493d71 Fluent API : EntityDisplay::setContent() & deleteContent() now return $this, allowing code like :

entity_get_display('node', 'forum', 'default')
  ->setContent('taxonomy_forums', array(
    'type' => 'taxonomy_term_reference_link',
    'weight' => 10,
  ))
  ->save();

- c8f99c6 Fix TermTest fails.

unfinished sentence

Forgot this one, went in before the commits mentioned in #19 :

- 8252222
Store 'extra fields' in the display object even when they are set to hidden. Unlike fields, 'extra fields' have no CRUD and can appear any time. We need to track those for which we have actual settings vs those that we don't know about.

Pushed :

- 7e2dae7 Move all code pertaining to formatter configuration and instanciation out of FieldInstance and into FormatterPluginManager
- bf3aedf, ba00dc7 Minor code reordering.

I'm more or less done with major reshuffle here for now, and we're green. What this needs now are reviews :-)

Most important @todo left is to abstract out the code specific to either "field API fields" or "extra fields" from EntityDisplay.
I'd rather get that done in a followup, but that's depending on community feedback.

1st file is patch without updates for API changes in test classes, easier to review.
2nd file is the full patch.

Thanks for the feedback :-)

- Pushed e6e4571 for the code style errors pointed in #21.

- #21 / code commented out in tests : Yes, those are tests that are no longer applicable (test the behavior of $instance['display']). I'm starting a new test class for the EntityDisplay objects, where the equivalent tests will live (I'll update the OP to clarify that it's still a TODO). Meanwhile, keeping this commented out so that we don't forget it's there.

- Agreed that naming needs to be improved. More thoughts on that in a followup comment.

update issue

re @fago #24/#26:

this seems to depend on field api. What about non-fieldable entities? Is support for them planned also?

This is not more dependent on fieldable entities than what we have right now (with field_attach_*() code in EntityRenderController::buildContent()), and the code won't break when ran on non-fieldable entities (like the current code doesn't) : field_info_instance() returns NULL, and field_attach_*() are no-ops.

But yes, this is not ideally decoupled yet. My plan is to introduce the notion of 'component type' :
- Each component type ("field API field", "extra field", "field_group", ... possibly "block") has a handler class (one object of each is enough for the whole request).
- The EntityDisplay defers handling of setContent() / getContent() methods to the relevant component type handler, and just stores the result (i.e no more massaging code specific to fields or extra fields in EntityDisplay methods).
- Those handler classes possibly take care of the actual rendering of the components of their type as well (i.e no more field_attach_*() calls in EntityRenderController::buildContent())
- They possibly also take care of outputing the UI elements for configuring the components in field_ui (probably renamed entity_ui at some point, and contains no more code specific to Field API).
- Registration of those handler classes is not fully clear yet. Plugins, entries in the DIC, ... not decided.

However, as mentioned in the OP, I'd rather take incremental steps, and keep a limited scope for this patch : introduce EntityDisplay and use it in the rendering flow. Coupling between Field API and Entity API is not made better just yet.

Having formatter support the new entity fields would be superb

It would, but that speaks about the formatters API and what formatters are able to work with (currently, a $field and $instance definition array), and is totally orthogonal to this patch.
Extra fields remain extra fields, aka "an arbitrary piece of render array that we don't know much about". We have them, and the patch doesn't try to remove them or turn them into something else.

In short: that's another discussion, and the patch doesn't make things worse towards that goal. I'd say it makes them slightly better by providing a place where the display settings for "EntityNG fields" could be stored.

Updated TODO : tests for EntityDisplay

re #29 / #30: cool :-)

re #30 / 31, ReenderableInterface :
Yeah, I don't know :-) Some people advocate this is in fact BlockInterface - i.e. everything should be a block. But that would be a massive code change in many places, and the performance impact is still highly unclear.
The proposal in #28 does not go for "the stuff we render need to be objects", but rather "the stuff we render needs to be associated with a handler class (implementing a generic DisplayComponentHandler interface or something) that knows how to render it, build a configuration form, save/load/massage its configuration...". That's much less objects to instanciate.

But well, those discussions are typically the ones I'd rather have in a followup, to not derail that patch too much :-)

So, regarding naming, definitely agreed that this needs some more consideration.

Current API is :

$display = entity_get_display($entity_type, $bundle, $view_mode);
// Sets the $name component to be displayed, with the options specified in $array
$display->setContent($name, $array);
// Sets the $name component to be displayed, with the default options for this component (and weight = at the bottom):
$display->setContent($name);
// Sets the $name component to be hidden:
$display->deleteContent($name);
// Gets the options that were set for the $name component:
$display->getContent($name);

We need to :
- Find better names for the methods.
- Agree on how we call the $array param. Current code is a mix of "display settings" & "display properties" (or "settings" and "properties" depending on context).

I tried to get away from "settings", because of the clash with formatter settings.
I kind of wish that we managed throughout core to more or less consistently keep the term "settings" for "the configuration that's specific to a given plugin implementation and will be different for another plugin implementation" - as opposed to "the collection of properties that are going to be the same across all plugins of a given type". Keeping a mental separation proved very helpful with CCK / Field API, and that's kind of an underlying idea of #1786550: Plugin configuration.

My proposal would be $options, or $configuration - which is actually pretty close to what Plugin API's PluginManagerInterface uses.

So what about:

$display->showComponent($name, $options);
$display->showComponent($name);
$display->hideComponent($name);
$display->getOptions($name);

(Heh, crosspost :-p)

Hmm, thinking a bit more:
- show / hide is tempting but probably a bit misleading. We're not actually showing stuff here, but setting up a config.
- $display->getOptions($name) === NULL meaning "the component is hidden" is not fully obvious.

So, new proposal:

$display->addComponent($name, $options);
$display->addComponent($name);
$display->removeComponent($name);
$display->getComponent($name);

#38 : Agreed.

$display->addComponent('foo', $options);
$display->addComponent('foo', $options_2);

makes it sound like you're adding 'foo' twice, while you're in fact just overwriting the options.

So, back to the basics ?

$display->setComponent($name, $options);
$display->setComponent($name);
$display->removeComponent($name);
$display->getComponent($name);

heh, full circle back to what we currently have, only renaming Content to Component :-) - which is fine by me.

Pushed :
- 0c33a18 Added tests for EntityDisplay.
- 3729187 Removed the old tests for $instance['display'] that had been commented out so far.

So, two proposals for the API :

Current patch :

$display = entity_get_display($entity_type, $bundle, $view_mode);
$display->setContent($name, $array);
$display->setContent($name); // // Applies default options
$display->deleteContent($name);
$display->getContent($name);

Proposal A)

$display = entity_get_display($entity_type, $bundle, $view_mode);
$display->showComponent($name, $options);
$display->showComponent($name); // Applies default options
$display->hideComponent($name);
$display->getComponent($name);

Proposal B)

$display = entity_get_display($entity_type, $bundle, $view_mode);
$display->setComponent($name, $options);
$display->setComponent($name); // Applies default options
$display->removeComponent($name);
$display->getComponent($name);

@KarenS, @aspilicious, @andypost - what do you think ?

added link to "plan for uncoupling entity API / field API

DONE: added tests

Updated for method renames.

Alright, so, pushed :
- 8786856 rename EntityDisplay methods as per #41-B)
- 735582f harmonize variable naming around $options / $display_options (instead of $settings, $properties and variants)

New patches against 8.x

#45: entity_display-1852966-45.patch queued for re-testing.

Pushed :
- e007db9b Fixes / removes a couple @todos
- 0aa1d4b Renames the $context['display'] entry passed to hook_field_attach_view_alter() to 'display_options', as per the new naming convention.
- 7a1f07f fix $display_options passed to hook_field_attach_view_alter() not being the "prepared" version (with defaults merged in)

Still one @todo regarding the upgrade path and we should be good.

TODO -> DONE

Merged 8.x, and pushed:
- 4ca1d33 Fix missing entries in CMI files created during upgrade, and play nice with displays already existing - "user picture" update currently runs before ours, and directly creates displays, and $instances with no 'display' entries.
- 3e92956 Now that #1857074: Re-introduce entity.module as a configuration owner is in, move EntityDisplay over to the new entity.module. This includes moving CMI files over to entity.display.* instead of field.entity_display.* so far
This means people having already applied and tested the patch will need to start over, or manually rename their existing CMI files (including the manifest and its content)

With this, no more @todos pending, we should be good to go for final reviews :-)

Oops. Apart from a typo in EntityDisplayTest, easily fixed, the other fails are upgrade tests.

#1857074: Re-introduce entity.module as a configuration owner forgot to include an upgrade function to enable entity.module.
Even after adding it, we still seem to hit some update dependency issues here though. Investigating.

Doesn't prevent reviewing the patch itself :-)

Was a bit harder than I thought. Essentially, user.module's hook_update_N() for user_picture field cannot reuse the same function for upgrade and initial install, because the same set of APIs do not apply in both cases.
Those fixes would make sense as a followup for #1292470: Convert user pictures to Image Field, but we're modifying that code here anyway.

Pushed:
- f9e132e Fixes EntityDisplayTest fails, silly typo
- 8a37cd9 Fix user_picture upgrade path. What this does is:
Use different code for upgrade (uses helper functions that are kosher for upgrade) and initial standard profile install (uses regular APIs).
The code used in standard.install is kept in a separate function, just so that UserPictureTest can keep using the minimal profile, since that seemed like the original intent.
Reworked our helper function that lets you act on EntityDisplay CMI files directly in upgrades, and moved it to entity.install : _entity_update_8000_get_display_config()

Posted a separate patch in #1860418: Fix user_picture field creation for the user_picture upgrade fixes.

Added API change

- Removed call feedback for post Dec 1st commitability :-p
- Added dependencies

typo

Removed done TODOs, reordered sections a bit

The missing upgrade to enable entity.module got committed in #1857074: Re-introduce entity.module as a configuration owner, so this will need a reroll.
I'm hoping we can get #1860418: Fix user_picture field creation in quick too, though.

Meanwhile, we really need some reviews / RTBC agreement here :-)

Thanks @swentel :-)

- Merged latest 8.x
- Fixed the 80 chars wrapping errors
- The lines showing merge conflicts were not in the actual patch, but only in the 'no-test-changes' patch (bug in the way I generate one from the other), so it didn't affect bot runs. Fixed now, though :-)

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayStorageController.phpundefined
@@ -0,0 +1,37 @@
+  protected function getProperties(EntityInterface $entity) {

Why does it extend on the default one ?

I'm not sure I understand the question :-)
EntityDisplayStorageController extends ConfigStorageController because we need the other base methods unchanged, and getProperties() doesn't call the parent implementation because its own logic is enough.

We do this because EntityDisplay has public properties that we don't want to get saved in the CMI file. The default behavior in ConfigStorageController is to store any public property. If you want a different behavior, getProperties() is the method to override.
That's discussed in #1849480: Allow ConfigEntity classes to control how they are saved,but it's the official current way (that's how ViewStorageController does it currently)

Thanks a lot for the review Tim !

Fixed the styling / typo issues, below are answers for the more elaborate points :

1.

+function entity_get_display($entity_type, $bundle, $view_mode) {

I realize this is called a lot, but it seems to be a new pattern. How often is it loaded, and how often is it created? And is that situational, can the calling code pick one?

The pattern of "read from config or create a fresh empty object ready to use" adresses the fact that you cannot know whether the config store holds an existing, configured display object for any arbitrary [entity_type, bundle, view_mode] tuple.
Displays are only created when actually configured and saved (though code or the UI). E.g until you configure 'RSS' on article nodes to actually use dedicated display settings instead of 'default', there is no config file in the system.
View modes currently come and go silently, and even if we had an observable CRUD cycle on view modes, we wouldn't go and create tens of empty display config files for each entity bundle on the fly.
So this "get current or fresh empty one" pattern is incredibly helpful DX-wise. If not for it, all code currently calling the function would need to do the if (config exists) {return config} else {entity_create()} logic themselves.

And, no, i don't think we need an option to specify a preferred behavior. The function does "give me what's configured". If nothing gets configured, you get the object that's equivalent to "nothing configured" to work with. If what you want is not "what is configured" but a fresh object, entity_create() is still the official way.

I'll try to add some doc in the PHPdoc for the function. KarenS already requested it above.

2. ERC::viewMultiple() / ERC::buildContent() receive an array of entities with arbitrary keys, and use those as the keys of the resulting render array, while hook_entity_prepare_view() / field_attach_prepare_view() expect an array of entities keyed by entity ID.
The current code already does this internal rekeying, the patch doesn't change that.

3. hook_entity_view_mode_alter() / hook_entitiy_display_alter()
We have both for the same reason we currently have hook_entity_view_mode_alter() & [hook_field_display_alter() + hook_field_extra_field_display_alter()]. hook_entitiy_display_alter() only replaces the last two.
hook_entity_view_mode_alter() lets you alter displayed view mode on a per entity basis - use case is "I want to display sticky nodes differently, using settings stored in a teaser_sticky view mode" - see #1154382: View mode no longer can be changed where it was introduced.
Then, for each of the view modes used, we collect the actual display settings configured for the view mode in each bundle (the 'display"), and you can alter those settings. The only difference this patch introduces is that you alter it as a whole instead of "field by field / extra-field by extra-field".

4. Right, added "uuid" in entity_keys in the annotation. We don't have a "label", and we have other config entity types with no "label" entity_key in core, so I left that one out.

5. Fixed

6. $content should probably be protected, true. Will try to roll that in asap. The other ones need to be readable from the outside.

7. 8. 9. Fixed

10. getHighestWeight() : return NULL or -1 if empty ?
Returning -1 would be the same as "there are components, the highest is at -1. It's like "I'll give you -1, because I know what you'll want to do next is put a component at this value +1, and then it's handy because it's gonna be 0". Feels like lying, a bit :-).
No strong feeling, but if that's fine I'd rather keep the current.

11. field_info_instance(), field_info_field(), field_info_extra_fields() are already reading statically cached data, so we don't need nor want to statically cache them somewhere else :-)

12. 13. Fixed

14. Will look into this. DrupalUnitTestBase was a little fragile last time I played with this.

15. 16. Fixed

17. "The code in FormatterPluginManager::getInstance() should be moved into a custom Mapper"
No, why ? AFAIK there's no enforcement that getInstance() *has* to delegate to a separate Mapper class. That's only what PluginManagerBase suggests, but inlining the code directly in getInstance() is fine (and faster), especially if that code is not going to be reusable for other plugin types.
FormatterPluginManager::getInstance() inlining its own code is already present in current 8.x, and @EclipseGc & @neclimdul have seen this without blinking.
Actually, the only classes in core that implement Mapperinterface are plugin managers.

18. "FormatterPluginManager::prepareConfiguration() should be protected and part of the mapper"
Nope, we need the method to be callable separately. That's the method that handles "add default field display options before storing them" in EntityDisplay::setComponent().
The underlying pattern is that we always make sure that stored configuration are complete - as opposed to "store incomplete options, and fill defaults at runtime", because defaults might change later, and you don't want your stored behavior to change beneath your feet.
So we accept incomplete display options, but we fill in defaults right away, and then your config is stable as any other one.
This pattern has been a constant in CCK / Field API, and proved very helpful for peace of mind :-)

Pushed :
- df41cda streamline EntityDisplayTest (@tim.plunkett's #57-14)
- 751af6c Make EntityDisplay::$content protected. This required adding two new methods :
EntityDisplay::createCopy($view_mode), clones a display over to a new view mode (Field UI uses this)
EntityDisplay::getComponents(), returns display options for all components as an array (node's implementation of hook_entity_display_alter() needs this, read from $content previously, which was bad)
- b19249d Expand phpdoc for entity_get_display(), as per #22 and #59.
- 84f6902 Minor - simplify some variable names in Field UI's DisplayOverview.
- f1b91bf Minor - remove mentions of "CMI" or "configuration files"

Interdiff attached, see links above for the individual commitdiffs.

Doh. Stupid logic error in the new EntityDisplay::getComponents().

- 63b071b Fixed that, and added tests for the recent getComponents() & createCopy() methods.

Will need a reroll for #1849480: Allow ConfigEntity classes to control how they are saved. Still hoping for #1860418: Fix user_picture field creation to get in though :-)

Both issues got in, rerolled :-)

Oops, @swentel pointed a 80 chars wrapping error in IRC.

More typos & fixes spotted by swentel :-)

Fix test fails
- EntityDisplayTest fails caused by #68 reassigning the 'module' entry in EntityDisplay annotations to 'entity' instead of 'field' (leftover of before #1857074: Re-introduce entity.module as a configuration owner - but then EntityDisplayTest must make sure entity module is enabled for DrupalUnitTestBase
- CommentPreviewTest / UserPictureTest fails caused by incomplete merge of #1860418: Fix user_picture field creation in #65.

We should be good :-)

Additionally;
- fixes one last 80 chars wrapping error
- rewords the phpdoc for the EntityDisplay class.

#70: entity_display-1852966-70.patch queued for re-testing.

typo

Yeah, noticed that too, and left a note about that in #1802750-5: [Meta] Convert configurable data to ConfigEntity system.
That probably doesn't need to block the patch though, since the "convert X to config entities" tasks that got in so far (image styles, contact categories) didn't create manifest entries either - sometimes no UUID either.

Thanks @catch.

Rerolled after #1862656: Move field type modules out of Field API module. Things should be at the correct place, but please wait for green to commit :-)
+ Fixed the points mentioned in #76, except

- field_get_actual_view_mode() / entity_get_display_with_fallback() :
Agreed, something like that would be better, but since the storage for this is untouched and still in a field-owned variable for now (and moving it to a better place is not in the strict scope of the patch), I'm leaving the corresponding API function unchanged too for now.
But definitely belong in the entity API in the long run.

- drupal_container() in a ConfigEntity : yeah, for now we don't really now how injectability icould be done :-/

- "Would be great to move extra fields out of the field system"
Yup, most definitely needs to happen, but in a patch of its own :-)

- Will fix the $conf stuff in the update tomorrow (in a followup patch if this got in meanwhile)

Side note: please look at the OP for commit credits :-)

Some bug with #1825044: Turn contact form submissions into full-blown Contact Message entities, without storage that got in yesterday, it seems $contact_message->bundle() returns NULL in some cases.
Investigating.

- Fails in ContactPersonalTest: see the comment I left in #1825044-70: Turn contact form submissions into full-blown Contact Message entities, without storage.
In short, contact.module currently produces entities with a NULL bundle in some cases, fixing that was left as a followup : #1856556: Convert user contact form into a contact category/bundle.
So for now, I commented out the part the checks on having a valid bundle property in EntityDisplay, with a @todo to add them back when that issue is fixed.

- Changed field_update_8002() to read from the {variables} table directly instead of relying on $conf.

Back to RTBC as per #76.

OK, polishing a bit after #83. Stay tuned.

re @sun #83:

- field_get_actual_view_mode() :
So it seems this function bugs a lot of people :-). When responding to @catch's similar concerns in #77 I forgot that, while the logic is strictly the same as in D7, the function as a standalone helper is introduced by the patch, so this should probably be addressed here.
After a bit of a back & forth dance between finding a better name, inlining the logic in entity_get_display() with an additional $render_time optional param (was suggested earlier in the thread), I eventually chose to use a separate entity_get_render_display() function for "I need to render an entity, apply whatever logic and give me the relevant display object".
This is clearer in terms of documentation, and should also be a better direction if we wanted to make the logic extendable (like allow per-entity display objects in contrib, aka "hide body in node 12 teaser").
Interdiff: b2bd587

- Hunk in theme_node_preview() / "Does this mean that every [entity]_view() needs to clone the $entity ?"
No, that's specific to what is done in node preview. However, while digging back in there to explain better, I found a better fix.
In short, the $node comes out of preview with values massaged by the 'prepare_view' steps and the entity_view_prepared flag set, and is then put back in $form_state. On next submit, the values get reset to the current form values, but the entity_view_prepared flag stays, which will cause errors if a 2nd preview is done.
The current "fix" in HEAD and D7 is a strange mix of cloning the node for one of the renders in theme_node_preview(), and hardcoding a call to field_attach_prepare_view() in node_preview() - but we can't have the latter anymore, and the patch removes it.
The correct fix is :
- to unset the entity_view_prepared flag in NodeFormController::buildEntity()
- to turn the entity_view_prepared flag from "the entity has been prepared", which means nothing, to "the entity has been prepared for this $view_mode", so that when the entity is rendered in several view modes in a row, the 'prepare_view' steps are correctly performed if needed. (That part is not in the interdiff, it was already in the previous iterations of the patch)
Interdiff: 073c8db

- 'vidible" / 'visible': fixed
The typo actually had no effect, since 'visible' is assumed to be TRUE if not specified.

Updated patch attached - see the links above for the interdiff.
The test bot seems to choke on it with a "Failed to run tests: failed to enable simpletest module." error, but I have no clue why - everything works locally :-/

OK, testbot fail was because calls to entity_get_render_display() did not match the signature.
Should be good to go now.

Pushed minor fixes :

- 0b34a89 Renamed the update helper function from _entity_update_8000_get_display_config() to _update_8000_entity_get_display(), to match current core practice :
_update_7000_field_create_field()
_update_7000_node_get_types()
_update_8000_field_sql_storage_write()

- 7a9245a Fixed incorrect calls to setComponent() in TextFieldTest & NumberFieldTest (wrong refactor at some point earlier on). Those calls were trying to assign the default formatter, which was the end result anyway, so the error had no effect.

@sun: if your concerns are addressed, back to RTBC ? :-)

#88: entity_display-1852966-88.patch queued for re-testing.

According to the test log, this is an occurrence of #1862222: Random deadlock issues in simpletest

Same fail, different place...
Re-uploading so that the broken test log stays around.

Rerolled / updated after #1552396: Convert vocabularies into configuration
Interdiff : 95ed8a0

"Deadlock found when trying to get lock"...
#93: entity_display-1852966-93.patch queued for re-testing.

http://bit.ly/Uc2PLU

Ok, he was not talking about *this* patch, but... pleeease ?

Checking how this behaves after #1824500: In-place editing for Fields
#93: entity_display-1852966-93.patch queued for re-testing.

OK, actually #1824500: In-place editing for Fields added code with $instance['display'] in its tests. Updated patch.
Interdiff: 8fcd3f5

With the patch.

#100: entity_display-1852966-99.patch queued for re-testing.

Back to RTBC.
+ tagging

Now that #1817182: Add upgrade path tests for contact category config conversion is in and provides a helper function for this, create manifest entries for the entity_display config entities created during D7 upgrade path (with corresponding tests).

Also, bump :-)

Yay at the commit !
Although, ouch at @Stalski & @zuuperman forgotten in the commit message (we had left some "commit message" instructions in the OP and a couple posts ago, but that got out of sight in the reroll / RTBC dances...)
They did all the initial work in the sandbox.

@catch Is there something we can do about that ? :-/

Thanks @catch !

Created change notice at http://drupal.org/node/1875952.

Tiny followup patch fixes a small error in the PHPdoc for entity_get_display():
args are $entity_type, $bundle, not the other way around.
Trivial doc fix, setting straight back to RTBC.

I created a couple followup tasks, and a meta issue for tracking them: #1875994: [meta] EntityDisplays.
And a 1st followup patch at #1875970: Pass EntityDisplay objects to the whole entity_view() callstack.

@114: right, leftovers after some code refactoring.
The code sample for hook_entity_display_alter() is copied from node_entity_display_alter(), but was not updated correctly.

Patch fixes those + #111.

resetting title

Sorry, last couple weeks were pretty packed with my non-drupal line of work, slowly getting back to the core queue.

re: making field_create_instance() able to write display settings as a convenience.
I considered that at some point, I'm a bit wary of that.
'display' settings are now out of $instance. $field and $instance are only about the presence and definition of data on an $entity, not how to display it (or edit it, once $instance['widget'] gets out too).
So I don't wan't to reintroduce a fake $instance['display'] entry that field_create_instance($instance) would understand - then field_update_field() would need to as well ? Like, "display settings are not part of $instance, but well, you can pretend they are".
Also, once $instance shifts to an object as it moves to CMI, it would be even uglier.

I guess a second, optional parameter for field_create_instance() might be doable, but :
- do we want that second param to control "display using default formatter+settings on the default view mode" or "save this array of display settings keyed by view mode" ? Not sure how the two could live along.
- again, when $instance moves to a ConfigEntity, field_create_instance($instance) becomes
$instance = entity_create('field_instance');
$instance->foo = 'bar';
$instance->save();
so no second param here.

I'd also tend to think what you describe mostly affects writing tests, CMI deployment and import of default config should take care of the other use cases.

Checking the change notice, I recognized that there's a 'default' view mode baked into the API...
So I tried whether replacing 'account' with 'default' makes any difference, and poof, tests are passing
However, 'account' is the actual view mode being used by the calling/runtime code. Looks like some inheritance got broken here

This hasn't changed from D7. Any view mode gets rendered using the settings configured for the "default" view mode, unless the view mode has been configured to use dedicated settings for the bundle.
This reduces the sea of config ("configure all existing view modes for all bundles") , and makes sure that newly added view modes (say, defined by a module you just enabled) "just work" without forcing the user to go and explicitely configure the content of the view mode for each node type while his live pages display unconfigured junk.

(side note: moved the "render-side functions and hooks" part of the change record into the new record created for #1875970: Pass EntityDisplay objects to the whole entity_view() callstack)

@andypost #126 : seems you found the right way meanwhile.
So yeah, the whole config entity loading API is not available / not to be trusted within hook_update_N(), you have to work on the raw config data, just like you had to work on raw db data in D7 when config was in the db.

I'll add a word about _update_8000_entity_get_display() in the change notification [edit: done - http://drupal.org/node/1875952 ]

FYI - upgrade path fix in #2089273: upgrade path puts D7 deleted fields in EntityDisplay objects

Add commit message