Motivation:

This patch reworks how display settings for the components of a rendered entity (fields, 'extra fields', field_groups...) are stored and how entity render arrays get assembled. Work started at the Ghent sprint early november, after a couple discussions with the layouts folks in october.
Development happens in field-api-displays-1830868 ("D8 Field API" sandbox)

Commit message

Issue #1852966 by yched, Stalski, zuuperman, swentel: Rework entity display settings around EntityDisplay config entity.

It is a required preliminary step for:
- "entity layouts", aka "panelizer in core"
- admin configurability of the "node edit" design introduced in #1510532: [META] Implement the new create content page design - aka layouts on entity edit forms (will need a similar rework to be applied on the 'form' side, in a followup)

But it constitutes a very much valid cleanup in itself, with associated performance gains - See "Performance impact" below.

This comes late in the cycle, but it builds on a couple recent stuff like EntityRenderController as a unified way of rendering entities, or formatters as plugins. It also needed a notion of where page layouts and blocks were going.

Context:

Display settings for the "components of an entity view in a given view mode" are currently scattered in many separate locations:
- in $instance['display'] for each individual (Field API) field in the bundle
- in the 'field_bundle_settings_[entity_type]_[bundle]' variable (yes, eww...) for the 'extra fields' in the bundle
- in other places for contrib additions that are not one of the two above (mostly - field_groups)

This means that:
- we load too much info in memory (like, display settings for the fields / extra fields in all view modes, while the current request
only displays teasers, or is just on the edit side)
- the entity rendering system itself has no official knowledge of what's in a rendered node, making it difficult to provide general features like , well Field UI ;-), or dispatching into regions in a layout.

Patch:

What the patch does:
- introduce an EntityDisplay config entity, holding the display settings for all the components of an entity in a view mode.
- remove $instance['display'] - $field and $instance definitions structures are purely about data, not about how render it.
If this gets in, $instance['widget'] will go away in a similar fashion in a followup patch.
- remove the part of the field_bundle_settings_[entity_type]_[bundle] variable that deals with extra fields on the display side
- reworks the EntityRenderController flow to load the EntityDisplay and pass it along to the code in charge of rendering the various components
- formatter plugin objects are not persisted within the $instance anymore, but within the $entity_display object , since it is the one that receives definition changes. This is as per discussions with @sdboyer in #1817044: Implement Display, a type of config for use by layouts, et. all (#72 / #73 / #82).
- fix node_preview() - the field_attach_prepare_view() call actually serves no purpose since the render refactorings from #1026616: Implement an entity render controller.

Performance impact

Roughly -4% CPU on node/%nid and frontpage - see comment #15 for details.
+ With a followup views patch, it should also allow to considerably speed up the rendering of 'per field' views (i.e those that don't consist of entity_view_multiple($the_result_ids, $some_view_mode) - that includes admin tables...)

API changes

- Display settings are not specified in the $instance definition structure anymore :
Before:

<?php
field_create_instance
(array(
 
'field_name' => 'some_field',
 
'entity_type' => 'node',
 
'bundle' => 'article',
 
'widget' => array(
   
'type' => 'some_widget',
  ),
 
'display' => array(
   
'default' => array(
     
'type' => 'some_formatter',
     
'settings' => array('foo' => 'bar')
     
'weight' => 1,
    ),
   
'teaser' => array(
     
'type' => 'some_other_formatter',
     
'settings' => array('baz' => 'barbaz')
     
'weight' => 1,
    ),
  )
);
?>

After:
<?php
field_create_instance
(array(
 
'field_name' => 'some_field',
 
'entity_type' => 'node',
 
'bundle' => 'article',
 
'widget' => array(
   
'type' => 'some_widget',
  ),
);
entity_get_display('article', 'node', 'default')
  ->
setComponent('some_field', array(
   
'type' => 'some_formatter',
   
'settings' => array('foo' => 'bar')
   
'weight' => 1,
  ))
  ->
save();
entity_get_display('article', 'node', 'teaser')
  ->
setComponent('some_field', array(
   
'type' => 'some_other_formatter',
   
'settings' => array('baz' => 'barbaz')
   
'weight' => 1,
  ))
  ->
save();
?>

- field_attach_prepare_view() and field_attach_view() now receive the $display object and read from it, instead of a view mode.
= "display the fields in this entity according to those display settings", which makes much sense IMO.
This is also what will let 'by field' views perform better, by doing a single field_attach_prepare_view() / field_attach_view() instead of a series of field_view_field() calls currently.

- hook_field_attach_view_alter() : the existing $context['display'] parameter is renamed to $context['display_options'], for accuracy and consistency with how we now call this kind of "array of field display options" everywhere else.

- hook_field_display_alter() / hook_field_extra_field_display_alter() are gone in favor of hook_entity_display_alter()
Before:

<?php
// Called once per field & per entity
function hook_field_display_alter(array &$display_properties, array $context) {
  if (
$context['entity_type'] == 'node' && $context['view_mode'] == 'teaser' && $context['field']['field_name'] == 'my_field') {
   
$display_properties['type'] = 'hidden';
  }
}
?>

After:
<?php
// Called once per entity_view_multiple()
function hook_entity_display_alter(EntityDisplay $entity_display, array $context) {
  if (
$context['entity_type'] == 'node' && $context['view_mode'] == 'teaser') {
   
$entity_display->removeComponent('my_field');
  }
}
?>

Followups:

- Pass $display in EntityRenderController::buildContent(), hook_entity_view(), hook_entity_view_alter(). Would be the logical next step, but not strictly needed as a 1st step, so I'd better leave this out of this patch.
- Abstract Field API specific code out of EntityDisplay.
The EntityDisplay methods should be agnostic about what type of "thing" they hold (field, 'extra field', field_group, possibly blocksNG...).I have a plan (see #28 below), but similarly, I'd rather go there in a followup.

Files: 
CommentFileSizeAuthor
#115 entity_display-docfix-1852966-115.patch1.71 KByched
PASSED: [[SimpleTest]]: [MySQL] 50,023 pass(es).
[ View ]
#114 entity_get_display-docfix-1852966-114.patch1.42 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 49,996 pass(es).
[ View ]
#111 entity_get_display-docfix-1852966-111.patch638 bytesyched
PASSED: [[SimpleTest]]: [MySQL] 49,758 pass(es).
[ View ]
#105 entity_display-1852966-105.patch174.89 KByched
PASSED: [[SimpleTest]]: [MySQL] 49,763 pass(es).
[ View ]
#105 interdiff.txt2.51 KByched
#100 entity_display-1852966-99.patch174.24 KByched
PASSED: [[SimpleTest]]: [MySQL] 49,424 pass(es).
[ View ]
#93 entity_display-1852966-93.patch172.22 KByched
PASSED: [[SimpleTest]]: [MySQL] 49,445 pass(es).
[ View ]
#92 entity_display-1852966-88.patch172.23 KByched
PASSED: [[SimpleTest]]: [MySQL] 49,434 pass(es).
[ View ]
#88 entity_display-1852966-88.patch172.23 KByched
FAILED: [[SimpleTest]]: [MySQL] 49,419 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#87 entity_display-1852966-87.patch172.57 KByched
PASSED: [[SimpleTest]]: [MySQL] 49,445 pass(es).
[ View ]
#87 interdiff.txt4.67 KByched
#85 entity_display-1852966-85.patch171.4 KByched
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#81 entity_display-1852966-81.patch170.84 KByched
PASSED: [[SimpleTest]]: [MySQL] 49,419 pass(es).
[ View ]
#81 interdiff.txt2.24 KByched
#77 entity_display-1852966-77.patch170.43 KByched
FAILED: [[SimpleTest]]: [MySQL] 49,395 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#70 entity_display-no_test_changes-70-do-not-test.patch106.83 KByched
#70 entity_display-1852966-70.patch170.7 KByched
PASSED: [[SimpleTest]]: [MySQL] 49,334 pass(es).
[ View ]
#70 interdiff.txt3.52 KByched
#68 entity_display-no_test_changes-68-do-not-test.patch105.13 KByched
#68 entity_display-1852966-68.patch169.01 KByched
FAILED: [[SimpleTest]]: [MySQL] 49,288 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#68 interdiff.txt4.38 KByched
#66 entity_display-no_test_changes-66-do-not-test.patch105.13 KByched
#66 entity_display-1852966-66.patch169 KByched
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#65 entity_display-no_test_changes-65-do-not-test.patch105.1 KByched
#65 entity_display-1852966-65.patch168.98 KByched
FAILED: [[SimpleTest]]: [MySQL] 49,321 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#62 entity_display-no_test_changes-62-do-not-test.patch112.37 KByched
#62 entity_display-1852966-62.patch177.18 KByched
PASSED: [[SimpleTest]]: [MySQL] 49,229 pass(es).
[ View ]
#60 entity_display-no_test_changes-60-do-not-test.patch111.65 KByched
#60 entity_display-1852966-60.patch176.46 KByched
FAILED: [[SimpleTest]]: [MySQL] 49,419 pass(es), 0 fail(s), and 348 exception(s).
[ View ]
#60 interdiff.txt14.51 KByched
#58 entity_display-no_test_changes-58-do-not-test.patch108.24 KByched
#58 entity_display-1852966-58.patch173.05 KByched
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#58 interdiff.txt4.37 KByched
#56 entity_display-no_test_changes-56-do-not-test.patch108.18 KByched
#56 entity_display-1852966-56.patch172.99 KByched
PASSED: [[SimpleTest]]: [MySQL] 49,325 pass(es).
[ View ]
#52 entity_display-no_test_changes-52-do-not-test.patch116.59 KByched
#52 entity_display-1852966-52.patch173.38 KByched
PASSED: [[SimpleTest]]: [MySQL] 48,938 pass(es).
[ View ]
#49 entity_display-no_test_changes-49-do-not-test.patch108.33 KByched
#49 entity_display-1852966-49.patch165 KByched
FAILED: [[SimpleTest]]: [MySQL] 48,396 pass(es), 17 fail(s), and 40 exception(s).
[ View ]
#45 entity_display-1852966-45.patch161.9 KByched
PASSED: [[SimpleTest]]: [MySQL] 48,963 pass(es).
[ View ]
#45 entity_display-no_test_changes-45-do-not-test.patch105.22 KByched
#20 entity_display-no_test_changes-20-do-not-test.patch88.56 KByched
#20 entity_display-1852966-20.patch154.96 KByched
PASSED: [[SimpleTest]]: [MySQL] 49,289 pass(es).
[ View ]
#18 entity_display-1852966-18.patch151.52 KByched
PASSED: [[SimpleTest]]: [MySQL] 48,997 pass(es).
[ View ]
#18 entity_display-no_test_changes-18-do-not-test.patch85.11 KByched
#16 entity_display-no_test_changes-do-not-test.patch79 KByched
#16 entity_display-1852966-16.patch145.24 KByched
FAILED: [[SimpleTest]]: [MySQL] 49,000 pass(es), 8 fail(s), and 1 exception(s).
[ View ]
#11 entity_display-1852966-11.patch145.69 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 48,949 pass(es), 7 fail(s), and 1 exception(s).
[ View ]
#11 interdiff.txt4.74 KBswentel
#10 entity_display-1852966-10.patch141.84 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#10 interdiff.txt721 bytesswentel
#9 entity_display-1852966-9.patch141.13 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 48,497 pass(es), 25 fail(s), and 52 exception(s).
[ View ]
#7 hack-static-cache-interdiff.txt861 bytesyched
#1 entity_display-1852966-1.patch141.1 KByched
FAILED: [[SimpleTest]]: [MySQL] 48,360 pass(es), 26 fail(s), and 52 exception(s).
[ View ]
#1 entity_display-no_test_changes-do-not-test.patch74.9 KByched

Comments

Status:Active» Needs review
StatusFileSize
new74.9 KB
new141.1 KB
FAILED: [[SimpleTest]]: [MySQL] 48,360 pass(es), 26 fail(s), and 52 exception(s).
[ View ]

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

+1000 from me on this idea. The array of display settings is just going to get more and more out of hand as something that is plastered into the $instance object.

Generally this looks great to me. I wonder about the need for the field_get_actual_view_mode() function. Why not just have entity_get_display() handle the fallback to 'default' if you ask for a view mode that doesn't exist in the display? I'm not sure I see the need to jump through the extra hoop, but it's possible I'm missing something.

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-1.patch, failed testing.

Status:Needs work» Needs review

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()]

StatusFileSize
new861 bytes

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

This is really awesome news! thx yched

StatusFileSize
new141.13 KB
FAILED: [[SimpleTest]]: [MySQL] 48,497 pass(es), 25 fail(s), and 52 exception(s).
[ View ]

Chasing head so I can work on the upgrade path.

StatusFileSize
new721 bytes
new141.84 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Another patch to actually let update.php work by registering widget and formatter plugin managers in the DIC.

StatusFileSize
new4.74 KB
new145.69 KB
FAILED: [[SimpleTest]]: [MySQL] 48,949 pass(es), 7 fail(s), and 1 exception(s).
[ View ]

And upgrade path + tests.

+++ b/core/update.phpundefined
@@ -477,6 +477,8 @@ function update_check_requirements($skip_warnings = FALSE) {
   ->addArgument(new Reference('dispatcher'));
+$container->register('plugin.manager.field.widget', 'Drupal\field\Plugin\Type\Widget\WidgetPluginManager');
+$container->register('plugin.manager.field.formatter', 'Drupal\field\Plugin\Type\Formatter\FormatterPluginManager');

This should no longer be necessary with #1849004: One Service Container To Rule Them All.

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-11.patch, failed testing.

@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 ;-) )

Issue summary:View changes

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%

Status:Needs work» Needs review
StatusFileSize
new145.24 KB
FAILED: [[SimpleTest]]: [MySQL] 49,000 pass(es), 8 fail(s), and 1 exception(s).
[ View ]
new79 KB

Forgot to attach the patch in #14...

Issue summary:View changes

added more specific perf numbers

Issue summary:View changes

add section for performance

Issue summary:View changes

headers

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new85.11 KB
new151.52 KB
PASSED: [[SimpleTest]]: [MySQL] 48,997 pass(es).
[ View ]

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.

Issue summary:View changes

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.

StatusFileSize
new154.96 KB
PASSED: [[SimpleTest]]: [MySQL] 49,289 pass(es).
[ View ]
new88.56 KB

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.

Overall this looks great at the moment.
Here are some small issues I saw when reading through the patch.
Keep up the good work!

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,287 @@
+   * Overrides Drupal\Core\Entity\Entity::id().

There is a mixture between \DRupal\Core... and Drupal\Core. Should be the first one everywhere

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,287 @@
+   * Gets the display settings of a field.
+   *
+   * @param string $field_name
+   *   The field name to get settings for.
+   * @param bool $add_default
+   *   Add default settings or not.
+   *
+   * @return array
+   *   The settings for the field.
+   */
+  public function getContent($field_name) {
+    // We always store 'extra fields', whether they are visible or hidden.

Naming is a bit off here. Why not getDisplaySettings()? Thats just what the method does.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,287 @@
+  public function setContent($field_name, array $properties = array()) {

Same, setDisplaySettings() sounds better. The docs say settings, the function param is properties.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,287 @@
+  public function deleteContent($field_name) {

Same ;)

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.phpundefined
@@ -42,31 +43,87 @@ public function __construct() {
+   * ¶

Trailing whitespace

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
@@ -175,8 +175,8 @@ function testInstancePrepare() {
+    //$data['display']['default']['settings'] = array();

Lots oof tests are commented. I didn't read every comment so I'm not sure it's intentional.

+++ b/core/modules/node/node.tokens.incundefined
@@ -149,7 +149,19 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
+                ¶

trailing whitespace/tabs

I kind of agree that we might need some consistency in naming. We store the values in 'content' using 'getContent' and 'setContent', but we also call them 'settings' and 'properties' in various places. It doesn't feel like 'content', although technically I guess it is the content of the display entity. It's hard not to confuse that with the content of the field that is being displayed. For instance, I might imagine that getContent() would return the formatted output for the field.

It might be nice to add some documentation about how to use this. Not sure where to put it. Something like;

Example usage to set a display settings for a specific field and view mode:
@code
entity_get_display('article', 'node', 'default')
  ->setContent('body', array(
    'type' => 'text_summary_or_trimmed',
    'settings' => array('trim_length' => '200')
    'weight' => 1,
  ))
  ->save();
@endcode
Example usage to retrieve display settings. Note that the content for any view mode other than 'default' might be empty if it has not been set. Use field_get_actual_view_mode() to ensure a valid display, it will fall back to 'default' if the requested view mode has not been set;
@code
  $view_mode = field_get_actual_view_mode('node', 'article', 'full');
  $display = entity_get_display('node', 'article', $view_mode);
  $formatter_settings = $display->getContent('body');
@endcode

Other than that, it seems to work fine and it makes sense architecturally.

usage of Content here is really confusing a lot. imo Display is a kind of view with settings and properties to display

This looks like a good cleanup, I'm not sure about the overall plans for it though. So here some questions:

  • this seems to depend on field api. What about non-fieldable entities? Is support for them planned also? Also, I guess we should check the fieldable flag in the render controller.
  • related: is it possible or planned to support formatting non-fields also? I see it has extra fields, but I'd assume to be able to use this instead of having to declare an extra field?

@display-content:
I kind of like the term "display component", but I'd see that as any "entity display component." (not necessarily a field). Or "display field" maybe? (yet another field..)

I think all your remarks are valid and would be taken up in follow-up issues.
- AFAIK the support for non-fieldable entities is not yet planned (as in, there is no issue yet)
- formatting extra fields is something we all would like I persume. I always thought that was something that would be introduce by the new field API, no? That said, if a decision has been made about that, I would be very happy to implement the new dynamic field thingies.

My opinion on the whole "extra field" / "dynamic field" feature is that the viewing of a field and extra field should be the same thing. In other words, if fields come from storage and extra fields come from a definition somewhere (but NOT the preprocessors) that would be no responsibility of the display rendering.

As you I also like "component" as a name.

- formatting extra fields is something we all would like I persume. I always thought that was something that would be introduce by the new field API, no? That said, if a decision has been made about that, I would be very happy to implement the new dynamic field thingies.

Having formatter support the new entity fields would be superb. I think we'd get it automatically if we manage to unify field api fields and entity fields.

My opinion on the whole "extra field" / "dynamic field" feature is that the viewing of a field and extra field should be the same thing. In other words, if fields come from storage and extra fields come from a definition somewhere (but NOT the preprocessors) that would be no responsibility of the display rendering.

I'd love to see that - so we'd have display components that are not necessarily bound to fields at all?

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.

Issue summary:View changes

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.

Issue summary:View changes

Updated TODO : tests for EntityDisplay

#28 sounds like a good plan - thanks for writing down the thoughts.

>In short: that's another discussion, and the patch doesn't make things worse towards that goal.
I think it's a step in the right direction. Doing it iteratively is certainly a good approach.

@yched: good write-up and makes it very clear. Totally in favour to proceed in those defined steps.
Great thing about this is that everyone wants the same thing and I actually see it happening.

In addition to the suggestion of the "component" approach. I think of it in the same way Yves does (with concrete handlers for each type). In fact since we are talking fields, extra fields, fieldgroups, blocks, ... we are talking about everyhing renderable in drupal, so this asks a bit for an interface for all renderable stuff, although that might be a bridge too far.

An interface is not a bridge too far. It's a must :). When you want custom content to be shown in the display object. All the required component methods should be declared. (example: default display properties).

yeah :)

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 :

<?php
$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:

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

I kind of like it, it seems odd when you read it and then it hits ya. +1
+1 for the options name as well

(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:

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

+1 from me for add / remove

add is not always true, but show isn't either. We actually set the if and how a component will be visible (or not). Adding is a bit weird if you only want to change component options, so setComponent might be better.
Remove seems correct.

#38 : Agreed.

<?php
$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 ?

<?php
$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 :

<?php
$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)

<?php
$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)

<?php
$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 ?

Issue summary:View changes

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

The get/set terminology makes the most sense to me, and it is consistent with terminology used all over the place. I'm not sure what 'Show' means exactly, but I understand 'Set'.

Sounds good to me as well.

The separate handler for actually managing the display of the components maps nicely to the concept of entities and entity render controllers/handlers, so that makes sense to me as well.

And I like 'remove' better than 'hide'. 'Hide' implies it is still there but you can't see it, but it's actually removed.

Issue summary:View changes

DONE: added tests

Issue summary:View changes

Updated for method renames.

StatusFileSize
new105.22 KB
new161.9 KB
PASSED: [[SimpleTest]]: [MySQL] 48,963 pass(es).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-45.patch, failed testing.

Status:Needs work» Needs review

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

Issue summary:View changes

TODO -> DONE

StatusFileSize
new165 KB
FAILED: [[SimpleTest]]: [MySQL] 48,396 pass(es), 17 fail(s), and 40 exception(s).
[ View ]
new108.33 KB

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 :-)

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-49.patch, failed testing.

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 :-)

Status:Needs work» Needs review
StatusFileSize
new173.38 KB
PASSED: [[SimpleTest]]: [MySQL] 48,938 pass(es).
[ View ]
new116.59 KB

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.

Issue summary:View changes

Added API change

Issue summary:View changes

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

Issue summary:View changes

typo

Issue summary:View changes

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 :-)

Some code style remarks, but I'll leave it to NR though. I could mark it RTBC after a re-roll, as I've only worked on an upgrade path. That would get more eyes on this patch maybe :)

+++ b/core/includes/entity.api.phpundefined
@@ -279,6 +279,28 @@ function hook_entity_view_mode_alter(&$view_mode, Drupal\Core\Entity\EntityInter
+ *   The entity_display object that will be used to display the entity components.

Shouldn't exceed 80 chars

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayStorageController.phpundefined
@@ -0,0 +1,37 @@
+ * Defines the storage controller class for EntityDisplay  entities.

A space too much between EntityDisplay and entities.

+++ 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 ?

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.phpundefined
@@ -0,0 +1,187 @@
+    // being being assigned.

Two times 'being'

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
@@ -168,15 +168,18 @@ function testInstancePrepare() {
+<<<<<<< HEAD

Merge conflicts in the patch ? Or am I going crazy here ?

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
@@ -168,15 +168,18 @@ function testInstancePrepare() {
+=======

same here

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInstanceCrudTest.phpundefined
@@ -72,13 +71,19 @@ function testCreateFieldInstance() {
+>>>>>>> field-api-displays-1830868

Also in this file

+++ b/core/modules/node/node.tokens.incundefined
@@ -149,7 +149,19 @@ function node_tokens($type, $tokens, array $data = array(), array $options = arr
+                // Get the 'trim_length' size used for the 'teaser' mode, if present, or

exceeds 80 chars

+++ b/core/profiles/standard/standard.installundefined
@@ -440,3 +438,85 @@ function standard_install() {
+ * \Drupal\user\Tests\UserPictureTest can test the user_picture functionnality without

80 chars

StatusFileSize
new172.99 KB
PASSED: [[SimpleTest]]: [MySQL] 49,325 pass(es).
[ View ]
new108.18 KB

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)

Status:Needs review» Needs work

The overriding of ConfigStorageController::getProperties() is exactly the same as the Views one, which is why I added that method. That part looks great.

1.

+++ b/core/includes/entity.incundefined
@@ -563,6 +563,59 @@ function entity_view_multiple(array $entities, $view_mode, $langcode = NULL) {
+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?

2.

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -30,37 +30,69 @@ public function buildContent(array $entities = array(), $view_mode = 'full', $la
-    foreach ($entities as $key => $entity) {
+    $view_modes = array();
...
+    foreach ($entities as $entity) {
...
+      $view_modes[$view_mode][$entity->id()] = $entity;

Isn't this already keyed by entity ID?

3.

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -30,37 +30,69 @@ public function buildContent(array $entities = array(), $view_mode = 'full', $la
       drupal_alter('entity_view_mode', $view_mode, $entity, $context);
...
+        drupal_alter('entity_display', $display, $display_context);

I don't yet understand why we'd need both here.

4.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,286 @@
+ *     "id" = "id"

Missing uuid, and maybe label?

5.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,286 @@
+   * Unique id for the config entity.

Here and elsewhere, id should be ID in comments.

6.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,286 @@
+  public $targetEntityType;
...
+  public $bundle;
...
+  public $viewMode;
...
+  public $content = array();
...
+  public $originalViewMode;
...
+  protected $formatters = array();

Can these be made protected?

7.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,286 @@
+   * Contains all field display configuration

Missing period

8.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,286 @@
+    return $this->targetEntityType . "." . $this->bundle . "." . $this->viewMode;

Weird double quotes

9.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,286 @@
+   *   The name of the component..

Double period

10.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,286 @@
+      $max = $this->getHighestWeight();
+      $options['weight'] = isset($max) ? $max + 1 : 0;
...
+  public function getHighestWeight() {
...
+    return $weights ? max($weights) : NULL;

I understand that NULL means there isn't one yet, but I think returning -1 would make more sense.

11.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,286 @@
+    if ($instance = field_info_instance($this->targetEntityType, $name, $this->bundle)) {
+      $field = field_info_field($instance['field_name']);
...
+    $extra_fields = field_info_extra_fields($this->targetEntityType, $this->bundle, 'display');

All three of these are called? Should they be statically cached as an object property?

12.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,286 @@
+     $this->content[$name] = array(
+       'visible' => FALSE,
+     );
+    }

Wrong indentation

13.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.phpundefined
@@ -0,0 +1,187 @@
+ * Definition of Drupal\entity\Tests\EntityDisplayTest.

Contains \Drupal...

14.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.phpundefined
@@ -0,0 +1,187 @@
+  public static $modules = array('system', 'entity_test');
...
+    $this->enableModules(array('system', 'field'));
...
+    $this->enableModules(array('system', 'field'));
...
+    $this->enableModules(array('system', 'field'));
...
+    $this->enableModules(array('system', 'field', 'field_sql_storage', 'field_test'));

These should be combined into a setUp(), and 'system' could be removed from the static $modules

15.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.phpundefined
@@ -0,0 +1,187 @@
+  function testEntityDisplayCRUD() {
...
+  function testExtraFieldComponent() {
...
+  function testFieldComponent() {

missing visibility

16.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.phpundefined
@@ -0,0 +1,187 @@
+    $this->assertEqual($formatter->getPluginId(), $default_formatter);
+  }
+}
diff --git a/core/modules/field/field.api.php b/core/modules/field/field.api.php

Missing blank line before end of class

17.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.phpundefined
@@ -42,31 +43,87 @@ public function __construct() {
   public function getInstance(array $options) {
+    $configuration = $options['configuration'];
     $instance = $options['instance'];
-    $type = $options['type'];
-
-    $definition = $this->getDefinition($type);
     $field = field_info_field($instance['field_name']);
+    // Fill in default configuration if needed.
+    if (!isset($options['prepare']) || $options['prepare'] == TRUE) {
+      $configuration = $this->prepareConfiguration($field['type'], $configuration);

This should be moved into a custom Mapper.

18.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.phpundefined
@@ -42,31 +43,87 @@ public function __construct() {
+  public function prepareConfiguration($field_type, array $configuration) {

This should be protected, and should be part of the mapper.

19.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/FieldUpgradePathTest.phpundefined
@@ -0,0 +1,82 @@
+class FieldUpgradePathTest extends UpgradePathTestBase {
+  public static function getInfo() {

Missing blank line

20.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/FieldUpgradePathTest.phpundefined
@@ -0,0 +1,82 @@
+    $this->assertEqual($displays['teaser']['content']['language'], $expected['teaser']);
+  }
+}
+
diff --git a/core/modules/system/lib/Drupal/system/Tests/Upgrade/UserPictureUpgradePathTest.php b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UserPictureUpgradePathTest.php

The blank line should be before the end of the class not after :)

StatusFileSize
new4.37 KB
new173.05 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new108.24 KB

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 :-)

Great feedback!

1) Thanks for the explanation, summing that up in docs++

6) I'm not worried about them being readable, but more writable. If you make them protected you could still do $entity->get('whatever'), that's what Views does.

11) Good point! :)

17) We'll have a generic ConfigMapper soon from the blocks issue. I'm not so sure about the inlining, but if it's just me I'll let it go.

18) I missed the external calls. Carry on!

Status:Needs work» Needs review
StatusFileSize
new14.51 KB
new176.46 KB
FAILED: [[SimpleTest]]: [MySQL] 49,419 pass(es), 0 fail(s), and 348 exception(s).
[ View ]
new111.65 KB

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.

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-60.patch, failed testing.

StatusFileSize
new177.18 KB
PASSED: [[SimpleTest]]: [MySQL] 49,229 pass(es).
[ View ]
new112.37 KB

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

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

Status:Needs work» Needs review

StatusFileSize
new168.98 KB
FAILED: [[SimpleTest]]: [MySQL] 49,321 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new105.1 KB

Both issues got in, rerolled :-)

StatusFileSize
new169 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
new105.13 KB

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

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-66.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.38 KB
new169.01 KB
FAILED: [[SimpleTest]]: [MySQL] 49,288 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
new105.13 KB

More typos & fixes spotted by swentel :-)

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-68.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.52 KB
new170.7 KB
PASSED: [[SimpleTest]]: [MySQL] 49,334 pass(es).
[ View ]
new106.83 KB

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.

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-70.patch, failed testing.

Status:Needs work» Needs review

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

Issue summary:View changes

typo

Status:Needs review» Reviewed & tested by the community

I've looked at this a couple of times now and this looks good imo. It also makes core faster, so everyone should like this :)
It's also holding up new tasks for us, so marking RTBC to get core committers eyes on this one.

Also added suggested commit message at the top.

Issue summary:View changes

Remove dependent issues

So, this probably not RTBC actually, the upgrade path should also create the manifest file ..
We should probably wait to commit this before #1817182: Add upgrade path tests for contact category config conversion gets in which adds a generalised function to create manifest files at the end of hook_update_n - or create a follow up - it's an easy patch.

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.

Status:Reviewed & tested by the community» Needs work

I mainly have minor comments except the upgrade path which while it works and is tested now, is going to break soon (see below).

Since this is blocking quite a bit of work, I would have gone ahead and committed it except it no longer applies.

+++ b/core/includes/entity.incundefined
@@ -564,6 +564,103 @@ function entity_view_multiple(array $entities, $view_mode, $langcode = NULL) {
+ * an entity_display object is explicitely configured and saved. This function

'explicitly'.

+++ b/core/includes/entity.incundefined
@@ -564,6 +564,103 @@ function entity_view_multiple(array $entities, $view_mode, $langcode = NULL) {
+ * @code
+ * $view_mode = field_get_actual_view_mode('node', 'article', 'full');

Why not entity_get_actual_view_mode()? Also it isn't really getting the 'actual' view mode, it's getting the view mode with a fallback to default. Something like entity_get_display_with_fallback() might be more accurate?

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -30,37 +30,69 @@ public function buildContent(array $entities = array(), $view_mode = 'full', $la
-          // Mark this item as prepared.
-          $entity->entity_view_prepared = TRUE;
+          // Mark this item as prepared for this view mode.
+          $entity->entity_view_prepared = $view_mode;

I think EntityNG gets rid of this logic. But it's also not converted across core yet, so just ignore this comment then I guess...

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,341 @@
+    if (!isset($values['targetEntityType']) || !isset($values['bundle']) || !isset($values['viewMode'])) {
+      throw new \InvalidArgumentException(t('Missing required properties for an EntiyDisplay entity.'));

No t() necessary here.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,341 @@
+    if ($instance = field_info_instance($this->targetEntityType, $name, $this->bundle)) {
+      $field = field_info_field($instance['field_name']);
+      $options = drupal_container()->get('plugin.manager.field.formatter')->prepareConfiguration($field['type'], $options);

Was going to ask about injecting these, but that's not doable with the entity classes as they currently are. And arbitrary injectable stuff isn't possible unless things are separately registered on the container (which I don't think makes sense, entity types aren't a 'service'), so ignoring this for now and there's already the plugin issue open to deal with it.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,341 @@
+    // We always store 'extra fields', whether they are visible or hidden.
+    $extra_fields = field_info_extra_fields($this->targetEntityType, $this->bundle, 'display');

Would be great to move extra fields out of the field system.

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
@@ -0,0 +1,341 @@
+    // Instanciate the formatter object from the stored display properties.

Instantiate.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.phpundefined
@@ -0,0 +1,201 @@
+ * Tests the EntityDisplay configuration entities.
+ */
+class EntityDisplayTest extends DrupalUnitTestBase {
+
+  public static $modules = array('entity_test');

Handy that DrupalUnitTestBase :) Test coverage in general looks really good.

+++ b/core/modules/field/field.installundefined
@@ -403,6 +394,96 @@ function field_update_8001() {
+  // Migration of 'extra_fields' display settings. Avoid calling
+  // entity_get_info() by iterating directly on the global $conf array.
+  foreach ($conf as $variable_name => $variable_value) {
+    if (preg_match('/field_bundle_settings_(.*)__(.*)/', $variable_name, $matches)) {

It's very likely that $conf is not going to be prepared during the Drupal 8 upgrade path (although I just checked and it still is now).

Really this should move to direct queries against the variable table. If this wasn't a 120kb patch I'd knock it back to needs work for this, but since there's upgrade path tests that prove it works currently, we can knock that back to a follow-up I think.

Status:Needs work» Needs review
StatusFileSize
new170.43 KB
FAILED: [[SimpleTest]]: [MySQL] 49,395 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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 :-)

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-77.patch, failed testing.

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.

Status:Needs work» Needs review
StatusFileSize
new2.24 KB
new170.84 KB
PASSED: [[SimpleTest]]: [MySQL] 49,419 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Back to RTBC as per #76.

+++ b/core/modules/field/field.module
@@ -725,7 +724,33 @@ function field_view_mode_settings($entity_type, $bundle) {
+ * Returns the actual view mode to use for a bundle.
...
+function field_get_actual_view_mode($entity_type, $bundle, $view_mode) {

Is there an "un-actual" (surreal?) view mode? ;)

+++ b/core/modules/node/node.pages.inc
@@ -180,7 +178,7 @@ function theme_node_preview($variables) {
   $elements = node_view(clone $node, 'teaser');
   $elements['#attached']['library'][] = array('node', 'drupal.node.preview');
   $trimmed = drupal_render($elements);
-  $elements = node_view($node, 'full');
+  $elements = node_view(clone $node, 'full');
   $full = drupal_render($elements);

This looks like a potentially larger consequence.

Does this mean that every [entity]_view() needs to clone the $entity?

+++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -19,6 +19,32 @@ function entity_test_entity_info_alter(&$info) {
+        'vidible' => TRUE,
...
+        'visible' => FALSE,

Typo in first 'vidible'

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new171.4 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

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 :-/

The last submitted patch, entity_display-1852966-85.patch, failed testing.

StatusFileSize
new4.67 KB
new172.57 KB
PASSED: [[SimpleTest]]: [MySQL] 49,445 pass(es).
[ View ]

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

StatusFileSize
new172.23 KB
FAILED: [[SimpleTest]]: [MySQL] 49,419 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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 ? :-)

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-88.patch, failed testing.

Status:Needs work» Needs review

#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

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-88.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new172.23 KB
PASSED: [[SimpleTest]]: [MySQL] 49,434 pass(es).
[ View ]

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

StatusFileSize
new172.22 KB
PASSED: [[SimpleTest]]: [MySQL] 49,445 pass(es).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-93.patch, failed testing.

Status:Needs work» Needs review

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

Status:Needs review» Reviewed & tested by the community

Let's do this!

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

Status:Reviewed & tested by the community» Needs review

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

StatusFileSize
new174.24 KB
PASSED: [[SimpleTest]]: [MySQL] 49,424 pass(es).
[ View ]

With the patch.

Status:Needs review» Needs work

The last submitted patch, entity_display-1852966-99.patch, failed testing.

Status:Needs work» Needs review

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

Back to RTBC.
+ tagging

Status:Needs review» Reviewed & tested by the community

StatusFileSize
new2.51 KB
new174.89 KB
PASSED: [[SimpleTest]]: [MySQL] 49,763 pass(es).
[ View ]

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 :-)

Title:Rework entity display settings around EntityDisplay config entityChange notice: Rework entity display settings around EntityDisplay config entity
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

Getting this in while it's hot. Committed/pushed to 8.x, thanks!

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 ? :-/

@yched @catch Suppose git commit --amend -m "New commit message" should help

#108, that only works if you haven't pushed yet, since d.o disallows non-ff commits.

One option is to revert and then recommit, which then gives yched and swentel 3 commits each for the issue :)

Yeah I did that. Better too much commit credit than too little.

Title:Change notice: Rework entity display settings around EntityDisplay config entityRework entity display settings around EntityDisplay config entity
Status:Active» Reviewed & tested by the community
StatusFileSize
new638 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,758 pass(es).
[ View ]

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.

Priority:Critical» Normal

The change notice looks quite good to me: once the follow-up is committed we can call this fixed.

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.

Status:Reviewed & tested by the community» Needs work
Issue tags:-Avoid commit conflicts
StatusFileSize
new1.42 KB
PASSED: [[SimpleTest]]: [MySQL] 49,996 pass(es).
[ View ]

I found two other small PHPdoc problems:

1) hook_entity_display_alter() is referring to an unexistent class Drupal\field\Plugin\Core\Entity\EntityDisplay
2) inside the same hook example, there is a foreach loop on $display->content but $content is protected.

Patch attached only fixes 1, so leaving at NW.

Title:Rework entity display settings around EntityDisplay config entityDoc fixes followup: Rework entity display settings around EntityDisplay config entity
Status:Needs work» Needs review
StatusFileSize
new1.71 KB
PASSED: [[SimpleTest]]: [MySQL] 50,023 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Looks good to me.

This broke inline editing: #1876992: Toolbar doesn't show the "Edit" tab: fix by using new entity display settings, add tests.

We should check the edit module in future patches. (or improve the tests if possible)

Status:Reviewed & tested by the community» Fixed

Committed/pushed, thanks!

Title:Doc fixes followup: Rework entity display settings around EntityDisplay config entityRework entity display settings around EntityDisplay config entity

resetting title

+++ b/core/modules/link/lib/Drupal/link/Tests/LinkFieldTest.php
@@ -62,13 +62,14 @@ function testURLValidation() {
-      'display' => array(
-        'full' => array(
-          'type' => 'link',
-        ),
-      ),
     );
     field_create_instance($this->instance);
+    entity_get_display('test_entity', 'test_bundle', 'full')
+      ->setComponent($this->field['field_name'], array(
+        'type' => 'link',
+      ))
+      ->save();

Mmm... this really complicated the API and also tests.

Was there any particular reason for not leaving the existing 'display' key for field_create_instance() to trigger the appropriate entity_get_display() call as a convenience measure/handling?

And/or actually... much more important:

Previously, you were able to leave out the 'display' definition entirely, and Field API simply defaulted all fields to their default display settings.

With current HEAD, leaving out any 'display' in field_create_instance() literally means that no fields are displayed at all. No defaults, nothing, nada.

That's really not what I expect as a developer. I expect reasonable/sane defaults.

We also seem to have lost the facility to configure default field settings for all non-customized displays?

I'm struggling hard with this change to get the tests for #1726822: Port Profile2 to D8 and provide upgrade path from D7 core profile back to work. This is what I currently have:

<?php
    $this
->field = array(
     
'field_name' => 'profile_fullname',
     
'type' => 'text',
     
'cardinality' => 1,
     
'translatable' => FALSE,
    );
   
$this->field = field_create_field($this->field);
   
$this->instance = array(
     
'entity_type' => 'profile2',
     
'field_name' => $this->field['field_name'],
     
'bundle' => $this->type->id(),
     
'label' => 'Full name',
     
'widget' => array(
       
'type' => 'text_textfield',
      ),
     
// Note: there was no 'display' key required before; it simply used defaults.
   
);
   
$this->instance = field_create_instance($this->instance);
   
entity_get_display('profile2', 'test', 'account')
      ->
setComponent($this->field['field_name'], array(
       
'type' => 'text_default',
      ))
      ->
save();
?>

However, the field doesn't appear.

Status:Fixed» Active

Eh? 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?

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)

Another topic that needs docs - usage of displays in hook_update_N()

Probably it depends on field_update_8002()

So is there *proposed* way to write a updates for fields?
Currently this works:

<?php
    $instance
= array(
     
'bundle' => $node_type,
     
'default_value' => array(array(array('comment' => $default_value))),
     
'deleted' => '0',
     
'display' => array(
       
'default' => array(
         
'label' => 'above',
         
'module' => 'comment',
         
'settings' => $formatter_settings,
         
'type' => 'comment_default',
         
'weight' => '1',
        ),
       
'rss' => array(
         
'label' => 'hidden',
         
'type' => 'hidden',
        ),
       
'teaser' => array(
         
'label' => 'hidden',
         
'type' => 'comment_links',
         
'settings' => $formatter_settings_links,
        ),
...
_update_7000_field_create_instance($field, $instance);
?>

Current implementation https://gist.github.com/andypost/4945004 leads to randomly broken tests http://qa.drupal.org/pifr/test/442548

Failed: InvalidArgumentException: The entity (entity_display) did not specify a controller_class. in Drupal\Core\Entity\EntityManager->getControllerClass() (line 226 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Entity/EntityManager.php).

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

Status:Active» Fixed

@yched thanx a lot. Change notice describes enough

Status:Fixed» Closed (fixed)

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

How does this effect #1256368: Add 'visible' key to hook_field_extra_fields() and if the 'visibility' flag is not supported, then the committed PHPDoc in the field.api.php for hook_field_extra_fields() are wrong.

There is confusion in that issue about the changes here.

Issue summary:View changes

Add commit message