Problem/Motivation

View modes are one of the single most useful site building tools that matured during the D7 cycle. The fact that they are never exposed to site builders is a huge disservice to them. Let's add a UI for managing them.
In addition, view modes are tacked onto hook_entity_info(), they are a good candidate for config files.

Proposed resolution

  • Convert view modes to configuration entities
  • Provide a UI for editing them

Remaining tasks

Code-wise

  • Figure out how to do the destination handling in ViewModeFormController::delete(). @see @andypost in #45.
  • Improve test coverage or convince @Crell (and @webchick, I fear :-)) that that can be handled in #1831910: Improve test coverage for EntityViewMode and entity_ui.module. @see @Crell in #48.
  • Find out why @yched doesn't want to use config for this. @see @yched in #103.
  • Decide whether to (re-)rename EntityViewMode to ViewMode. @see @tstoeckler in #145. Votes: Yay 1 (tstoeckler), Nay 0

UX-wise

  • Decide whether view modes should have custom_ (or similar) prefix similar to Field UI's field_ prefix. @see @yched in #44. Votes: Yay 2 (yched, RobW), Nay 1 (tstoeckler)
  • Decide whether to enable entity_ui.module in Standard profile. @see @sun in #110. Votes: Yay 3 (tim.plunkett (Right?!), tstoeckler, RobW), Nay 1 (sun)
  • Decide whether there should be a separate table for each view mode. @see @sun in #110. Votest: Yay 1 (sun), Nay 2 (RobW, tstoeckler).

User interface changes

A new UI for managing view modes, modeled after Entity view modes

API changes

hook_entity_info() should no longer be used to provide view modes, they should be shipped as config.

Original report by femrich

With Views being an official initiative and also the desire to get an Entity reference field into core, a UI in core to manage custom view modes would benefit those two modules.

D7 field ui uses a field display system that defines a few categories of custom display (teaser, rss, etc).

A commenter asks whether it is possible to create custom categories for additional displays (such as teaser 2) (http://drupal.org/node/774798#comment-3678538) and receives the following response (http://drupal.org/node/774798#comment-4002608):

***Begin quote***

You can add extra display modes using hook_entity_info_alter.

Say you have a custom module 'myModule' and want to add a custom view to the node entity:

<?php
function myModule_entity_info_alter(&$entity_info) {
    $entity_info['node']['view modes']['mymode'] = array('label' => 'My Custom Mode', 'custom settings' => 'TRUE');
}
?>

Flush caches and this should show up as a customisable view mode on the 'manage display' page of any node content type.

***end quote***

This seems like a very basic task. Would it be possible please to build this into UI in such a way that coding is not required?

Thanks.

Files: 
CommentFileSizeAuthor
#215 view-mode-1043198-215.patch30.13 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,855 pass(es). View
#215 interdiff.txt8.38 KBtim.plunkett
#211 view-modes-1043198-211.patch29.88 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,986 pass(es). View
#211 interdiff.txt17.66 KBtim.plunkett
#208 view-modes-1043198-208.patch22.29 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,925 pass(es), 1 fail(s), and 0 exception(s). View
#208 interdiff.txt1.29 KBtim.plunkett
#194 view-mode-1043198-194.patch22.29 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,483 pass(es), 1 fail(s), and 0 exception(s). View
#191 view-mode-1043198-191.patch21.88 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,540 pass(es), 1 fail(s), and 0 exception(s). View
#187 viewmode-1043198-187.patch21.97 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,286 pass(es). View
#187 interdiff.txt1.14 KBtim.plunkett
#185 view-mode-1043198-185.patch21.58 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,263 pass(es), 1 fail(s), and 0 exception(s). View
#172 1043198-172.patch20.4 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 48,805 pass(es). View
#172 interdiff.txt1.59 KBswentel
#170 1043198-170.patch20.4 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 49,020 pass(es), 0 fail(s), and 2,324 exception(s). View
#170 interdiff.txt7.53 KBswentel
#168 1043198-168.patch20.67 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 48,596 pass(es). View
#168 interdiff.txt744 bytesswentel
#165 views-modes-1043198-165.patch19.94 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,510 pass(es), 0 fail(s), and 2 exception(s). View
#155 view-mode-separate-content-types.png32.13 KBtstoeckler
#155 view-mode-separate-view-modes.png23.94 KBtstoeckler
#155 view-mode-separate-add.png18.17 KBtstoeckler
#155 view-mode-separate-edit.png19.82 KBtstoeckler
#154 1043198-154-view-mode-ui.patch40.15 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 47,958 pass(es). View
#154 interdiff-147-154.txt2.05 KBtstoeckler
#151 1043198-151-view-mode-ui-do-not-test.patch39.05 KBtstoeckler
#151 interdiff-147-151.txt10.62 KBtstoeckler
#151 view-mode-list.png48.23 KBtstoeckler
#151 add-view-mode-1.png23.42 KBtstoeckler
#151 add-view-mode-2.png24.3 KBtstoeckler
#151 add-view-mode-3.png26.28 KBtstoeckler
#147 1043198-147-view-mode-ui.patch39.76 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 47,688 pass(es). View
#147 interdiff-145-147.txt7.63 KBtstoeckler
#145 1043198-145-view-modes-ui.patch39.76 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 47,957 pass(es), 10 fail(s), and 2,627 exception(s). View
#145 interdiff-144-145.txt20.58 KBtstoeckler
#144 1043198-144-view-modes-ui.patch36.09 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 47,746 pass(es). View
#144 interdiff-141-144.txt2.62 KBtstoeckler
#141 1043198-141-view-modes-ui.patch35.91 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 46,539 pass(es), 5 fail(s), and 0 exception(s). View
#141 interdiff-129-141.txt782 byteststoeckler
#139 addnewviewmode.jpg44.4 KByoroy
#129 1043198-129-re-roll.patch39.92 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 46,534 pass(es), 20 fail(s), and 73 exception(s). View
#129 interdiff-125-129.txt3.93 KBtstoeckler
#125 1043198-125-re-roll.patch43.49 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#125 interdiff-93-125.txt30.94 KBtstoeckler
#93 1043198-93.patch34.17 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1043198-93.patch. Unable to apply patch. See the log in the details link for more information. View
#93 interdiff.txt2.58 KBswentel
#93 Screen Shot 2012-10-22 at 22.35.02.png54.68 KBswentel
#91 interdiff.txt7.21 KBtim.plunkett
#90 Screen Shot 2012-10-22 at 19.37.45.png53.67 KBswentel
#90 1043198-90-tests-only.patch32.43 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 42,908 pass(es), 4 fail(s), and 0 exception(s). View
#90 1043198-90.patch34.01 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 42,915 pass(es). View
#89 Screen Shot 2012-10-21 at 22.53.25.png22.46 KBswentel
#88 view_mode-1043198-88.patch30.08 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 42,895 pass(es). View
#83 view_mode-1043198-82.patch31.71 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 42,905 pass(es), 0 fail(s), and 4 exception(s). View
#83 interdiff.txt16.98 KBtim.plunkett
#83 Screen Shot 2012-10-19 at 4.46.07 PM.png55.28 KBtim.plunkett
#83 Screen Shot 2012-10-19 at 4.45.51 PM.png123.28 KBtim.plunkett
#74 system-view-modes.png11.37 KBBojhan
#74 view-mode-description.png16.04 KBBojhan
#74 entity_type.png1.5 KBBojhan
#74 fieldset-grouping-tables.png18.22 KBBojhan
#74 Add-view-mode.png25.44 KBBojhan
#71 view-mode-list.png129.28 KBtim.plunkett
#71 view-mode-edit.png82.79 KBtim.plunkett
#68 view_mode-1043198-68.patch34.36 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 42,906 pass(es). View
#68 interdiff.txt800 bytestim.plunkett
#65 view_mode-1043198-65.patch34.33 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 42,909 pass(es), 2 fail(s), and 0 exception(s). View
#65 interdiff.txt925 bytestim.plunkett
#62 view_mode-1043198-62.patch33.77 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 42,761 pass(es). View
#62 interdiff.txt1.22 KBtim.plunkett
#60 view_mode-1043198-60.patch33.8 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 42,686 pass(es), 0 fail(s), and 38 exception(s). View
#60 interdiff.txt10.53 KBtim.plunkett
#57 view_mode-1043198-56.patch34.06 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 42,680 pass(es). View
#57 interdiff.txt576 bytestim.plunkett
#53 view_mode-1043198-53.patch34.02 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 42,694 pass(es), 0 fail(s), and 4 exception(s). View
#53 interdiff.txt3.18 KBtim.plunkett
#52 view_mode-1043198-50.patch31.16 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 42,676 pass(es), 0 fail(s), and 4 exception(s). View
#52 interdiff.txt6.44 KBtim.plunkett
#46 view_modes-1043198-46.patch29.96 KBtim.plunkett
PASSED: . View
#46 interdiff.txt22.6 KBtim.plunkett
#46 view-modes-ui.png89.51 KBtim.plunkett
#40 evm-management.jpg167.37 KBRobW
#40 evm-edit.jpg60.31 KBRobW
#36 view-modes-links.png86.8 KBtim.plunkett
#36 view-modes-operations.png84.94 KBtim.plunkett
#36 view-modes-edit.png70 KBtim.plunkett
#34 view-modes-1043198-34.patch18.55 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 42,166 pass(es). View
#34 interdiff.txt4.25 KBtim.plunkett
#33 view-modes-1043198-33.patch15.58 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 42,162 pass(es). View
#33 interdiff.txt15.33 KBtim.plunkett
#29 view-modes-1043198.28.patch13.48 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 42,162 pass(es). View
#29 view-modes-1043198.28.interdiff.txt2.66 KBlarowlan
#25 view-modes-1043198-25.patch13.36 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 42,151 pass(es), 16 fail(s), and 0 exception(s). View
#25 interdiff.txt15.17 KBtim.plunkett
#23 view_mode-1043198-23.patch13.31 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 42,164 pass(es). View
#23 interdiff.txt7.64 KBtim.plunkett
#15 1043198-15.patch13.34 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 42,183 pass(es). View

Comments

yched’s picture

Version: 7.0 » 8.x-dev

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.

femrich’s picture

Thanks! Will look into display suite...

Stalski’s picture

Assigned: Unassigned » Stalski

I'll be working on this asap.

yched’s picture

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.

Stalski’s picture

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.

yched’s picture

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.

swentel’s picture

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

swentel’s picture

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.

catch’s picture

Component: field system » entity system

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

andypost’s picture

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

swentel’s picture

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

swentel’s picture

Title: Add extra field display modes without coding » Add a custom view modes UI to core

Better title ? Feel free to correct.

Bojhan’s picture

So I am following this :D But why?

yched’s picture

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.

swentel’s picture

Status: Active » Needs review
FileSize
13.34 KB
PASSED: [[SimpleTest]]: [MySQL] 42,183 pass(es). View

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)

larowlan’s picture

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?

swentel’s picture

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.

tim.plunkett’s picture

Assigned: Stalski » tim.plunkett

Working on this.

cosmicdreams’s picture

Assigned: tim.plunkett » Stalski

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?

tim.plunkett’s picture

Assigned: Stalski » tim.plunkett

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.

andypost’s picture

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?

tim.plunkett’s picture

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.

tim.plunkett’s picture

FileSize
7.64 KB
13.31 KB
PASSED: [[SimpleTest]]: [MySQL] 42,164 pass(es). View

Whoops.

swentel’s picture

Issue tags: -Configuration system

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.

tim.plunkett’s picture

FileSize
15.17 KB
13.36 KB
FAILED: [[SimpleTest]]: [MySQL] 42,151 pass(es), 16 fail(s), and 0 exception(s). View

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.

swentel’s picture

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.

andypost’s picture

Status: Needs review » Needs work
+++ 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

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.66 KB
13.48 KB
PASSED: [[SimpleTest]]: [MySQL] 42,162 pass(es). View

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

larowlan’s picture

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

catch’s picture

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

andypost’s picture

@catch++ Awesome idea!

tim.plunkett’s picture

FileSize
15.33 KB
15.58 KB
PASSED: [[SimpleTest]]: [MySQL] 42,162 pass(es). View

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.

tim.plunkett’s picture

Issue tags: +Entity system, +Configuration system
FileSize
4.25 KB
18.55 KB
PASSED: [[SimpleTest]]: [MySQL] 42,166 pass(es). View

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.

Bojhan’s picture

Can this have a screen.

tim.plunkett’s picture

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:
view-modes-links.png

And here is is in conjunction with the other issue:
view-modes-operations.png

This is the edit form:
view-modes-edit.png

Bojhan’s picture

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.

andypost’s picture

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

tim.plunkett’s picture

  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.
RobW’s picture

FileSize
60.31 KB
167.37 KB

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.
    evm management
    For D8 the type column could be cut out.

    Edit page. Short and to the point.
    evm edit

  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.

andypost’s picture

@RobW +++

swentel’s picture

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

tstoeckler’s picture

Status: Needs review » Needs work
+++ 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.

yched’s picture

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

andypost’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
89.51 KB
22.6 KB
29.96 KB
PASSED: . View

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.
view-modes-ui.png

Crell’s picture

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

tim.plunkett’s picture

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

yched’s picture

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 ?

tstoeckler’s picture

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.

andypost’s picture

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)

tim.plunkett’s picture

FileSize
6.44 KB
31.16 KB
FAILED: [[SimpleTest]]: [MySQL] 42,676 pass(es), 0 fail(s), and 4 exception(s). View

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.

tim.plunkett’s picture

FileSize
3.18 KB
34.02 KB
FAILED: [[SimpleTest]]: [MySQL] 42,694 pass(es), 0 fail(s), and 4 exception(s). View

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.

tim.plunkett’s picture

Title: Add a custom view modes UI to core » Convert view modes to ConfigEntity and add a UI
andypost’s picture

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

tim.plunkett’s picture

FileSize
576 bytes
34.06 KB
PASSED: [[SimpleTest]]: [MySQL] 42,680 pass(es). View

Fixed bug.

tim.plunkett’s picture

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.

fago’s picture

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.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
FileSize
10.53 KB
33.8 KB
FAILED: [[SimpleTest]]: [MySQL] 42,686 pass(es), 0 fail(s), and 38 exception(s). View

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.

tim.plunkett’s picture

FileSize
1.22 KB
33.77 KB
PASSED: [[SimpleTest]]: [MySQL] 42,761 pass(es). View

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

tim.plunkett’s picture

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

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

Issue summary: View changes

add html

podarok’s picture

#63 +1 for that

tim.plunkett’s picture

FileSize
925 bytes
34.33 KB
FAILED: [[SimpleTest]]: [MySQL] 42,909 pass(es), 2 fail(s), and 0 exception(s). View

Okay, added a hook_help().

tstoeckler’s picture

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

tim.plunkett’s picture

FileSize
800 bytes
34.36 KB
PASSED: [[SimpleTest]]: [MySQL] 42,906 pass(es). View

Wow, hook_help fail.

podarok’s picture

Status: Needs review » Reviewed & tested by the community

#68 looks good for me

Bojhan’s picture

Status: Reviewed & tested by the community » Needs review

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?

tim.plunkett’s picture

FileSize
82.79 KB
129.28 KB

Here are updated screenshots.

List:
view-mode-list.png

Edit:
view-mode-edit.png

zuuperman’s picture

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

tim.plunkett’s picture

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

Bojhan’s picture

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?

system-view-modes.png

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

view-mode-description.png

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

entity_type.png

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.

fieldset-grouping-tables.png

  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.

Add-view-mode.png

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

swentel’s picture

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

tim.plunkett’s picture

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

RobW’s picture

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

Bojhan’s picture

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

tim.plunkett’s picture

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.

RobW’s picture

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.

Bojhan’s picture

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

webchick’s picture

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.

tim.plunkett’s picture

FileSize
123.28 KB
55.28 KB
16.98 KB
31.71 KB
FAILED: [[SimpleTest]]: [MySQL] 42,905 pass(es), 0 fail(s), and 4 exception(s). View

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:
Screen Shot 2012-10-19 at 4.45.51 PM.png

Edit:
Screen Shot 2012-10-19 at 4.46.07 PM.png

Bojhan’s picture

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.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work

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

RobW’s picture

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.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
30.08 KB
PASSED: [[SimpleTest]]: [MySQL] 42,895 pass(es). View

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.

swentel’s picture

Status: Needs review » Needs work
FileSize
22.46 KB

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
  5. Screen Shot 2012-10-21 at 22.53.25.png

swentel’s picture

Status: Needs work » Needs review
FileSize
34.01 KB
PASSED: [[SimpleTest]]: [MySQL] 42,915 pass(es). View
32.43 KB
FAILED: [[SimpleTest]]: [MySQL] 42,908 pass(es), 4 fail(s), and 0 exception(s). View
53.67 KB

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

Screen Shot 2012-10-22 at 19.37.45.png

tim.plunkett’s picture

FileSize
7.21 KB

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

pounard’s picture

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

swentel’s picture

FileSize
54.68 KB
2.58 KB
34.17 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1043198-93.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Screen Shot 2012-10-22 at 22.35.02.png

tim.plunkett’s picture

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

andypost’s picture

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

Bojhan’s picture

Status: Needs review » Needs work
Issue tags: +needs accessibility review

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.

Crell’s picture

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

Crell’s picture

Damnit, Drupal!

swentel’s picture

Status: Needs work » Needs review

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

swentel’s picture

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

Crell’s picture

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

yched’s picture

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 ?

tim.plunkett’s picture

Status: Needs review » Needs work

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

Crell’s picture

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.

tim.plunkett’s picture

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.

yched’s picture

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

yched’s picture

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.

tim.plunkett’s picture

+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().

sun’s picture

Status: Needs work » Needs review

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

tim.plunkett’s picture

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.

sdboyer’s picture

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

yched’s picture

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.

tstoeckler’s picture

Assigned: Unassigned » tstoeckler

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

sdboyer’s picture

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

tstoeckler’s picture

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.

tim.plunkett’s picture

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

What? I'm not sure what this means.

tstoeckler’s picture

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.

tim.plunkett’s picture

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

andypost’s picture

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

tstoeckler’s picture

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.

yched’s picture

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

xjm’s picture

tstoeckler’s picture

Issue tags: +Entity system, +needs accessibility review, +Configuration system, +Configurables
FileSize
30.94 KB
43.49 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

tstoeckler’s picture

Status: Needs review » Needs work

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

tstoeckler’s picture

FileSize
3.93 KB
39.92 KB
FAILED: [[SimpleTest]]: [MySQL] 46,534 pass(es), 20 fail(s), and 73 exception(s). View

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.

tstoeckler’s picture

Status: Needs work » Needs review

Forgot to mark needs review.

yched’s picture

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

tim.plunkett’s picture

Status: Needs review » Needs work

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

You and me both.

swentel’s picture

Status: Needs work » Needs review

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

yannickoo’s picture

Title: Convert view modes to ConfigEntity and add a UI » Convert view modes to ConfigEntity and add an UI
tstoeckler’s picture

Status: Needs review » Needs work

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.

tim.plunkett’s picture

Title: Convert view modes to ConfigEntity and add an UI » Convert view modes to ConfigEntity and add a UI

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

yoroy’s picture

FileSize
44.4 KB

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?

mockup showing expanded Advanced display settings fieldset expanded, with a text field and add-link below the checkboxes for already existing view modes

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

tstoeckler’s picture

Issue tags: +Needs usability review

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

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
782 bytes
35.91 KB
FAILED: [[SimpleTest]]: [MySQL] 46,539 pass(es), 5 fail(s), and 0 exception(s). View

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

yoroy’s picture

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.

tstoeckler’s picture

FileSize
2.62 KB
36.09 KB
PASSED: [[SimpleTest]]: [MySQL] 47,746 pass(es). View

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.

tstoeckler’s picture

Assigned: tstoeckler » Unassigned
FileSize
20.58 KB
39.76 KB
FAILED: [[SimpleTest]]: [MySQL] 47,957 pass(es), 10 fail(s), and 2,627 exception(s). View

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.

tstoeckler’s picture

FileSize
7.63 KB
39.76 KB
PASSED: [[SimpleTest]]: [MySQL] 47,688 pass(es). View

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

decafdennis’s picture

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.

decafdennis’s picture

Issue summary: View changes

add remaining task

tstoeckler’s picture

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

RobW’s picture

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.
RobW’s picture

Issue summary: View changes

Updated issue summary, specifically todos

tstoeckler’s picture

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

tstoeckler’s picture

Issue summary: View changes

Added RobW to the vote counts

yched’s picture

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.

yoroy’s picture

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.

tstoeckler’s picture

FileSize
2.05 KB
40.15 KB
PASSED: [[SimpleTest]]: [MySQL] 47,958 pass(es). View

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

tstoeckler’s picture

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.

sun’s picture

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

tstoeckler’s picture

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

RobW’s picture

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

pounard’s picture

Both could be implemented without too much code duplication maybe?

yched’s picture

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.

yched’s picture

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

yched’s picture

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.

yched’s picture

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

andypost’s picture

tim.plunkett’s picture

Title: Convert view modes to ConfigEntity and add a UI » Convert view modes to ConfigEntity
Issue tags: -Needs usability review, -needs accessibility review +Needs issue summary update
FileSize
19.94 KB
FAILED: [[SimpleTest]]: [MySQL] 48,510 pass(es), 0 fail(s), and 2 exception(s). View

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.

yoroy’s picture

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.

swentel’s picture

FileSize
744 bytes
20.67 KB
PASSED: [[SimpleTest]]: [MySQL] 48,596 pass(es). View

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

sun’s picture

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?

swentel’s picture

FileSize
7.53 KB
20.4 KB
FAILED: [[SimpleTest]]: [MySQL] 49,020 pass(es), 0 fail(s), and 2,324 exception(s). View

Like this ?

swentel’s picture

FileSize
1.59 KB
20.4 KB
PASSED: [[SimpleTest]]: [MySQL] 48,805 pass(es). View
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

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!

sun’s picture

Status: Reviewed & tested by the community » Needs review

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.

yched’s picture

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

moshe weitzman’s picture

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.

yched’s picture

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 ?

sun’s picture

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.

yched’s picture

@sun: #178 2) & 3) - that's why #1857074: Re-introduce entity.module as a configuration owner was open :-)

Regarding "view modes for node living in node.view_mode.*" more specifically - quoting myself from that issue :

Proposal 3) Move those config files to the respective [entity_type_providing_module].* namespaces.
Would mean a dynamic 'config_prefix' entry in the entity type definition. This is not supported by the config entity system.
And again, those are config for generic behavior handled by the entity system, so It's not node or user modules' hooks that should be invoked on config import,

sun’s picture

those are config for generic behavior handled by the entity system, so It's not node or user modules' hooks that should be invoked on config import

But why is that? That's the answer I'm missing.

Earlier in this and related issues, wasn't it you (and also me) who raised the concern that not all view modes are necessarily user-configurable, and that there is a need for entity types to e.g. define static view modes that are guaranteed to exist? What if the entity type providing module wants to enforce certain things? And what if these non-configurable definitions need to change over time? (e.g., in updates) Wouldn't all of these needs be resolved if the entity type owning module manages and maintains its own view modes, just like it manages and maintains every other aspect of its entity type?

Just like any other entity controller, there could simply be a default base EntityViewModeController implementation that works for most entity types and which is assigned by default. Pass a view mode config prefix into that (or even better, use a sensible default), and the entire thing turns into a service that is consumed by entity types, but most importantly, owned by each entity type.

yched’s picture

Yes, I do have my reservations on the topic of turning all view modes into configuration. I decided to put them on hold for now so as to not block the patch. And, well, *maybe* having some observable CRUD cycle for the appearance / disappearance of view modes (as opposed to "they come and go silently in an info hook") might be a good thing.

Wouldn't all of these needs be resolved if the entity type owning module manages and maintains its own view modes

Not really, because node.module wasn't the one that defined most of the node view modes in D7 to begin with. So having them owned by node doesn't really make things better regarding the actual drawbacks of putting them *all* in config land.

My point right now is : whether *all* or *some* view modes live in config makes no difference for the "owner" issue. We do know that we'd want at least some view modes to live in config - that's what this issue is about, anyway. For those that do live in config, the owning module should be 'entity', and cannot be 'node' or 'comment', because the existence of those view modes will be handled by the entity system.
(and if the patch doesn't make it in core, some module will do it in contrib for custom modes only, and they will be owned by that module, not node or comment),

Similar for #1852966: Rework entity display settings around EntityDisplay config entity and the EntityDisplay config entities. They are handled by the entity system. Node module has nothing to do with them, and can even ignore their existence.

andypost’s picture

Status: Needs review » Needs work
tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Ugh.

xjm’s picture

Category: feature » task

Why is this filed as a feature request?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
21.58 KB
FAILED: [[SimpleTest]]: [MySQL] 54,263 pass(es), 1 fail(s), and 0 exception(s). View

Since view modes were moved out of entity_info in #1822458: Move dynamic parts (view modes, bundles) out of entity_info(), this got a LOT easier.

Also since the last patch, entity.module was readded! So we can avoid that whole debate from above.

This should be all we need now.

tim.plunkett’s picture

FileSize
1.14 KB
21.97 KB
PASSED: [[SimpleTest]]: [MySQL] 54,286 pass(es). View

Forgot about the compact user view mode.

Berdir’s picture

+++ b/core/modules/block/custom_block/config/view_mode.custom_block.full.ymlundefined
@@ -0,0 +1,5 @@
+label: Full
+custom_settings: '0'
+targetEntityType: custom_block
+locked: '1'

I think we usually use something_whatever and not somethingWhatever for these keys? why mix it here?

yched’s picture

- view_mode.user.compact.yml
Config files should be prefixed with the name of an owner module - here, 'entity' ?

- Then,

+      foreach (entity_load_multiple('view_mode') as $view_mode) {
+        list($view_mode_entity_type, $view_mode_name) = explode('.', $view_mode->id());

will break, but I think we have smarter ways of dealing with config prefixes now ?

tim.plunkett’s picture

targetEntityType was taken from EntityDisplay

Also, wtf is EntityDisplay and why does it look exactly like a view mode?

I can switch the YAML names to be like them, entity.display.user.user.compact.yml and entity.view_mode.user.user.compact.yml, but wow that's confusing

tim.plunkett’s picture

FileSize
21.88 KB
FAILED: [[SimpleTest]]: [MySQL] 54,540 pass(es), 1 fail(s), and 0 exception(s). View

Fixing the config prefix.

Status: Needs review » Needs work

The last submitted patch, view-mode-1043198-191.patch, failed testing.

Berdir’s picture

EntityDisplay is the field formatter configuration for a view mode. And yes, it could make sense to combine these two things into a single config entity? @yched? Not really sure how it exactly works because it's something in entity.module but mostly contains information for field.module.

Note that EntityDisplay entities are already converted from 7.x for all view modules, I have hundreds of those on a big upgraded site.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
22.29 KB
FAILED: [[SimpleTest]]: [MySQL] 54,483 pass(es), 1 fail(s), and 0 exception(s). View

Okay, this is "done" (I was working from a stale branch, this is the *same* interdiff as #187 but with the rename).
If yched thinks we could merge this with EntityDisplay, that'd be cool.

amateescu’s picture

This cannot be merged with EntityDisplay, at least in its current form, because we don't create a EntityDisplay for each view mode. View modes that don't have a corresponding EntityDisplay config entity falls back to the 'default' one. So we still need a way to define all available view modes..

Unless we want to do it backwards, you only 'get' a view mode when you create a entity display config entity.

Status: Needs review » Needs work

The last submitted patch, view-mode-1043198-194.patch, failed testing.

Dave Reid’s picture

I was thinking it might be nice to have the UI for adding a new view mode and/or display from the 'Manage display' tab as a local action, but this means this is only available to fieldable entities.

amateescu’s picture

That's not too bad, non-fieldable entities always have the possibility to do it in code or with default config.

tim.plunkett’s picture

No, this issue already tried to provide a UI, and that's the main reason we're now at #199 comments. No UI will be added by this issue.

amateescu’s picture

Who said anything about adding that UI in this issue?

tim.plunkett’s picture

Oh I misread the issue flow, I thought it was marked needs work for that.
I'm just overly sensitive about it after the first round of UI discussion in this issue.

Dave Reid’s picture

Yeah sorry to go off-track. I was thinking things that I could possibly do for Entity view mode in contrib for D8.

yched’s picture

Clarifying view modes / EntityDisplays :
EntityDisplays are not view modes, they are "what happens for a view mode on a given bundle : this field is hidden, this field is displayed with this formatter and those settings, ...".
It's config data that was scattered in many places in D7 ($instance['display'][$view_mode] for each individual field, a variable for "extra fields", field_groups db tables for field_groups...), and just got centralized in one location.

Aside from this centralization, things haven't changed much. You have:
- a list of available view modes (which is what this patch is about, moving it from an info hook to config land),
- configuration of what happens in those view modes, bundle by bundle - that's EntityDisplays in D8

The main reason why the two are difficult to merge is:
- available view modes are per entity type, for all bundles - or else you can't say "I'll display a list of nodes, whatever their bundle, in 'teaser' mode", and you can't display a select dropdown to "pick the view mode" in cases where you haven't narrowed to a specific bundle yet - views, anyone ? :-)
- EntityDisplays are per bundle, since they are very much about the collection of fields that exist on a given entity.

tim.plunkett’s picture

So, EntityDisplay is what results when you have the "custom settings" checked for a given view mode.
And view modes are per entity type.

Can't we just enforce having an empty EntityDisplay for every bundle that doesn't have the custom settings, and kill off view modes altogether?

yched’s picture

So, EntityDisplay is what results when you have the "custom settings" checked for a given view mode.

Sort of, but not exactly.
The "custom settings" checkbox just routes node_view($node, $view_mode) to the EntityDisplay to be used : either entity.display.node.[bundle].$view_mode, or entity.display.node.[bundle].default.

But you can always have an EntityDisplay config entry for node.article.rss even though the "custom settings" checkbox for 'rss' in article nodes is unchecked:
- some code programmatically created it, or it was added as part of a module's default config. It then acts as "suggested display settings for article nodes in rss", but are not used until the admin explicitly says he wants "rss on article nodes to be non-default".
- the "custom settings" checkbox was checked, then unchecked. Then, we don't delete the custom settings you configured, they are just not used anymore until you possibly re-check the "custom settings" checkbox one day.

I'm fully aware that this is not exactly straightforward. But this the only way we found in D7 to combine "rich set of display 'flavours' (view modes)" and "progressive disclosure / no losing site admins in a sea of config". And it works pretty well in D7 IMO.

Ideally, I'd wish view modes and entity displays to be even more decoupled : you have a set of view modes defined by the modules you currently run, you can configure a set of displays, and can freely assign view modes to displays in a matrix : view mode a and b use display 1, all the others use display 0.
But that has its own set of challenges, so the current (either a specific display tied to the view mode, or 'default') is a scaled down version.

Can't we just enforce having an empty EntityDisplay for every bundle that doesn't have the custom settings, and kill off view modes altogether?

Not sure I get what you mean. We still need a list of the view modes that exist for an entity type ?

amateescu’s picture

I think he means what I said in the second paragraph of #195 :)

andypost’s picture

EntityDisplay is more like a bundle of renderable components that assigned to layout regions. For me it is very similar to panel's display but bound to entity route. The primary confusion happens once view modes get's their layout making difference between display and view mode really thin.

Working on #731724: Convert comment settings into a field to make them work with CMI and non-node entities we have a lot of troubles with view modes because comments currently exposed as 'node links' in teaser and custom render for search index and search result but for 'full' (probably default) we render comments as configurable list.
So converting comments to field makes me think that we need to bound them to entity display but only formatter in view mode allows to configure settings (per page and threading)

+++ b/core/modules/block/custom_block/config/entity.view_mode.custom_block.full.ymlundefined
@@ -0,0 +1,5 @@
+custom_settings: '0'

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityViewMode.phpundefined
@@ -0,0 +1,91 @@
+   * Whether or not this view mode has custom settings by default.
...
+  public $custom_settings = FALSE;

+++ b/core/modules/system/system.installundefined
@@ -2179,6 +2179,58 @@ function system_update_8053() {
+        ->set('custom_settings', $custom_settings)

this looks more like 'status' that all configurables have

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
22.29 KB
FAILED: [[SimpleTest]]: [MySQL] 55,925 pass(es), 1 fail(s), and 0 exception(s). View

custom_settings is the name currently used in HEAD, I see no reason to change it.
Let's abandon the EntityDisplay thing I brought up, sorry for the distraction.

Rerolled.

ParisLiakos’s picture

disclaimer: i read, none of the comments above

+++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityViewMode.phpundefined
@@ -0,0 +1,93 @@
+  public $targetEntityType;
...
+  public $custom_settings = FALSE;

+++ b/core/modules/file/config/entity.view_mode.file.full.ymlundefined
@@ -0,0 +1,5 @@
+custom_settings: '0'
+targetEntityType: file

any reason we cant have both consistent? either camel case or not..i am sure there is one, just asking

otherwise this seems very RTBCable to me if bot agrees

Status: Needs review » Needs work

The last submitted patch, view-modes-1043198-208.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
17.66 KB
29.88 KB
PASSED: [[SimpleTest]]: [MySQL] 55,986 pass(es). View

After further discussion with @swentel and @alexpott, I decided to go with renaming custom_settings to status anyway as @andypost suggested.
Also, I added EntityViewModeInterface to keep up with core best practices.

Finally, fixed the test failure.

twistor’s picture

Does the config need a schema?

Also, we need a new config file:
id: view_mode.woah
label: !!!!!!
status: '1'
targetEntityType: view_mode

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

i asked for upgrade paths in irc, but tim correctly pointed that user picture already covers it. (it was the fail in #208)
i asked about the schema, and it seems those are usually followups for the experts there:) (if it is actually needed)

+++ b/core/modules/field/field.moduleundefined
@@ -574,7 +574,7 @@ function _field_sort_items_value_helper($a, $b) {
+ *     - status: Boolean specifying whether the view mode uses a
  *       dedicated set of display options (TRUE), or the 'default' options

@@ -623,7 +623,7 @@ function field_bundle_settings($entity_type, $bundle, $settings = NULL) {
+ *   - status: Boolean specifying whether the view mode uses a
  *     dedicated set of display options (TRUE), or the 'default' options

those docblocks could be re-wrapped, but should not really hold this patch

+++ b/core/modules/file/config/entity.view_mode.file.full.ymlundefined
@@ -0,0 +1,5 @@
+label: File default
+status: '0'

Unrelated with this issue..but this is a WTF for me:) quotes should be for strings..but oh well thats a whole another topic

good job:)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/block/custom_block/config/entity.view_mode.custom_block.full.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/book/config/entity.view_mode.node.print.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/comment/config/entity.view_mode.comment.full.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/file/config/entity.view_mode.file.full.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/node/config/entity.view_mode.node.full.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/node/config/entity.view_mode.node.rss.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/node/config/entity.view_mode.node.teaser.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/search/config/entity.view_mode.node.search_index.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/search/config/entity.view_mode.node.search_result.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/system/system.installundefined
@@ -2142,6 +2142,58 @@ function system_update_8055() {
+        ->set('locked', TRUE)

+++ b/core/modules/system/tests/modules/entity_test/config/entity.view_mode.entity_test_render.full.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/system/tests/modules/entity_test/config/entity.view_mode.entity_test_render.test.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/taxonomy/config/entity.view_mode.taxonomy_term.full.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/taxonomy/config/entity.view_mode.taxonomy_vocabulary.full.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/user/config/entity.view_mode.user.compact.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

+++ b/core/modules/user/config/entity.view_mode.user.full.ymlundefined
@@ -0,0 +1,5 @@
+locked: '1'

The locked property is no longer used...

So I guess we can address the comment wrapping mentioned in #213 too...

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.38 KB
30.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,855 pass(es). View

Removed the left over locked (was used by the UI I tried to add earlier) and rewrapped those comments.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, back to rtbc.

alexpott’s picture

Title: Convert view modes to ConfigEntity » Change notice: Convert view modes to ConfigEntity
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: -Needs issue summary update

Committed 6fa50c6 and pushed to 8.x. Thanks!

Wim Leers’s picture

This broke Edit module (due to lack of test coverage there, I take the blame!). Follow-up to unbreak it: #1996378: Edit broken because of #1043198 and routing system bug + missing test coverage.

tstoeckler’s picture

Wow, huge kudos @tim.plunkett for picking this up and getting it in. Thanks and congrats!!!

larowlan’s picture

Status: Active » Needs review
andypost’s picture

Title: Change notice: Convert view modes to ConfigEntity » Convert view modes to ConfigEntity
Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Awesome!

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

tim.plunkett’s picture

Issue summary: View changes

Added tstoeckler to people who don't like one huge table.