Convert view modes to ConfigEntity

Feature requests are for D8. When / if done, this could arguably be a candidate for a D7 backport, though.

Meanwhile, http://drupal.org/project/ds lets you create custom view modes - amongst other things the module does. I kind of think this would be cool as a separate feature in astandalone project, though.

I'll be working on this asap.

Note : This looks like "entity_view_mode in core".

The value of shipping core with this feature might be debated, I guess, since core itself will never let you actually display those custom view modes. Completely different story if Views gets in core, though.

Totally agree on that.
It looks like views is going to make it, so imo the patch can be prepared.

However I must say another advantage is that you don't have to download some contribs just to have the benefits of displaying things in view modes, which core already introduces. Also this functionality won't add much load to the requests. They are separate pages and don't affect the rest too much.

the benefits of displaying things in view modes, which core already introduces

Yeah, but I can't really think of another piece of UI in core that has no visible effect on anything unless you enable some contrib module or write some custom code. Without Views in core, a UI for custom view modes would be just that.

Then again, porting entity_view_mode to D8 will need to happen no matter what, and should ba a fairly straightfoward port. So even if this gets turned down (for instance because views didn't make it), it's still worth having.

I guess you might want to ping Dave Reid to get his feelings on this.

We might just let xjm or merlinofchaos look at this as well, then we know for sure whether to proceed or not.

Well, VDC is now an official initiative - see http://buytaert.net/views-in-drupal-8 and the goal is to get this in by 1 december.

Changing component, view modes should really be an entity thing that the field UI tacks onto.

I think the best way to implement view mode management in the same place where they are enabled to be changed.
Also better to have view-mode on per-bundle basis

view modes per bundle won't work in the current system, but would a great asset indeed, but I wouldn't focus on that.

As to determine where the ui belongs: my initial reaction was field ui as well, on the other hand, what if someone in contrib writes a new ui for configuring fields/display of entities, you would still have field ui enabled to get new view modes, which makes it rendundant in that way.

So where would the code be in the end, system module ?
Or a new separate one - say entity_view_mode(s) ? But what happens if you disable that one, do you lose the view modes or not ? I personally vote no.

So the way I see it:
- add a new module called entity_view_modes which is simply the UI
- somewhere in entity_get_info() (or wherever it lives now) - grab the view modes from config() (yeah!)
- the hard coded default view modes that exist now should be converted to config - so core/node/config has a config file called entity.view_modes (or something a like) . This might be a bit tricky though as some view modes only make sense when a module is enabled, e.g. search_result and search_index.

Thoughts, opinions, things I've missed ?

- edit - added more info to last point

Better title ? Feel free to correct.

So I am following this :D But why?

re @andypost #10:
View modes cannot be per bundle. View modes are what a module call when they do [entity_type]_view($entity, $view_mode) or [entity_type]_view_multiple($entities, $view_mode). You don't necessarily know - and at any rate don't need to care - what bundles $entity or $entities belong to, you only know they are of a given entity type.
For instance, when displaying entities with Views, you can pick a view mode. You do this without knowing which bundles the selected entities will be.

re @swentel #11:
"the hard coded default view modes that exist now should be converted to config"
I don't think we can do that. If a module exposes a view mode, it will most likely use it in some of its code somewhere.
View modes are exposed in an info hook about the entity type. entity_view_modes.module's implementation of the hook just reads a list of custom view modes from its own config. The UI only lets you administrate those view modes exposed by entity_view_modes.module. As an admin, I cannot remove a view mode provided by, say, print.module.

Here's a first version. I took a simple approach getting the best of DS and Entity view mode. Has basic tests, but needs a api.php file.
I'm AFK until monday, so anyone else can take this forward in the meantime, let's do this so Views or either the Entity reference field can profit from it (see #1801304: Add Entity reference field)

swentel, thanks for this.
imo this is a perfect candidate for #1781372: Add an API for listing (configuration) entities and #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) with the edit form being replaced with a config entity form controller.
Which means a decent rewrite of your patch, sorry :(
Thoughts?

Had a quick talk with timplunkett on IRC about that. He came with the same suggestion and we kind of agreed on a follow up, although he's working on the patch a bit now - at least I think, unless it's only writing the api.php file :).

It feels like overhead to me, and we need this info in entity_info, which smells like possible racing conditions.
So, if it can be done fast, sure, otherwise, get this in before 1 dec and convert after that.

Working on this.

yes, after a quick review of the patch, the CRUD and form functions would be better implemented with Configurables.

What kind of racing conditions do you anticipate?

Well, Config Entities are parsed in entity_get_info, which is also where view modes are loaded. And, we haven't explicitly prevented config entities from having view modes.

But, I agree that they should be in the long run. It might have to wait on #1763974: Convert entity type info into plugins, not sure yet.

Great patch!
I think we should decide on implementation first:
- use alter or directly in entity_get_info()?
- used storage seems bad, what if someone needs just to disable some view-mode or add a description? Better use a cache or k/v for a list of custom modes.
- I think this needs to be implemented as Configurable to show what is view-mode is. Now it needs to find an edit form to see it's structure. OTOH KISS

+++ b/core/includes/entity.inc
@@ -81,6 +81,23 @@ function entity_get_info($entity_type = NULL) {
+      // Add custom view modes.
+      $custom_view_modes = config('view_modes')->get();
+      if (!empty($custom_view_modes)) {

I think this debatable - having this module disabled we still use this functionality? so it looks like a menu.inc and menu modules... In other case this module should just use hook_entity_info_alter

+++ b/core/modules/view_mode/lib/Drupal/view_mode/Tests/ViewModeTest.php
@@ -0,0 +1,106 @@
+  public static $modules = array('field_ui', 'view_mode');
+
+  protected $profile = 'standard';

Any reason for standard profile? Just point a needed modules.

+++ b/core/modules/view_mode/view_mode.admin.inc
@@ -0,0 +1,182 @@
+  if (empty($rows)) {
+    $rows[] = array(array(
+      'data' => t('No custom view modes available.') . ' ' . l(t('Add new view mode.'), "admin/config/system/view-modes/add"),
+      'colspan' => 4,
+    ));
+  }
+
+  $build['view_modes'] = array(
+    '#theme' => 'table',
+    '#header' => array(
+      t('View mode'),
+      t('Entities'),
+      t('Operations'),
+    ),
+    '#rows' => $rows,
+  );

Please, use #empty of theme_table()

+++ b/core/modules/view_mode/view_mode.admin.inc
@@ -0,0 +1,182 @@
+  $entity_info = entity_get_info();
+  foreach ($entity_info as $entity_type => $info) {
+    if (!empty($info['fieldable'])) {
+      $options[$entity_type] = $info['label'];

In D8 only 'fieldable' entities are allowed to be extended with view-modes?

+++ b/core/modules/view_mode/view_mode.admin.inc
@@ -0,0 +1,182 @@
+function view_mode_exists($machine_name, $element, $form_state) {
+  $entity_info = entity_get_info();
+
+  foreach ($entity_info as $entity_type => $info) {
+    foreach (array_keys($info['view modes']) as $view_mode) {
+      if ($view_mode === $machine_name) {
+        return TRUE;
+      }

I think this useless, view_mode_load() is enough to test that one already used.

+++ b/core/modules/view_mode/view_mode.module
@@ -0,0 +1,119 @@
+  // Save the view mode.
+  $view_mode = array_intersect_key($view_mode, drupal_map_assoc(array('label', 'entity_types')));
+  config('view_modes')->set($view_mode_name, $view_mode)->save();

I think it's not a good idea to store them in one config file. This will make harder to pack them in features.

+++ b/core/modules/view_mode/view_mode.module
@@ -0,0 +1,119 @@
+  entity_info_cache_clear();
+  variable_set('menu_rebuild_needed', TRUE);

Do we really need always a menu-rebuild here?

This addresses most of #21.

Pushed to config-view-modes-1043198-tim.

6758d76 Don't subcategorize these as 'custom'.
b92a2da Use array_filter() for more obvious filtering.
8bccb1a menu_rebuild_needed is now state, not a variable.
11970bb Store each view mode in a separate file.
6b63869 Switch to using theme_table()'s #empty.
3002d33 Replace view_mode_exists() with view_mode_load().

I'm going to attempt this as config entities, just to see what happens.

Whoops.

used storage seems bad, what if someone needs just to disable some view-mode or add a description? Better use a cache or k/v for a list of custom modes.

Oh no, this is CMI for sure, no doubt about that.

So. This needs a bit more work for when I'm awake. But it's pretty close to what I want it to be.

Needs tweaks on the list controller, and docs, mostly.

Looks cool to me. One thing I'm wondering: wouldn't it make more sense to register the system_entity_info in view_mode_entity_info - correct me if I'm missing something here. I can review more this evening as I stupidely missed my flight today so I'm not afk anymore until monday.

+++ b/core/includes/entity.incundefined
@@ -82,6 +82,20 @@ function entity_get_info($entity_type = NULL) {
+          'label' => check_plain($view_mode['label']),

Not sure that check_plain() needed here. Because this info could be used to populate some options so would be double-escaped.

+++ b/core/modules/view_mode/lib/Drupal/view_mode/ViewModeFormController.phpundefined
@@ -0,0 +1,98 @@
+    $label = $view_mode->label();
...
+      '#default_value' => $view_mode->id(),

Controller should use direct property else it could get troubles with translation.

+++ b/core/modules/view_mode/lib/Drupal/view_mode/ViewModeFormController.phpundefined
@@ -0,0 +1,98 @@
+    $view_mode->save();

I'd recommend array_filter() not checked entity_types here

+++ b/core/modules/view_mode/lib/Drupal/view_mode/ViewModeFormController.phpundefined
@@ -0,0 +1,98 @@
+    if (isset($_GET['destination'])) {
+      $destination = drupal_get_destination();
+      unset($_GET['destination']);
+    }
+    $view_mode = $this->getEntity($form_state);

I think we need common practice for confirmation forms. Probably in base formController

+++ b/core/modules/view_mode/view_mode.moduleundefined
@@ -0,0 +1,69 @@
+  $entity_info['view_mode']['list controller class'] = 'Drupal\Core\Config\Entity\ConfigEntityListController';
+  $entity_info['view_mode']['form controller class'] = array(
+    'default' => 'Drupal\view_mode\ViewModeFormController',

Suppose it needs a brief comment for reason of alteration

Keeps going with tim's work, should fix test failures.

damn, cross-post, missed suggested changes from @andypost

I know we just got rid of entity.module, but what about adding that (or entity_ui.module) instead of view_mode module? Then if we convert entity types to CMI as well the UI for those could also live in there.

Arguably most of field UI should move to that module too, it's configuring display modes for entities, fields just happen to be part of that (not in this issue of course).

@catch++ Awesome idea!

0b0da48 Clean up documentation.
e396911 Clean up tests.
a3904da Clean up form controller.
7566419 Rename module to Entity UI.
08b9899 Add the 'entities' column to the view mode list.

8edb2ad Added API docs.

This UI directly mimics Display Suite, which is a simplified (and vaguely incorrect) representation of how view modes work.
But I'm hesitant to expand it until there is more consensus around #1782460: Allow to reference another entity type to define an entity type's bundles.

I'm happy with this in its current state, I no longer consider it a work-in-progress.

Can this have a screen.

Because #1802834: Use #type => operations for EntityListController::buildOperations() is already RTBC, I didn't include that in this patch.
Here is the patch as it stands:

And here is is in conjunction with the other issue:

This is the edit form:

Interesting, I totally did not imagine we would ever add such a UI. However its here, so to discuss it.

1) Could we please add some useful help text all around, most users have no idea what view modes is.

2) What usecase is there for non-hardcore developers, to use this?

3) We need to a good place for all this generic entity configuration stuff, it is in configuration perhaps a new category.

4) There is a lot of technical stuff in the table, that - we cannot do. We need to either have sensible labels, or remove it from the table. We don't expose machine_name in tables generally, neither do we use the word entities in the UI facing part of core.

I am not really fond of UI's that have no use in core, I would like there to be a clear case to put it in.

As I commented in #10 - I think more usable to manage view modes in context of the entities they are related. DS style mostly confusing by editing "view-mode-wtf" without context.

When we talk about print and preview modes that could apply to most of entities it make sense to have a list of entities below the "Label-name" but in case of specific modes more sane to add them within field_ui as @catch mention #31

1. Yes, help text is needed.
2. View modes are the single most useful tool in site building that I have discovered in the D7 cycle. The fact that they are never exposed to site builders is a huge disservice to them
3. Agreed. I'm open to suggestions. It's possible that it can stay where it is now until the Entity UI module is expanded
4. As I discussed with you on IRC, the layout of the list is open to change, which I elaborate on in #34.

I use view modes constantly, and totally agree with Tim's statement that they're one of the most useful site building tools in D7 (not just visual site building, but from a site architecture/ code standpoint as well). As I mentioned on IRC, I co-organize the Drupal Pittsburgh monthly meetup and have probably discussed using view modes at one point or another with every other member that attends. We all use view modes. Really glad to see a management tool going into core.

A couple of quick points:

1. The Entity View Mode module management page is superior to the Display Suite version, which this seems to be patterned after. Personally, the more exact a port of Entity View Modes we can have in D8 the better -- it's ux is simple and to the point. The biggest difference is view modes sorted by entity, which already is a request from a commenter above. This mirrors the code and the rest of the view mode interface behavior, where all view modes are defined and available by entity type.
   
   

   Management page. A little long here, so you can see the different groups.
   
   For D8 the type column could be cut out.

   Edit page. Short and to the point.
   

2. To #10/ lightspeed's request for view modes by bundle: if the goal is to have only some view modes available for a bundle's manage display, that's already possible. Under your bundle's manage display -> default tab, checking and unchecking view modes in the "Custom Display Settings" menu makes those view modes available or hidden for that bundle. You can do this from the Entity View Mode module's page as well, as seen in the second screenshot. Very, very useful.

I would partially disagree that most users don't know what view modes are -- everyone knows the difference between "full content" and "teaser", but not everyone knows that those are view modes and the options they can unlock by defining their own. I've written some Media/File Entity documentation that mentions view modes, but it doesn't cover their full usefulness.

@RobW +++

I don't necessarily agree with a more advanced interface, but that's because I'm biased of course as the lead maintainer of DS. Still, some remarks.

• If you list the 'locked' view modes, you should be able to remove the custom display settings as well. This interface doesn't allow you todo that now. Imo, removing display settings should happen on Field UI (note that doesn't even happen right now and is imo a bug because it leaves redundant info in the database).
• Having the option to remove the custom display settings (either globally or just unchecking the bundle) directly on the edit screen of a view mode is going to cause a lot of frustrations, it's to destructive. I don't mind when deleting though.
• I disagree that unchecking or checking (especially this one) view modes from this page is useful. It's actually not, you still need to go to Field UI to configure the view mode anyway. If we would opt for this option, it doesn't make sense to have two places to enable or disable them, that only adds more confusion. Removing them from Field UI doesn't sound like a good option to me either because that would mean you would have to enable this module by default too.
• I like the fact that the DS add/edit screen (again, biased, I'm the maintainer) has the checkboxes for every entity type. That allows for faster management, because at this point, view modes can't be tied to individual bundles (and shouldn't be on the roadmap here to get this in first, do that in a follow up). It's also in our workflow of course all the time. For example, we always create a 'Short' view mode which is simply available for all entity types. That is managed on one screen. In this scenario, in case there are 5 entity types, I need to go through 5 screens. And with D8 coming, we'll even have more entities available. However, as long as the machine names can be the same, I can live with that, because we rely on 'view-mode-{machine_name}' class in our templates which we should maybe try to make sure this patch adds in preprocess for all core entities.

In short:

• I feel that managing the 'custom display settings' from these screens is too much overhead and will lead to confusion and easily lost display data.
• I do like that all view modes are listed, including those defined by default in hook_entity_info().

Important: I don't want to bikeshed hard on this one though, I DO want an interface in core, and the faster, the better :)

+++ b/core/includes/entity.api.php
@@ -535,3 +535,91 @@ function hook_entity_field_info_alter(&$info, $entity_type) {
+ * @param array $entities
+ *   The view modes keyed by name.

This also applies below. I think it is very strange that we call these $entities instead of $view_modes. We also don't do function hook_node_load($entity) but function hook_node_load($node). This also applies to the other added hook definitions. For this one specifically, I think it makes sense to document Drupal\Core\Entity\EntityInterface for the array values in the documentation as we can't type-hint array values.

+++ b/core/includes/entity.api.php
@@ -535,3 +535,91 @@ function hook_entity_field_info_alter(&$info, $entity_type) {
+  entity_info_cache_clear();
+  state()->set('menu_rebuild_needed', TRUE);

This is quite nitpicky, but I think that's a pretty advanced use-case. How about simply:
mymodule_cache_clear()
This also applies to hook_view_mode_delete().

+++ b/core/includes/entity.inc
@@ -82,6 +82,20 @@ function entity_get_info($entity_type = NULL) {
+      $view_modes = drupal_container()->get('config.storage')->listAll('view_mode.');
...
+        $view_mode = config($config_name)->get();

Any reason this is not using entity_load()? Infinite recursion perhaps? In that case this deserves a comment IMO.

+++ b/core/includes/entity.inc
@@ -82,6 +82,20 @@ function entity_get_info($entity_type = NULL) {
+        foreach (array_filter($view_mode['entityTypes']) as $key => $value) {

camel-casing (entityTypes) seems non-standard for configuration keys. Any specific reason for that?

+++ b/core/lib/Drupal/Core/Entity/EntityViewMode.php
@@ -0,0 +1,78 @@
+  /**
+   * An array of entity types that use this view mode.
+   *
+   * The array is keyed by the entity type, and the value is either the entity
+   * type if this view mode is used, or 0 if it is not.
+   *
+   * @var array
+   */
+  public $entityTypes = array();

I might have missed it above, but is there any reason why view modes are for multiple entity types? Considering how view modes are currently used, this seems like a novel concept. And I don't really see a reason for this. UI-wise this would allow to move the view mode adding/editing to or near the "Manage Display" section of the idividual entity types, which seems more intuitive IMO.
Also, if we stay with this, I don't get why we are using the entity type as value and not simply TRUE and FALSE or, even more simply, why we are not simply storing an array of the used entity types. There seems to be no value in storing all existing entity types here. If I'm not mistaken that would simply mean moving the array_filter() that currently lives in ViewMode::getUsedEntityTypes() into ViewModeFormController::save(). This is of course an implementation detail that could be dealt with in a follow-up.

+++ b/core/modules/entity_ui/entity_ui.module
@@ -0,0 +1,88 @@
+/**
+ * Load a custom view mode by machine name.
+ */
+function view_mode_load($machine_name) {
+  return entity_load('view_mode', $machine_name);
+}
+

It seems strange that we're violating the entity_ui namespace here, but anything else (i.e. entity_ui_view_mode_load()) would be even more weird, so I guess there's no way around that. Simply mentioning here, in case anyone has some thoughts on that.

+++ b/core/modules/entity_ui/lib/Drupal/entity_ui/ViewModeFormController.php
@@ -0,0 +1,88 @@
+      if (!empty($info['fieldable'])) {

I know this has been mentioned above, but this at the very least deserves a comment or (IMHO preferred) a @todo.

One more general remark: I would have expected for the view modes we already have and that are currently defined in code (in hook_entity_info()) to be shipped as default configuration of the entity-providing module. I think the current split between default/in-code view modes and custom ones is unneccessary and unnatural. That probably means we need a 'locked' key on view modes so that we don't break everything that is currently using the default view modes, but I would find that be a superior approach.

One-more thing:

View modes are the single most useful tool in site building that I have discovered in the D7 cycle. The fact that they are never exposed to site builders is a huge disservice to them

I could not agree more. I do not want to spam this issue with a three-page (well, maybe not quite, but...) essay why this feature in core would be the best thing since sliced-bread, but, well, it would be.

And also props for coming up with the idea to use ConfigEntity here, that's super slick!

EDIT: This was a cross-post with the previous post. Mostly different topics, but noting here for reference.

Just quick notes, sorry, cannot read all :-(.

- Totally agreed on "Field UI" being actually an "entity UI"
- Just like custom fields created in the UI have a "field_" prefix, machine names of custom view modes need a prefix ('custom_' or smthg) to avoid clashes with module-provided view modes

+++ b/core/modules/entity_ui/lib/Drupal/entity_ui/ViewModeFormController.phpundefined
@@ -0,0 +1,88 @@
+    $destination = array();
+    if (isset($_GET['destination'])) {
+      $destination = drupal_get_destination();
+      unset($_GET['destination']);
+    }
+    $view_mode = $this->getEntity($form_state);
+    $form_state['redirect'] = array('admin/config/system/view-modes/' . $view_mode->id() . '/delete', array('query' => $destination));

$destination could be empty

Okay, so this is very much a WIP, I know there is a lot of hacky code and missing docs.
Please only comment on the approach.

Here is the new UI. You might recognize it.

Generally +1 on the approach. Nicely done, Tim!

Although the amount of code needed here makes me concerned that we still have a way to go in terms of the Entity API. That's not something we'll resolve here, though.

Other thoughts and comments I had while perusing the code, for completeness:

+++ b/core/includes/entity.inc
@@ -83,6 +83,22 @@ function entity_get_info($entity_type = NULL) {
+      $view_modes = drupal_container()->get('config.storage')->listAll('view_mode.');
+      foreach ($view_modes as $config_name) {
+        $view_mode = config($config_name)->get();
+        $definition = array(
+          'label' => check_plain($view_mode['label']),
+          'custom settings' => $view_mode['custom'],
+        );
+        $name = strtok($view_mode['name'], '.');
+        $name = strtok('.');
+        if (isset($view_mode['type']) && !empty($name)) {
+          $entity_info[$view_mode['type']]['view modes'][$name] = $definition;
+        }
+      }

If we've already got the config manager thingie from the container, why call config() again? Or is config() doing more non-injectable things we need to clean up?

+++ b/core/modules/entity_ui/entity_ui.module
@@ -0,0 +1,95 @@
+function entity_ui_entity_info_alter(&$entity_info) {
+  // Add a list and form controller for the view mode entity type.
+  $entity_info['view_mode']['list controller class'] = 'Drupal\entity_ui\ViewModeListController';
+  $entity_info['view_mode']['form controller class'] = array(
+    'default' => 'Drupal\entity_ui\ViewModeFormController',
+  );
+}

I'm confused. If you're defining the ViewMode config entity yourself, why do you need to alter this in rather than defining it from the get-go?

+++ b/core/modules/entity_ui/lib/Drupal/entity_ui/Tests/ViewModeTest.php
@@ -0,0 +1,92 @@
+class ViewModeTest extends WebTestBase {

Insert usual rant about unit vs. web tests here. :-)

1)

function config($name) {
  return drupal_container()->get('config.factory')->get($name)->load();
}

is different enough that I just copy/pasted from elsewhere.

2) Until entity info are plugins, the system module is providing the view modes entity type. The Entity UI only provides the UI portion, that way it can be disabled/uninstalled without killing all of the view modes. That should and will stay in an alter even after the aforementioned issue goes in.

3) Yeah yeah :)

I need to spend some more time with this, but I don't think I like the idea that all module-defined view modes are now strictly config (meaning, deletable by site admins). A contrib module cannot expose a 'foo' view mode and do entity_view($entity, .foo.) and be sure that the view mode still exists.

The goal here was to allow custom view modes as config in addition to module provided view modes, not to move all view modes to config land. Why did ee make that switch ?

I think it would be terrible DX to have some view modes in hook_entity_info() and some in config YAML files. You are absolutely correct that we should not allow admins to delete module-defined view modes, but (unless I'm missing something obvious) that should be as simple as adding a 'locked' attribute on ConfigEntity, which for various reasons makes sense anyway.

It's a really great question how to extend the config entities or to lock'em! The same thing we need for #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity)

Because the patch was assigned to me and I thought it was a good idea :)

Here's a "locked" key for module provided view modes to prevent deletion.

I didn't see #50 or #51 before posting, seems we were on the same track.

This removes the ability for hook_entity_info to provide view modes.

This removes the ability for hook_entity_info to provide view modes.

#54 just remove ability to provide 'custom settings' by modules. Not sure we have tests for that but suppose that kills ability to provide a customized view modes in features

Fixed bug.

I'm not too concerned with explicitly preventing usage of hook_entity_info to add view modes, since #1763974: Convert entity type info into plugins will be removing that hook.

This removes the ability for hook_entity_info to provide view modes.

View modes do not belong in there, as quite some other stuff. I'd be very glad to see it being moved elsewhere.

Okay, here's a pass to clean up some of the code, which should address all of #43.

This really needs reviews now, I think this is pretty good.

The menu loader should match the entity type. Otherwise it conflicts with hook_ENTITY_TYPE_load().

+++ b/core/includes/entity.api.phpundefined
@@ -120,27 +120,6 @@
- *   - view modes: An array describing the view modes for the entity type. View
- *     modes let entities be displayed differently depending on the context.
- *     For instance, a node can be displayed differently on its own page
- *     ('full' mode), on the home page or taxonomy listings ('teaser' mode), or
- *     in an RSS feed ('rss' mode). Modules taking part in the display of the
- *     entity (notably the Field API) can adjust their behavior depending on
- *     the requested view mode. An additional 'default' view mode is available
- *     for all entity types. This view mode is not intended for actual entity
- *     display, but holds default display settings. For each available view
- *     mode, administrators can configure whether it should use its own set of
- *     field display settings, or just replicate the settings of the 'default'
- *     view mode, thus reducing the amount of display configurations to keep
- *     track of. Keys of the array are view mode names. Each view mode is
- *     described by an array with the following key/value pairs:

This paragraph would be great for a hook_help! Anyone up for that?

Updated issue summary

Updated issue summary.

add html

#63 +1 for that

Okay, added a hook_help().

Looks good. I was about to complain that node.module can be disabled, so we would need a module_exists(), but then I realized:
A. If you take everything related to node.module out of the current hook_help, there's nothing left. :-)
B. The only non-optional entities currently are user and file, neither of which have multiple view modes currently, so they don't really serve as examples.
...so, nothing to complain after all. :-)

Wow, hook_help fail.

#68 looks good for me

Ehm, don't we need to review the UI this is introducing. I have only done a partial review, can we get some new screens?

Here are updated screenshots.

List:

Edit:

I'm not a fan of the 'Use custom display settings'. In drupal 7, this is located at the manage display screen, and you are able to enable / disable custom settings per content type. Moving this to a setting for all content types, looks a bit to much.

For example:
My site has following content types: page, news, events.

For events and page, i want featured to look like the default display settings.
For my news, i want to have different display settings for my featured view mode.

With this system, you won't be able to do that. (Correct me if i'm wrong :))

#72, that's still 100% possible with this. In fact, it shares similar code to that page. It's just a centralized way to control it.

That checkbox list is per bundle (for node, "content type"). No functionality is lost.

Ok, a full review - sorry if some parts might be brief I am trying to cover everything in a timely fashion. This seems to add a new concept to core, so excuse me if I misunderstand certain choices.

Labeling/descriptions
I am not entirely convinced that "view modes" is the correct label, although this might be a can of worms because of legacy. But I always saw these things as "display modes", since they are dealing with the display of things. We tend to use the word "display" to mean what you are seeing at least in Drupal UI, the word "view" is largely a code label. I also think there might be some confusion with Views?

We could make this description more descriptive, perhaps - Configure view modes for content, file, user displays, or Settings for view modes. Tell them a little more about what they can actually do here, "Manage" although used all over core is really not that usefull, heh :)

Please take a look at Interface text, at large though we try to avoid overly lengthy descriptions because people don't really read them and try to apply the twitter rule. This description seems lenghty and is packed with new words for users to understand.

I propose we take out words like view modes, node, context, field api, entity. And do something like:

"View modes allow you to apply different ways to see for example a content item, on its own page ('full' mode), on the home page ('teaser' mode), or in an RSS feed ('rss' mode).

Can this just be "Type", I dont feel comfortable with introducing the word Entity throughout the UI.

Visual/interaction
We are introducing a lot of new patterns with the table, I'd like us to take a small step back and see if we want to introduce this. because it might turn this UI into a one-off.

1. Lets remove the fieldset around each table. The other place we have this in core, is the modules page. And frankly it is a huge visual distraction. That can be solved in many other ways, for example having larger titles (Rules does this).
2. The "Add new Comment view mode" inside the table seems like a one-off, we don't have that anywhere else in core. We could introduce it here, but I cannot see many modules in the ecosystem using this pattern. Can we instead just have a chooser, e.g. same as when you click "Add content" and have a general "+ Add view mode".
3. The "Edit" button seems floated to the right, is this actually a dropbutton bug or?
4. Its not common to have the machine_name in a table, is there a 80% reason to have it here? Its normal for the machine name to be exposed on editing the object. We do that with content types too.

• Can we change "Label" to "Name". This is a unwritten and not consistently applied rule, but for adding "objects" we tend to use the label "Name" - see content type for these kind of config objects.
• The "Custom settings" functionality is a little puzzling to me. It seems not checking that in the case of content types enables, it for all content types but checking it allows you set it for only a few content types. If this is required for code, can't we just check whether they checked all or only a couple and then determine its custom or not, removes the need for a checkbox to see more checkboxes.
• I don't really like having an "you will delete things, when you click this checkbox!". Deleting should be an explicit action, not a checkbox. Perhaps, this can be a confirm form when you uncheck something?

There you go :) I can elaborate where needed.

+++ b/core/modules/field_ui/field_ui.moduleundefined
@@ -412,3 +415,14 @@ function field_ui_library_info() {
+/**
+ * Implements hook_view_mode_presave().
+ */
+function field_ui_view_mode_presave(EntityInterface $view_mode) {

This might be a problem as field UI can be disabled. While the chances are that it's going to be enabled because you're in this area, it's still possible. I think we should move this to the entity_ui module itself.

Other than that this looks fine to me - not totally 100% fan of the interface, but I don't mind and I need to shut my biased view :)
I like the locked option because that makes the definition of the view modes consistent throughout core. We'll need change notice for this then.

Let's hope the UX people like it!

+1 for improving the hook_menu description

RobW is going to work on the hook_help

If the module is called "Entity UI", I really think it's reasonable to use that word elsewhere :)

The fieldsets were borrowed from Entity View Modes, they can be H3's above each table
The machine name in the table is a default behavior of the list controller
The dropbutton floating is a known issue (#1799498: Improve dropbutton)
I'm not sure about the "add new %type mode" stuff, I particularly like that, and I think the two step process would be a bit much

Label vs Name, meh.

We could simplify this by removing the custom settings from here, or just work to clean it up.

@Bojan's review:

I think the name needs to stay View Modes. Current users familiar with the term already, and the name permeates a ton of other modules, and changing it would require a massive patch to redefine the machine names/functions and function calls across Drupal.

On a similar vein, Label is referred to as name, title, or label in a couple different places. Label is IMO most descriptive and also the machine name/ api name. I agree differences can be confusing, but I'd rather see all of Drupal (aside from node, where title has a long history and makes more sense) standardize on label.

I'm not sure that we can get away without describing things as entities. I started writing a full hook_help() today, and have used the term entities extensively in the full admin page help. "Types" doesn't really cut it, because... types of what? Types of text? Types of content? Types of views? Entities are the only thing that can have view modes on them. I don't think excluding the term actually makes anything clearer; it just sidesteps an important bit of info by obfuscating it. I think if you're deep enough into building a Drupal site that you're creating your own view modes you probably (or at least, should) know the basic structure of the system, and using entities shouldn't be a problem. I'd love to hear alternate suggestions though.

I am a little lost, seems like most of my review suggestions are rejected. What are you guys gonna tackle?

I'm not removing the word "Entity", since its already in the name of the module "Entity UI".

The generic EntityListController is where the machine name is added, if we don't want it showing up on every ConfigEntity, we should remove it there.

I'm removing the fieldset as you asked.

I haven't decided about the "add new" links, leaving them as is for the next patch.

On further reflection, I agree with changing it to "Name"

I'm going to remove the custom settings part from the Edit UI, though it should still be in the listing IMO.

Instead of calling it "custom settings", "enabled" might make more sense. Even as a developer who spends a huge time in the theme layer, that term in the Entity View Modes module always bothered me.

I don't think 'entity' is a good word, to me its a fancier word for "thingy". I will leave that point its clear after discussing with Angie we are not going to find a good alternative or avoid that word. We will let userdata show if its a clear or unusable label, which fyi is not going to happen before commit unless hordes of usability testers show up at my doorstep :)

I have no idea what this patch is doing yet, but was asked to chime in on the naming issue. I think not allowing us to use "Entity" as a word in the UI is unnecessarily boxing ourselves in. Because not only is that what these things are called in code (which would create another node/content and category/taxonomy fiasco) but "Entity" is actually a very common word in the data modeling world for exactly what Entities represent in Drupal (unlike "node" which never ever means "blog post" :P).

If we have data that shows this complicates things for users and a better word emerges, we can re-visit this then.

This removes the confusing "custom settings" logic from the UI.

In addition, it changes the entity keys to match @Bojhan's request for "Name" over "Label".

Since it's now using a caption tag, the screenshots look a bit silly, but that's being addressed in #1817996: Add sane default styles for table <caption>

This patch also now attempts to use entity_load_multiple() inside entity_get_info, which passed in this test, but it remains to be seen how it does against the whole suite.

I did not touch the "Add new %type view mode" links, I wanted to talk to swentel first.

Also, I explicitly put in an @todo in hook_help where the expanded help should go.

List:

Edit:

I'd still like us to tweak the top description, there is no reason to put a 4-line description in core.

"View modes allow you to apply different view modes to entities for example a content item, on its own page ('full' mode), on the home page ('teaser' mode), or in an RSS feed ('rss' mode)."

To elaborate why I don't want to put in the last row as action, is because its creating a one-off pattern. In core we are generally pretty strict about introducing new patterns like this, as it could easily be abused by contrib projects - who find they should also use a sexy feature like this. Vertical tabs for example, spawned a whole lot of unnecessary vertical tabs in contrib. In this case, we don't know how usable it is, we don't know if its accessible (e.g its not at a normal location, people might just skip it), its a new pattern we don't use elsewhere in core at all, the is only a small number of contribs that do it as far as I know. For the record I dont think the UI thing itself is necessarily a bad thing, I designed it for Rules2 UI.

Blergh, looks like the entity_load_multiple won't work after all. That was such a nice improvement...

I hope plain text is good for review right now -- I'm not finished setting up a development environment yet on my new machine so no patches. Guessing that we need a subtitle for the link on the admin config or structure page, a brief description for the manage view modes page itself, and a full help for the admin page. I'm basing the style and length off of the Image and Image Styles help text. First draft:

Subtitle for the admin/config page:

View modes:
Create and manage view modes for displaying content in different contexts.

The manage view mode page:

View modes allow entities to be displayed and manipulated depending on their context. A common example is configuring different field visibility, formatters, and template files for a content type's teaser and full content view modes.

The full admin view mode help page:

About

View modes allow you to display and manipulate pieces of content (entities) on your site depending on their context. You're probably familiar with the default Drupal view modes "full content" and "teaser", and configuring field visibility, formatters, and template files for each. Any number of custom view modes can be created for each entity type through the view mode administration page, allowing for a very flexible system of configurable contextual display.

Uses

It seems like most modules blur a step by step instruction list here with a list of uses. I'll come back and edit the full help descriptions in over the next day or so.

Creating custom view modes

Content to come

Managing an entity's display with view modes

Content to come

Displaying content through a custom view mode with Views

Content to come

Using view modes in template files

Content to come

View Mode administration pages

• View Modes
• Permissions
• ?Every entity type's Manage display page?

Does the word "context" here clash with another, more canonical use of it in D8 core? Drupal really needs a voice/ audience/ term guide for hook_help()s -- maybe something to wok on after D8 launch/ when I have more time.

This fixes #83, and adds the WIP from #87.

@RobW, assuming this passes, I'm done, so feel free to reroll the patch adding your docs directly.

A couple of things, one that's important:

1. we need an update hook to import the core view modes on an existing installation, should go into system module
2. Maybe we should move the 'Node' view modes to the top of screen, because they are usually the most important ones (noticed that during DS user testing when we construct our manage display screens). I find the 'entity type' label above the tables a bit weird. Maybe we should move this label into the first table cell instead of 'Name' ? Or is that a bad idea ?
3. The admin screens looks a bit 'wonky' because of the operations columns not being equal for every entity type, maybe we should add a fixed width here ?
4. Taxonomy vocabulary isn't fieldable so that shouldn't show up here

Alright, new patch, sorry I couldn't do an interdiff.

Changes

• Remove the caption above the tables and moved into the first table cell, looks nicer to imho. (see screenshot)
• Added an admin css file and a class on the operations column so it always has the same width (see screenshot)
• Added an upgrade test (one patch should have 4 fails, the other should be green)
• Added check plains on the entity type and view mode label
• Added check that the entity type is fieldable
• Added a weight on the entity types on the overview where Node is always the first
• Added the print view mode that book exposes

Here's the interdiff, for anyone curious.
Those changes look fine to me.

The CSS is a one-liner, wouldn't it be better to have this in another already existing css file? system.css or such?

Had a quick talk with Bojhan about the UI and didn't like the move of the entity type name to the table cell. Brought back the caption but added some CSS so it looks a bit how the tables on the Rules UI work, suggested by Bojhan. And it actually looks nice indeed. Pounard, so the css is a big bigger now ;)

Patch also fixes some stupid typos in the update hook.

+++ b/core/modules/entity_ui/css/entity_ui.admin.cssundefined
@@ -2,7 +2,14 @@
+table caption {
+  font-size: 110%;
+  font-weight: bold;
+  text-align: left;
+  padding-bottom: 0.5em;

This should be in #1817996: Add sane default styles for table <caption>, or just move it to the system module CSS and mark that a duplicate.

Also related issue #1503464: Automatically add theme hook suggestion for node view mode should be helpful for themers

Whoo :)

I am still not sure about that action bar being the last row, I see no real purpose for this pattern in other parts of core and really only a few contrib modules. It feels too much like a one-off, wondering what others think also marking for accessibility review.

I don't know how it plays from an accessibility standpoint, but it feels OK to me usability wise. It gives the impression of adding one more to the end of a list (which it is), and naturally degrades to the "empty" use case. And, more to the point, I can't think of anywhere else to put that wouldn't be worse. :-)

Damnit, Drupal!

#93: 1043198-93.patch queued for re-testing.

I think the test is glitch in the system somewhere (unless the views commit has triggered something, although I could not think what tbh).

Regarding the caption, not sure how to proceed here, I'm bad at making decisions on css/frontend level :)

- edit - we could also ditch the caption and add a #prefix <h3>title</h3> - thoughts ? c'mon let's get this in :)

The label on a table should be a caption. That's what it's for. How it looks, I don't care. Ask Bojhan. :-)

Purely down-to-earth code review below. I'm still very doubtful of the overall "move all to config land" approach, but I'll try to summarize in a later post. The following remarks will apply no matter what.

+  /**
+   * The entity type this view mode is used for.
+   *
+   * @var string
+   */
+  public $type;

$entity_type / $entityType is commonly used anywhere else - can't we use that here too instead of $type ? (same goes for the entry in the yml file)

+  /**
+   * Whether or not this view mode has custom settings in the Field UI.
+   *
+   * @var bool
+   */
+  public $custom = FALSE;

Doc is misleading. This is not about Field UI, this is whether this view mode should use dedicated settings or simply the 'default' one. This can be set in Field UI, but everything can be set in Field UI :-)

+    if ($type != 'view_mode' && !empty($info['fieldable'])) {

Wow. Is this the only way we have to differenciate content entity types ('node') from config entity types ('image style', 'view') now ? I hate that entity smushing ... :-(.
Also, I'm not sure $info['fieldable'] is really the criteria - a content entity type might only have 'base fields' (aka new property API) and not be 'extensible' (aka fieldable'), but still have a view operation (and thus view modes). I'd say the presence of a 'render controller' class would be more accurate.

+function entity_ui_view_mode_presave(EntityInterface $entity) {
+  entity_info_cache_clear();
+  state()->set('menu_rebuild_needed', TRUE);
+}

Why do we need a menu rebuild ? Menu items in entity_ui_menu() are wildcarded and do not depend on the actual list of view modes.
Also, the entity_info_cache_clear() should happen even if entity_ui is disabled, so it seems it should be done within the EntityViewMode class.
(same remarks apply to entity_ui_view_mode_delete()).

(in field_ui.module)
+use Drupal\Core\Entity\EntityInterface;

Seems unneeded ?

1) That would conflict because it, as a config entity, has $entityType itself.
2) True.
3) I agree that 'fieldable' isn't good enough. I like the idea of 'render controller class'
4) That's copied from Entity View Modes, not sure
5) Must just be left over

For fieldable, can't we use an instanceof check since config entities and content entities have different interfaces? If not, we probably need some kind of entityTypeThatIsntCalledType() method that returns "content" or "config" or whatever.

We haven't explicitly said that ConfigEntities can't have view modes.
If someone decides they want to have view modes be rendered, they can have a render controller.

I think it's an elegant solution.

1) That would conflict because it, as a config entity, has $entityType itself.

Oh doh. Right.
Well, then maybe $targetEntityType ? Because $type is really inaccurate, you can't really say 'teaser' view mode has a type of 'node' - as opposed to the common meaning of 'node 1' has a type of 'article'.

render controller : I'd even consider pushing this and move the list of view modes from entity_get_info() to a separate method on RenderControllerBase (or whatever its name). That specific bit would probably be for a followup, though.

+1 to both of those.
Though the second one will wait on #1763974: Convert entity type info into plugins, where it is relegated to hook_entity_info_alter().

I spent some time reviewing this issue. However, not the code, only the discussion and the proposed concept.

Here's what I found:

From #14:

View modes are what a module call when they do [entity_type]_view($entity, $view_mode) or [entity_type]_view_multiple($entities, $view_mode). You don't necessarily know - and at any rate don't need to care - what bundles $entity or $entities belong to, you only know they are of a given entity type.
For instance, when displaying entities with Views, you can pick a view mode. You do this without knowing which bundles the selected entities will be.

Please excuse my ignorance, but in the same way a module hard-codes the entity type, it also hard-codes the view mode, doesn't it?

If all entity listings were Views views, then the view mode would no longer be hard-coded but sorta configurable. (introducing a new dependency between a configured view and configured entity view modes) But that's not the case yet.

For an entity's page view, the view mode is hard-coded into the router / render controller.

Overall, I think I'm missing an answer on the question "Who is the authority for view modes?" We can turn them into config, but then we can no longer hard-code them, anywhere.

I think we need to address this fundamental question.

From #31 and #37:

what about adding that [entity.module] (or entity_ui.module) [...]? Then if we convert entity types to CMI as well the UI for those could also live in there.

Arguably most of field UI should move to that module too, it's configuring display modes for entities, fields just happen to be part of that (not in this issue of course).

I am not really fond of UI's that have no use in core, I would like there to be a clear case to put it in.

Ideally, entity[_ui].module should be detached from Field UI module, allowing the Standard installation profile to enable Field UI module without enabling Entity UI module. Thus, we expose all the glory of fields, but leave the more advanced entity type/display/view-mode configuration out of the starter kit and initial Drupal experience (i.e., not enabled by default). Drupal claims to be modular, so this granular separation of concerns ought to be possible.

Comparing #36 and #40:

The biggest difference is view modes sorted by entity [...], where all view modes are defined and available by entity type.

This change is what caused the UI pattern problems later on. I have to amend that I looked at the latest screenshots first and did not see the earlier ones, and I was very negatively impressed by the amount of tables, content, and clutter on this overview page.

The former, combined presentation was much more understandable, discoverable, and usable. Furthermore, the separation into entity types caused a disparity between overview and edit form in that the latter allows you to assign the view mode to other entity types — However, you actually clicked on the edit link of a view mode for a particular entity type.

In trying to understand why this change was proposed, I can only guess that site builders are sometimes (but not always) interested in the question of what view modes are available for a particular entity type. For this question, the combined listing is a bit cumbersome, since you need to check every row for whether it shows the entity type of question.

I think this can be addressed and resolved though. I think we should revert to the combined listing UI in #36, and instead, introduce a select list on this page that allows you to filter the table rows by a certain entity type. Essentially, that's the same requirement we have for the table on the Modules/Extend page, and even without that connotation, I'm fairly sure that we can whip up a JS-based table row filter for this page very quickly. However, I'd recommend to do that in a separate follow-up issue.

That would resolve all the UI pattern problems that @Bojhan raised (moving the single add link back into a local action), and also removes the need for specially styled table captions.

(I just see that I seem to mostly agree with @swentel's critique in #42 that more or less states the same but on technical grounds.)

Aside from these points, this looks like a good addition from a conceptual standpoint (again, I didn't look at the code yet).

Overall, I think I'm missing an answer on the question "Who is the authority for view modes?" We can turn them into config, but then we can no longer hard-code them, anywhere.

I think we need to address this fundamental question.

I've been wrestling with this as well, and I think @yched's suggestion of tying them directly to a render controller is a good solution.

--

The problem with #36 (Display Suites' approach), is that it misrepresents the system.
And the UI would not map to correct storage.

The Node 'full' view mode has a label of "Full content".
The User 'full' view mode has a label of "User account".

The implementation modeled after DS would not allow that, it implies that a view mode is shared across entity types, and only allows for a single label.

This is wrong, and is why we had to abandon that implementation.

--

Field UI has no dependency on Entity UI, and the view modes do not depend on it either. View modes are owned by system.module, and can be a standalone Core plugin after #1763974: Convert entity type info into plugins.
entity_ui_entity_info_alter() adds in the list and form controllers.

don't want to derail overmuch, but as it pertains to "render controllers" - on the blocks & layouts hangout we had last week, we began suggesting that the base storage format for view modes might be Displays, a la #1817044: Implement Display, a type of config for use by layouts, et. all. they are, at present, implemented as a ConfigEntity. that would mean allowing view modes to use layouts, or some subset of their functionality, and making their contents block-oriented. doing so is not THAT hard, but would inherently be blocked on #1535868: Convert all blocks into plugins.

if we do that, then the natural follow-on from that is that they can share at least some characteristics with the more general page controller renderer - a pseudocode version of which is the DrunkController in #1812720: Implement the new panels-ish controller [it's a good purple].

Still need to carefully read @sun's #110, but quickly responding @sdboyer's #112 :

View modes and displays would most probably be two different animals, with a layer of indirection between them.
- Each entity type has a list of view modes - those are the 'flavours' in which you can ask an entity_view() for this entity type. We need to allow user-defined ones, in addition to module-defined ones.
- For each bundle within the entity type, each view mode is configured to use a given 'display' (i.e. a collection of fields to display, with their order and formatters and formatter settings - those live in CMI).
Ideally, a given display can be reused for several view modes (i.e. 'teaser' and 'rss' use the same display - they get rendered the same, you configure only once). That is, for each bundle, we have a mapping (in CMI) between view modes and displays.

So :
- view modes are per entity type (and thus can be used for multiple entity view, aka lists, for which the bundle varies), and module code needs to be able to make assumption about the existence of a view mode (aka view modes are, largely, *not* config).
- displays are per bundle (since they list specific fields), and purely CMI land. A module cannot and should not need to make assumptions on the existence of a specific display.

So all in all, I think this issue and #1817044: Implement Display, a type of config for use by layouts, et. all should be largely independent, since the latter will only affect what happens within entity_type($entity, $view_mode), not the list of available view modes.
There might be an impact UI-wise, of course, but we'll see when we get there.

Have an updated patch coming up that tries to implement Bojhan's suggestions. Am now checking the recent comments for stuff to incorporate additionally.

@yched - ah yes, this distinction makes perfect sense. thanks, i hadn't thought of it that way before: displays might describe instances of view modes, but the view mode itself still needs to be defined somewhere.

cool beans.

Btw, the re-roll will take a bit longer than I had hoped, as I started converting Field API to use the new view mode objects. If anyone wants to jump in, I can post a WIP patch more or less anytime. Hope to get this done on the weekend, nevertheless.

as I started converting Field API to use the new view mode objects

What? I'm not sure what this means.

Field API has display settings per view-mode, where you configure which fields to display in which order, with which formatters, etc. It currently only passes the name of the view mode around when it manages those display settings. I have no intent in changing how those display settings are changed themselves, but what Field API currently also stores is whether a view mode has custom settings or not. This is now signified by the $custom property on the ViewMode class. IMO it would be nonsense to have Field API manage that on its own, on a separate layer, completely disregarding the actual view mode configuration. Most of the code changes are trivial or semi-trivial and it so far leads only to simplifications or the code, but there are quite a few lines involved.

Please, please, please make that a follow-up.
field_bundle_settings is a lot to *start* digging through after 119 comments.

View mode should be a kind of decorator/style plugin to router/render couple (as #110) and the best way to manage them with applied layout like a piece of config that field ui already manages

As I've tried to explain above, I'm trying to go with the least invasive changes possible. I'm not revamping Field API bundle settings.
But leaving Field API out of the mix entirely would leave view modes in a terribly broken state, and it makes zero sense to commit it that way. If that turns out to be some sort of consensus, then I won't stand in anyone's way, but for now I'm going to pursue that route. As stated, while I'll admit it'll a couple more hunks to review, I'm mostly removing stuff and replacing a bunch of heavy functions and function calls with simple entity_load()s.

@tstoeckler:

what Field API currently also stores is whether a view mode has custom settings or not. This is now signified by the $custom property on the ViewMode class. IMO it would be nonsense to have Field API manage that on its own, on a separate layer, completely disregarding the actual view mode configuration. Most of the code changes are trivial or semi-trivial and it so far leads only to simplifications or the code, but there are quite a few lines involved

What Field API stores in its 'bundle settings' variables is whether, *for a given bundle*, a view mode has custom display settings or just reuses the 'default'. That's per bundle.
$view_mode_entity->custom is cross bundles. It holds the default value of the above. That is, when the module exposing the view mode is enabled and before the admin visited field UI to configure the bundle.

So, albeit related, those are two different things, and I don't think the current patch breaks anything on that regard (cannot fully test though).
Unless things are broken, I'd agree with @tim.plunkett that we don't want to change this area in this patch, which is already too large for my taste given the initial scope.

The work on Entity Displays (#1817044: Implement Display, a type of config for use by layouts, et. all, mentioned in #112/#113 above) will let us reshuffle things in this area if needed.

#93: 1043198-93.patch queued for re-testing.

@yched: Thanks for that explanation. In that case I totally agree with @tim.plunkett that this should be left out and I will do so/have done so. I'll open a new issue to clean up Field UI's handling of view modes after this.

This patch is a pure reroll from #93. It was quite a pain to re-roll. When previously calling entity_load_multiple() inside of entity_get_info() worked, now calling it inside of EntityManager::processDefinition() leads to infinite recursion. I managed to get a grip on this with:
A. A change to entity storage controllers' constructors. They now get the entity info passed in, instead of calling entity_get_info(). This was small enough of a change that makes sense anyway (DI, anyone?), and works (assuming this comes back green), that I went ahead and did it.
B. Introducing EntityManager::processDefinitions that processes all definitions at once. That is, because we need to access view_mode's definition while accessing the other ones.
C. Two little hacks, that I've tried to motivate in-code. The (IMO, small) caveat is that it is not possible to swap out the EntityViewMode class with one that has a different implementation of EntityInterface::label. Once the entity info is properly injected everywhere, that should be solvable, though.

For the super-nerdy I've attached a diff of the two patches (a real interdiff was impossible). Note that because I already had a bunch of changes on top of this patch, I've had to manually re-extract all the hunks in #93, in order to create this clean patch. Therefore I'm 99.999% sure that I haven't missed anything and that I didn't include any extra changes. Of course anyone who feels like melting their brain is welcome to open that file and verify my claim. :-)

My plan originally was to provide patches addressing the comments above on top of this, but I'm pretty burnt out for tonight. I'm still leaving this assigned to me, because I want to pick this up tomorrow or so, but if anyone else can't wait, at least now the duplicated work should be minimalized.

Related: #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory

Oops, the previous was a crosspost. Will try to figure this out now.

Ahh, I had only tested the installer with minimal profile.

This was a case of rdf.module calling entity_get_info() in its hook_entity_load() implementation. That is really unavoidable short of huuuge hacks. Therefore I went back to loading config objects directly. Not as nice, as in alterable, but the actual use-case of altering that entity, is 0, and we can also look into fixing that once entity info is being injected everywhere.

Because of that, we no longer need the separate processDefinitions and also the storage controller change is no longer necessary. Therefore, extracted that into: #1828640: Pass entity info into entity storage controllers instead of calling entity_get_info()

This installs fine for me, and passes at least view mode tests (as did #125...), but as HEAD is broken ATM, this will be the last patch for now. I will only start rolling fixes/improvements into this, once we have a green here.

Forgot to mark needs review.

Aw - exactly the kind of cross-dependency issues why I advocate to move bundles and view modes outside of entity_info() in #1822458: Move dynamic parts (view modes, bundles) out of entity_info().
Was bad in D7, got even worse in D8 now that config are entities too - thus, a lot more possibilities for infinite loops... - load some configEntity within the building of your own entity info, boom...

I advocate to move bundles and view modes outside of entity_info()

You and me both.

#129: 1043198-129-re-roll.patch queued for re-testing.

I totally agree with that, but doing that here would require lots of changes to field_ui.module and would also require moving bundles out of entity info simultaneously (because of rdf.module).
Just pointing that out, not sure yet if I'm in favor of doing this in a follow-up or not. There are still a bunch of todos already here left to tackle.

#135, English is weird. But the title was correct.

#137, I say do what you need to do, and open follow-ups for the rest. @tstoeckler++

Discussed this issue with swentel today. The issue title already gives a good clue about why this is at 135 and not (nearly?) ready to go:

- convert stuff to config
- add a UI for it

Ambitious :-) And looks like the conversion part is already complex enough with consensus only just now emerging. I can only add thoughts from a UI/UX perspective:

- View modes are a most powerful site building tool. Would be a big win for site builders to make this available with a UI
- I was surprised to see the listing/add/edit of view modes tucked away in an admin/configuration sub page. Thinking about them in the context of content types, why not allow to add them in-context?

Anyway, yay for any progress being made here. View modes rule.

Well the complications of this patch are (mostly) not UI related. The UI you envision is a bit more complicated, than what was above, but if that's the way we should go, I'll/we'll go there. :-)

How do you envision editing and deleting view modes, then, though? Also we've always allowed to change the machine name (for advanced users), which doesn't seem to be part of your proposal.

Either way, I hope to get to a point where we can discuss the UI, and implement it (in case it diverges from the current one) in a matter of days. Until then I won't provide any own mock-ups, though.

Thanks anyway for diving into this. :-)

Ahh, since we're using config directly now and not the entity, we need to strip the entity type manually from the view mode name. I forgot about that.

Let's see if this cuts it.

Opened related issues:
#1829504: Fix WebTestBase being tied to Node and Comment module
#1829516: Stop passing in $view_mode as a string and pass the EntityViewMode entity instead

My mockup was mostly an incomplete sketch to stir the pot a bit :) There would still need to be a central place to manage/edit/delete view modes for sure. Thanks for the push forward.

This should pass, finally. Interdiff fixes the tests. In the meanwhile I've had to re-roll some hunks, so diffing the patches will yield something differently.

Now on to actually fixing some of the issues brought up here. If for reasons that will elude me this still fails, then I'll simply incorporate additional test fixes with the other fixes.

OK, here we go. The changes in this patch, in order of descending importance (more or less)

1. Changed the entity type label above the each table to be a <h3> instead of a <caption>. The recent discussion in #1817996: Add sane default styles for table <caption> indicates that that is actually the more correct usage. That allows us to remove some CSS.
2. Renamed EntityViewMode::type to EntityViewMode::targetEntityType per @yched's suggestion. I added a (maybe too lengthy...) comment in that variable's documentation to note the difference between it and EntityViewMode::entityType. For consistency, I also renamed the corresponding variable(s) in ViewModeListController. Because I didn't like stuffing our own type key into the entity info array, I split the variables into ViewModeListController::targetEntityType and ViewModeListController::targetEntityInfo. That last part is very debatable, but I couldn't resist (sorry... :-)).
3. Renamed EntityViewMode::machine_name to EntityViewMode::machineName for consistency with our OOP coding standards. What is strange is that I had to also update the corresponding keys in the YAML config files, where I would have usually used lower_case_with_underscores. I don't know what our standards for that are, but it seems this affects all ConfigEntities. Thoughts?
4. I changed the list of entities which have view modes from checking for whether the entity type is fieldable to whether the entity type implements ContentEntityInterface. I also like the idea of utilizing the render controller which was mentioned above, but currently every entity has a render controller because it gets a default one if it doesn't declare one, so I don't know how to check for that. I'm open to better suggestions, but that seems to match the current expectation and I added a @todo to revisit that.
5. Removed entity_ui_view_mode_presave() and entity_ui_view_mode_delete(). They were doing two things:
   • Clearing the entity info cache. I moved that into a dedicated EntityViewModeController. This is actually a necessary fix as we also need to clear the entity info if entity_ui.module is disabled. Since I was already moving the code I couldn't resist to change the type-hint from EntityInterface to EntityViewMode. This is, I think, more correct, but definitely debatable.
   • Rebuilding the menu. This is only needed for Field UI, so I moved that there. Since field_ui.module does not depend on entity_ui.module this is a bug-fix as well. I saw that this was in field_ui.module before and was moved deliberately to entity_ui.module above, but I'm pretty sure it belongs in field_ui.module.
6. Added the missing config file for the full view mode of the taxonomy_vocabulary entity type.
7. Fixed the upgrade path incorrectly setting the custom property for the node.teaser view mode to FALSE.
8. In EntityManager::processDefinition() removed the config() call inside the foreach and instead instantiated the config factory once outside of the loop.
9. Opened #1831890: Don't show "Machine name" in entity listings and added a @todo for removing the overrides to that effect from ViewModeListController.
10. Opened #1831928: Support a 'locked' property on ConfigEntities and added a @todo for removing the custom code in EntityViewMode ViewModeListController for that.
11. Change the type-hints in ViewModeListController and ViewModeFormController from EntityInterface to EntityViewMode (see also above). This is debatable, but what we do with Node, etc.
12. Added a comment why view_mode_load() violates the entity_ui namespace. And lots of other docs cleanup of that sort.

I read through the entire issue, to make sure I didn't miss anything. Things I did not resolve:

• @yched in #44:
  

  - Just like custom fields created in the UI have a "field_" prefix, machine names of custom view modes need a prefix ('custom_' or smthg) to avoid clashes with module-provided view modes

  I personally have always hated the field_ prefix, and I see no particular reason to add anything similar here. We don't do it for node types, vocabularies, etc... either. Anyone else have thoughts on this?

• @andypost in #45:
  

  +++ b/core/modules/entity_ui/lib/Drupal/entity_ui/ViewModeFormController.phpundefined
  @@ -0,0 +1,88 @@
  +    $destination = array();
  +    if (isset($_GET['destination'])) {
  +      $destination = drupal_get_destination();
  +      unset($_GET['destination']);
  +    }
  +    $view_mode = $this->getEntity($form_state);
  +    $form_state['redirect'] = array('admin/config/system/view-modes/' . $view_mode->id() . '/delete', array('query' => $destination));
  

  $destination could be empty

  I don't know what this comment means and I must say I don't understand the code 100% either. Specifically I don't understand why we check $_GET['destination'] when drupal_get_destination() does that for us. I just left that code as is for now.

• @Crell in #48:
  

  +++ b/core/modules/entity_ui/lib/Drupal/entity_ui/Tests/ViewModeTest.php
  @@ -0,0 +1,92 @@
  +class ViewModeTest extends WebTestBase {
  

  Insert usual rant about unit vs. web tests here. :-)

  I don't really see how we can use unit tests here. We are testing the UI and even if were testing just the API we need the Config system to work to test anything useful. I'm not sure if we can use the new DrupalUnitTestBase for that, but since we already have tests that show this works I would like to investigate that in a follow-up. Opened #1831910: Improve test coverage for EntityViewMode and entity_ui.module for that.

• @Bojhan in #74:
  

  I am not entirely convinced that "view modes" is the correct label, although this might be a can of worms because of legacy. But I always saw these things as "display modes", since they are dealing with the display of things. We tend to use the word "display" to mean what you are seeing at least in Drupal UI, the word "view" is largely a code label. I also think there might be some confusion with Views?

  I agree that that might be a worthwhile to discuss, but 'view mode' is already a pretty established term, at least for developers. So changing it would be a pretty invasive change. You proposed "display mode", but that conflicts with #1817044: Implement Display, a type of config for use by layouts, et. all. I opened #1831908: Discuss whether 'view mode' is a good name so we can discuss that in detail.

• @yched in #103:
  

  I'm still very doubtful of the overall "move all to config land" approach, but I'll try to summarize in a later post.

  Would love to hear more on that. :-)

• @sun in #110:
  

  allowing the Standard installation profile to enable Field UI module without enabling Entity UI module. Thus, we expose all the glory of fields, but leave the more advanced entity type/display/view-mode configuration out of the starter kit and initial Drupal experience (i.e., not enabled by default)

  I personally disagree with that, so I would like to hear some more thoughts on that. There are also a bunch of more fundamental points in that post that I'd love to hear more about. :-)

• @yoroy in #139:
  

  - I was surprised to see the listing/add/edit of view modes tucked away in an admin/configuration sub page. Thinking about them in the context of content types, why not allow to add them in-context?

  I opened #1831958: Allow to add view modes from Field UI's Manage Display page so we can discuss that in detail as it seems we can discuss that separately from the main UI.

Something that came to mind in working on this, but I did not want to decide on my own: How about naming EntityViewMode (back) to ViewMode?!

Next stop is addressing some of the UI concerns brought up by Bojhan. I want to tackle this in the coming days, but am unassigning this for now. I've spent quite some time on this now and I don't want to hold anything up in case I don't get to this again in a couple of days. Considering how long it took me to get the patch to this point, if anyone else wants to pick this up, feel free.

Regarding the camel-cased config keys, I opened #1832630: [policy, then patch] Decide on coding standard for configuration keys of ConfigEntities. As noted there we already have an inconsistency in core regarding this. Had I known this before I hadn't changed everything, but now since I already did, I'll keep it this way. This patch just cleans up some things I missed before. If someone wants it changed back, though, I'll do that too, it's not that many files.

Somehow PHP doesn't like overriding methods with differently typed arguments, even if those are compatible (i.e. EntityViewMode implements EntityInterface). Don't know why I missed that locally. Anyway rolled that part back.

I *think* this should be green, but given my history in this issue that's not saying much...

Somehow PHP doesn't like overriding methods with differently typed arguments, even if those are compatible (i.e. EntityViewMode implements EntityInterface). Don't know why I missed that locally. Anyway rolled that part back.

When overriding a method, the types of the arguments must be the same or less specific than the method you're overriding. So, your overridden method can accept more things, but not less things. EntityViewMode is more specific / encompasses less things than EntityInterface. Conversely, the return type must be the same or more specific than the method you're overriding. Only then does your subclass adhere to the 'contract' set forth by its parent class.

That's why PHP doesn't like what you were trying to do.

add remaining task

Updates issue summary with the todos so we/I don't lose track of anything.

Quick weigh in on UX issues:

• Decide whether view modes should have custom_ (or similar) prefix similar to Field UI: Yay, but something more specific than custom_, like viewmode_.
• Decide whether to enable entity_ui.module in Standard profile: Yay. Ask yourself would user #1 want it enabled? It's a key site building tool, so I think yes.
• Decide whether there should be a separate table for each view mode: I'm in favor of the tables by type, at the expense of defining view modes with the same name for multiple entity types at once. It more closely models what's actually going on under the hood, and as someone who uses the Entity View Mode module, my experiences with that ui have been pleasant and efficient. You can use the same field on multiple entity types, but we don't have an overview page that allows you to add the a single field to multiple entity types at once. I know it's not exactly the same thing, but I think the pattern of add a smaller thing to each larger thing one at a time has been working well across Drupal for quite a while.

Updated issue summary, specifically todos

Here's a WIP patch for the one table approach, with some screens attached.
I must say I don't really like it at all. The screenshot shows the default list of view modes before adding any, and it is quite overwhelming I must say. Imagine adding a bunch more and things really get out of hand. I would like it if we could find a way to create a "per-entity-type view modes" overview somewhere "near" the Manage Display UI by Field UI. I would find that much more intuitive.

If we do want to go with this approach there are a bunch of todos, partly marked as such in-code. I also couldn't be bothered to update the tests for this, which is why I'm attaching this with the -do-not-test suffix. It's mostly for people who want to try this out live or want to build on this themselves.

In case someone wants to cook something up and start in a different direction UI-wise they should start with the patch in #147, but don't forget to increase the update version number (the very last hunk in the interdiff here).

Added RobW to the vote counts

I like the previous "separate tables" approach better too.

View modes are really per entity type. One view mode cannot exist for several entity types, it's just different view modes that possibly happen to have the same name, but this has no actual meaning. So munging entity types together makes no real sense IMO.

Agreed that the most natural shape would be one page per entity type, listing all view modes for that entity type, "close" (IA-wise) to the Field UI pages where you configure their actual behavior.
But, precisely, those Field UI pages are per bundle, we don't really have a unified notion of "a page that talks about the entity type as a whole" across all entity types. I guess
- admin/structure/types (for nodes)
- admin/structure/taxonomy (for taxo terms)
would be the closest we have, but there's no predictable pattern for those across entity types (and actually no guarantee that all entity types have such a page).
And even in that case, the "administer view modes on this entity type" page would be out of the flow of the "configure actual entity display" pages, so there's little real gain.

I guess best we can do is put a link to the "administer view modes" page on each of Field UI's "Manage display for view mode foo" pages... That can be fine-tuned during slush, though.

Also, I wouldn't worry too much about what to do with entity_ui.module. As catch mentioned earlier, field_ui would more be accurately named entity_ui anyway, so those pages would currently end up in the same module than current field_ui.
As far as I'm concerned the current UI code in the patch could totally go in field_ui already, but doing the merge in a followup is probably safer. Meanwhile, we should probably not lose hair on this.

Thanks for exploring this UI direction. But yeah, agreed that multiple tables is better than the latest single table approach.

I'd much prefer to use js states to show the title field *after* an entity type is chosen instead of immediately showing a disabled text field.

Also agreed about the not losing hair about this part.

Minor update to bump the update version number. This also includes some minor docs improvements.

Today I thought how this relates to the code freeze. I personally think this would qualify as clean-up in the slush, but maybe we should have some confirmation on that. Since this is now mostly stuck on figuring out the UI we want, I don't think we're very far off, but also it would be trivial to roll a no-UI patch here. (Just take the patch file and delete all hunks in entity_ui module.) Would love to hear some thoughts on that, as I would find it very sad to not see this in D8.

I'll explore another UI direction in the upcoming patch (the one I referenced in #151).

Here's some screenshots. If someone wants a patch I can upload that too, but it's very much more WIP that the last, so I will only upload it on request. :-)

I tried splitting out the per-entity-type tables out on separate tables, specifically I did this for content types, i.e. the node entity type. I added a local task to the content type listing page. I actually like it more than I had originally thought and I would personally favour this version, as conceptually view modes are nearer to where they are bound to (a specific entity type) and where they are being used (Field API). It also allows to avoid the whole "entity type" concept in the UI as well, exactly because of that.

If this isn't it though, then we are back to a per-table approach on one page, I think, and I will post a refined version of that (with #153 incorporated for instance). I have absolutely no problem with that.

#155 makes a lot more sense to me.

UX-wise, in the earlier approaches, I was primarily offended by the huge page consisting of multiple, larger tables that appear in no particular order, and each table presenting its own, discrete interface — even only imagining to have to visit such a page would throw me off as a user. ;)

The add/edit interface for a single view mode looks a bit lost and empty, and could be improved, but I'd be fine with #155 for now, and improving the experience as a follow-up issue.

The add/edit interface for a single view mode looks a bit lost and empty, and could be improved, but I'd be fine with #155 for now, and improving the experience as a follow-up issue.

How about #1667742: Add abstracted dialog to core (resolves accessibility bug) and more specifically http://www.youtube.com/watch?v=VySgJjlkutU&feature=youtu.be (YouTube video demonstrating the latest patch there)...
(It's awesome enough that I couldn't resist posting that here, even though you guys are probably aware of it, and it's a whole other discussion... :-))

I much prefer the "giant table page" to the entity type tab pattern.

When I'm setting up view modes I'm often configuring similarly named view modes for multiple entities at once. This workflow is so common that display suite and the first version of this patch's ui center almost entirely around it. So when building a site, first I might set up node types, then fields, then maybe field collections, then view modes, then field, collection, and file displays. I don't want that fourth step split up over multiple screens in multiple sections of the site (edit node types, edit file types, edit taxonomy types), or to have to have multiple pages open to compare my available view modes on each entity.

Sometimes a bunch of big tables are the correct ui for a task. I don't think we're about to split permissions across every entity type, module, and configuration option that they apply to (egh I hope I didn't shoot myself in the foot with that example).

Both could be implemented without too much code duplication maybe?

In #152 I wrote why I think "one separate 'administer view modes' page per entity type" is going to be tricky IMO.

Agreed that the most natural shape would be one page per entity type, listing all view modes for that entity type, "close" (IA-wise) to the Field UI pages where you configure their actual behavior.
But, precisely, those Field UI pages are per bundle, we don't really have a unified notion of "a page that talks about the entity type as a whole" across all entity types. I guess
- admin/structure/types (for nodes)
- admin/structure/taxonomy (for taxo terms)
would be the closest we have, but there's no predictable pattern for those across entity types (and actually no guarantee that all entity types have such a page).
And even in that case, the "administer view modes on this entity type" page would be out of the flow of the "configure actual entity display" pages, so there's little real gain.

Unless tstoeckler comes up with a nice solution for this, I'd say our safest bet is to keep the "one page with different tables" approach for the UI, and possibly consider how we can refine after the patch gets in.

So, UI aside, my main issue with the patch is the direction that was taken to move all view modes to config - for which I could find no real justification other than "this sounded a good idea" in #52.

As I pointed earlier, and as @sun points in #110, references to view modes are hardcoded in module code. A module does an entity_view($entity, 'teaser') somewhere (a block, a page callback, whatever...). It can do so because it knows 'teaser' is a view mode known by the system. Thus, view modes are not config, and in D7 they are exposed in code.
The actual output of a given view mode, OTOH, is entirely config. But the list of available view modes is the hardcoded entry point for the entity rendering system, that's the "stable ground" for modules to be able to render entities.

Now, in *some* cases (a.k.a views, or "reference field" formatter...), what is done is entity_view($entity, $view_mode) where $view_mode is user-configurable (e.g. a "select view mode" drop down in Views UI). This is the main reason why being able to create custom view modes from the UI is a cool feature to offer - add your own "node display variants", and use them in places that let you pick the view mode to display.
If you want to use extra view modes in your custom code, the current way of working (expose your view mode in code) works fine.

So the goal was to move from "all view modes are exposed in code" to "in addition, some of them can be defined by config".
The current patch goes "all view modes are config, and the ones that module code need to rely on have can have a 'locked' flag that make them not configurable". That sounds... wrong ?

A bit like with the discussions about "router paths being defined in config", I'm a little worried about the trend to put that much stuff in config-land...

re @tstoeckler #145 - 1st of all thanks for the great job !

Renamed EntityViewMode::machine_name to EntityViewMode::machineName for consistency with our OOP coding standards. What is strange is that I had to also update the corresponding keys in the YAML config files, where I would have usually used lower_case_with_underscores. I don't know what our standards for that are, but it seems this affects all ConfigEntities. Thoughts

Good question - I'd tend to think we don't want camelCase properties in our config files, but that's probably a community-level discussion. I opened #1839956: camelCase for top-level entries in ConfigEntities CMI files ?.
For now, safest bet is probably to stick to the current coding standards - thus, camelCase properties.

(yched): Just like custom fields created in the UI have a "field_" prefix, machine names of custom view modes need a prefix ('custom_' or smthg) to avoid clashes with module-provided view modes

(tstoeckler): I personally have always hated the field_ prefix, and I see no particular reason to add anything similar here. We don't do it for node types, vocabularies, etc... either. Anyone else have thoughts on this?

Well, you may hate it ;-), but it does its job of preventing conflicts when you enable a new module one year from now... Also, it's only a machine name, I'm not sure what's the harm.

How about naming EntityViewMode (back) to ViewMode?!

I'd +1 this.

(in EntityManager::processDefinition(&$definition, $plugin_id) {
+    // Add view modes.
+    // We cannot use entity_load_multiple() as that leads to recursion.
+    $view_modes = drupal_container()->get('config.storage')->listAll('view_mode.');
+    $config_factory = drupal_container()->get('config.factory');
+    foreach ($view_modes as $config_name) {
+      $view_mode = $config_factory->get($config_name)->load()->get();
+      if ($view_mode['targetEntityType'] == $plugin_id) {
+        $entity_type = strtok($view_mode['machineName'], '.');
+        $definition['view_modes'][strtok('.')] = array(
+          'label' => check_plain($view_mode['name']),
+          'custom_settings' => $view_mode['custom'],
+        );
+      }
+    }

Yeah, we can't load view modes with the regular configEntity API because of recursion, you need entity type info in order to build entity type info... - and that stands whatever the approach of "all config" or "code based + config". We really need to fix #1822458: Move dynamic parts (view modes, bundles) out of entity_info() and move view modes out of entity_info.

Current code does the trick, but could we add a @todo with a link to that issue ?
Also, it looks like it could be improved a bit, currently we parse all view_mode.* files all over again for each entity type. Couldn't we look for view_mode.[entity_type].* specifically ?

Also, I thought CMI files were supposed to be prefixed by the owning module - which would mean system in our case.
Bleh, entity.view_mode.node.full.yml would be much better than system.view_mode.node.full.yml, but well, we don't have an entity.module...

A bit similar UI pattern used for translation #1810386-34: Create workflow to setup multilingual for entity types, bundles and fields

This issue was too ambitious.
We can work on a UI in another issue, but almost everyone has agreed on this code.

The issue summary needs updating, and a follow-up should be created.

I was about to suggest to drop the UI part from this issue for the sake of progress. I opened a stub for the UI here: #1847276: Add a UI for managing custom view modes that needs work.

Fix failures, I'm slowly starting to hate the rest tests tbh.

Thanks for scaling down the scope of this issue and patch. That really helps for reviewing the actual, low-level change proposal.

+++ b/core/lib/Drupal/Core/Plugin/Core/Entity/EntityViewMode.php
@@ -0,0 +1,101 @@
+ *   module = "Core",
...
+ *   config_prefix = "view_mode",

Hm.

This is a very interesting, and also a bit scary aspect — this seems to be the first config entity that is provided by a Drupal component.

That has a range of non-trivial implications:

1) It's not clear to me who is responsible for maintaining these config objects. E.g., when Node module is uninstalled, then all of the view modes for its entity types should be deleted.

2) Likewise, the first part up until to the first dot in a config object name denotes the name of the owning extension. This essentially means that the Entity component takes over the 'view_mode' module namespace, and I'm not sure whether that is legit.

3) The config import process relies on the module/hook system to invoke callbacks in the owning module of a configuration object. The Entity component is not able to participate in hooks, since it is not a module.

All of these issues are really not easy to address, and unfortunately, I don't have answers either.

+++ b/core/lib/Drupal/Core/Plugin/Core/Entity/EntityViewMode.php
@@ -0,0 +1,101 @@
+ *   entity_keys = {
+ *     "id" = "machineName",
+ *     "label" = "name",

Can we use the regular entity properties/keys $id and $label?

+++ b/core/lib/Drupal/Core/Plugin/Core/Entity/EntityViewMode.php
@@ -0,0 +1,101 @@
+  /**
+   * Whether or not this view mode has custom settings by default.
+   *
+   * Field UI allows to override this setting per bundle.
+   *
+   * @var bool
+   */
+  public $custom = FALSE;

+++ b/core/modules/comment/config/view_mode.comment.full.yml
@@ -0,0 +1,5 @@
+custom: '0'

I would have never assumed that $custom would stand for "has custom settings" — almost everywhere else, $custom is a differentiation between things defined by modules/code and things that have been created or customized by the site administrator.

Can we rename this to $hasSettings or something else that is more self-descriptive?

Like this ?

That has a range of non-trivial implications

The three points you make are valid, but are about non-module provided config entities, which is bigger than this issue.

Everything else in #169 is addressed. Thanks @swentel!

The three points you make are valid, but are about non-module provided config entities

...and this is the exact case here:

+++ b/core/lib/Drupal/Core/Entity/ViewMode/EntityViewModeStorageController.php

+ * Contains \Drupal\Core\Entity\ViewMode\EntityViewModeStorageController.

+++ b/core/lib/Drupal/Core/Plugin/Core/Entity/EntityViewMode.php

+ * Contains Drupal\Core\Plugin\Core\Entity\EntityViewMode.

This is an entity type that is provided by a PHP component, not by a Drupal extension.

Thus, all three points still stand. I thought about it, but unfortunately, I still don't have answers.

The seemingly simplest way would be to introduce a view_mode.module as a required module... but well, required modules generally are an artifact of a poor architecture, and this particular module would involve quite some circular dependencies.

Why couldn't we have an entity.module, again ?
There will be more ConfigEntities that would be best owned by 'entity'...

Looks good to me.

Feature request we really need descriptions for view modes. It helps to have notes about why a view mode exists. An
example is the new compact mode on the user entity. Thats for user pictures on nodes/comments.

So the issue is blocked on @sun's #169 - "who's the owner for the newly added config files ?"

This patch is definitely not the only one that needs to introduce config that should be owned by the entity system. Can we discuss this in #1857074: Re-introduce entity.module as a configuration owner ?
And maybe agree on a short term approach to unblock this patch here ?

To summarize the three points once more:

1) The view mode config objects are not maintained in relation to the modules and entity types they correspond to. E.g., if Node module is disabled or uninstalled, then all of its view modes continue to exist - re-installing Node module from scratch will show configuration that should not exist anymore. This could potentially be resolved by adding custom, entity-system-specific code to module_enable() and module_uninstall().

2) The Entity component takes over the 'view_mode' module namespace, which appears a bit arbitrary. 'entity.view_mode' would be more appropriate.

3) None of the config objects can be staged and imported in a proper way, because there is no corresponding owning extension for them. This could potentially be resolved by adding custom, entity-system-specific code to config_import_invoke_owner().

The last point is the most concerning and troublesome from my perspective.

The first point is a good and huge architectural question of its own, since, off-hand, I do not necessarily understand why we'd need and ask for view modes independently of the module that defines the entity type — i.e., why don't we ask the entity system to hand us the view modes that are defined for the XYZ entity type? Which in turn would mean we'd actually ask the entity type to return its view modes. And which in turn would mean that view modes would actually live in config objects à la node.view_mode.full?

Taking a step back, what we're essentially facing here is the attempt to introduce functionality that manages configuration for entities (in the pure generic entity-sense) of 0-N other modules. By design, such an approach inherently involves a lot of relationships and dependencies. Therefore, the very first question that should be addressed is why those modules are not managing and maintaining their configuration on their own? Typically, we'd resolve this by adding a EntityViewModeController to the entity [type] system, which turns the view mode stuff into a service that entity types can consume (and also override), and they are fully responsible to manage and maintain them.

In case there's a solid answer to that and I missed it, then it appears to be more reliable and sensible to move this code into a new, required entity.module, which would inherently solve all three points. If we find a way to allow components to participate in the higher-level business logic later on, then we can still attempt to move this code "back" into the Entity component. The corresponding, optional UI could still emerge in a corresponding, separate entity_ui.module.