Avoid conflating @EntityEmbedDisplay plugins and view modes

I agree. I argued initially against this change in #2736741: Treat each view mode in "Rendered entity" formatter as a standalone formatter because it seemed like we were trying to solve the problem of wanting to have configuration for which view modes were allowed by making them all display plugins, instead of just allowing the plugins to have configuration, in which case the 'Rendered entity' plugin would have added an extra checkboxes field for which view modes were allowed.

I think we need to look into reverting this change and making display plugins have configuration they can add into the embed button form.

@John Pitcairn, thanks for your update. Would you still support moving this change back with respect to the view modes being all contained to the 'Rendered Entity' formatter by default?

Also adding #2796269: Contact_form cannot be embedded without rendered entity formatter as another issue caused by this same bug.

Another related issue: #2828608: Embed button form lists cached @EntityEmbedDisplay plugin definitions; view mode-based ones need to be invalidated

This isn't exactly a bug, I think, but it's a pretty big usability snafu. Tagging accordingly. (Also removing the defunct D8Media tag.)

Changed the IS in attempt to more clearly summarize the problem.

But wouldn't prefacing the view mode-based checkboxes be fine if they were prefixed with: View mode: , so that you'd end up with View mode: [some name]?

And will this or won't this be a problem for #2801307: [META] Support WYSIWYG embedding of media entities? Why not?

But wouldn't prefacing the view mode-based checkboxes be fine if they were prefixed with: View mode: , so that you'd end up with View mode: [some name]?

It would make things a little clearer, sure, but then we need to be certain that, when you actually use one of those plugins at embed time, you don't get to choose a different view mode. In any case, we need to make a decision on how this stuff should be organized, and implement that.

And will this or won't this be a problem for #2801307: [META] Support WYSIWYG embedding of entities other than files? Why not?

Obviously nothing is certain at this point, but I don't think we're planning for core to support the concept of display plugins, which renders this entire point moot...right?

you don't get to choose a different view mode

Huh? Where is that happening?

I don't think we're planning for core to support the concept of display plugins, which renders this entire point moot...right

I'm not sure; because we are talking about allowing a view mode to be selected for an embed. Ooooooohh… all this time, I thought the issue title was referring to view modes versus entity view displays. But it's not referring to EntityViewDisplays, it's referring to EntityEmbedDisplay plugins. Alright, then yes, this is moot.

Where is that happening?

I think it would be happening in the dialog box presented when you choose an entity to embed in CKEditor. The final step before the drupal-entity tag is injected into the CKEditor source code is that you get a dialog box allowing to set various options for the display plugin. If the display plugin is a derivative tied to a particular view mode, that dialog should definitely not be allowing you choose a different view mode.

Ah, I see. This issue is conflating two scopes: a small and simple bug and addressing UI confusion.

That seriously confused me.

Thanks phenaproxima and John! 🙏

Here's a patch that just clarifies the display of the EntityEmbedDisplay plugins.

- When configuring the entity embed button it displays the EntityEmbedDisplay plugins as a tableselect with a column for type.

This makes it clear to the user if it's a view mode or a field formatter.
- When configuring the embed in the dialog it prefaces the EntityEmbedDisplay plugins with the type and a colon.

I just tested with file entity, and the tableselect is missing output in the type column. Needs some more work.

Fixes issue mentioned in #21, also, instead of adding the label in the deriver, I realized I can get it from the annotation of the EntityEmbedDisplay plugin.

I haven't had a chance to fully review yet, but I can say that this patch addresses the issue of not being able to use custom view modes created at admin/structure/display-modes/view.

I think the admin UI is intuitive but I think when the content editor is choosing a display mode they could likely not understand what "Entity reference" means. Ideally it would be good if the administrator could configure the labels of the options the user gets to choose from, but that is likely out of scope of this issue.

I'd like this to get feedback from at least two more people, because this will have significant impact on the UX.

Since we won't be bringing @EntityEmbedDisplay plugins to Drupal core, untagging.

So, I like this idea, and particularly this piece of the IS:

In other words: the subset of view modes I can use when embedding an entity should be presented as a configuration option of the Rendered entity display plugin. They should not be treated as separate display plugins.

I totally agree with this. It'd be a lot less confusing. Fewer options is generally better.

However, we DO need to maintain backwards compatibility forever, since plugin IDs are an API and will break existing embeds if we just change them. We can accomplish this by aliasing the rendered entity display plugin for every view mode in a hook_entity_embed_display_plugins_alter() implementation, and purposefully hiding them from the UI (just add a no_ui flag to the plugin definitions to remove them from the UI; core's Field UI has something like this already, so we can basically follow its lead).

This is also going to need tests, so I'm tagging for that.

#28: Unless I'm mistaken, #22 only changes labels, not identifiers. So it wouldn't break anything. The issue summary is outdated.

The UX solution in #22 only changes the UX of the embed button config form to make it clearer. It does this by using a table layout to list which are view mode displays and which are field formatter displays.

It doesn't update the dialog.

Also, there's a typo in #22, "Plugin" has the second letter capitalized.

+          'type' => $this->t('Display PLugin Type'),

I'm thinking the UX should be a separate issue, as it doesn't attempt to solve the problem with the proposed solution, but rather an alternate solution to the problem of the entity embed config form being "confusing".

The idea of combing the view mode displays in some way, either on the entity embed config form, or the dialog could and perhaps should still be explored.

IMHO, "Editor's confusion" is more important and should be fixed with higher priority than "Site builder's confusion".

I agree the table approach in #22 contributes a lot to reducing site builder's confusion, so +1 to that!

I have concerns however if it wouldn't make Editor's confusion a little bit worse, since they should never need to be familiarized with terminology such as "Field Formatter: x" or "Viewmode: x", IMO. In my mind, an ideal solution would allow the site builder to select which plugins Editors get to see, and optionally give them a user-facing label. So something like:

- Field Formatter: Rendered Entity, no field wrapper
- Viewmode: Embed
- Viewmode: Featured Videos

Would be presented to editors as something like:

- Teaser
- Fancy layout
- Stacked

These could obviously be optional overrides, so if the site builder doesn't define them, we would default to the first group.

Thoughts?

An ideal solution would allow the site builder to select which plugins Editors get to see, and optionally give them a user-facing label.

+1 to this. View mode labels often ship with modules and the labels aren't always editor friendly. Allowing the labels to change on a per embed button basis, while remaining the same elsewhere in the backend sounds like a good thing.

View mode labels often ship with modules

Really? What's an example of this?

I think we should just do #22 for both content creator and site builder for now. It's simple, is an improvement, and most importantly: it's both automatic and standardized. If we allow custom labels, then we need to provide an admin UI for it, we need to manage it, and it's then still possible to enter confusing labels.

I'm fine with not complicating it for now.

As far as view modes that ship with modules, here was a quick search in core:

Each of these has a label, for example:
label: Summary

Here's a reroll with a couple of changes.

1) I drop change to the display of the plugins in the select dropdown within the EntityEmbedDialog based on #31:

I have concerns however if it wouldn't make Editor's confusion a little bit worse, since they should never need to be familiarized with terminology such as "Field Formatter: x" or "Viewmode: x",

2) This patch fixes the typo:

-          'type' => $this->t('Display PLugin Type'),
+          'type' => $this->t('Display Plugin Type'),

I suggest we create a separate issue for the UX change to the embed button form, and we can leave the original proposal TBD.

Or we update the issue summary on this one.

#34: Right. I meant to say that contrib and custom modules rarely ship with additional view modes.

1. Notice: Undefined index: type_label 
   

   This needs an update hook that clears caches :)

2. This is not yet changing anything in the embed dialog that the content creator gets to see. I think we should improve that too.

For #37.2, I had had changes to the dialog in patch #22, but removed them because marcoscano felt the display types were not useful to the user:

I have concerns however if it wouldn't make Editor's confusion a little bit worse, since they should never need to be familiarized with terminology such as "Field Formatter: x" or "Viewmode: x"

diff --git a/src/Form/EntityEmbedDialog.php b/src/Form/EntityEmbedDialog.php
index d4fb7d4..f870fba 100644
--- a/src/Form/EntityEmbedDialog.php
+++ b/src/Form/EntityEmbedDialog.php
@@ -810,12 +810,19 @@ class EntityEmbedDialog extends FormBase {
    *   List of allowed Entity Embed Display plugins.
    */
   public function getDisplayPluginOptions(EmbedButtonInterface $embed_button, EntityInterface $entity) {
-    $plugins = $this->entityEmbedDisplayManager->getDefinitionOptionsForEntity($entity);
+
+    $plugins = $embed_button
+      ->getTypePlugin()
+      ->getDisplayPluginOptions($entity->getEntityTypeId());
 
     if ($allowed_plugins = $embed_button->getTypeSetting('display_plugins')) {
       $plugins = array_intersect_key($plugins, array_flip($allowed_plugins));
     }
 
+    $plugins = array_map(function ($item) {
+      return $item['type'] . ': ' . $item['label'];
+    }, $plugins);
+
     natsort($plugins);
     return $plugins;
   }

What do you think we should change in the EntityEmbedDialog?

The two proposals so far are to prepend the type (as the code above does) or to allow configuration of the labels.

I don't have any other ideas right now. Personally I haven't had a problem with the display in the dialog.

but removed them because marcoscano felt the display types were not useful to the user:

Right.

Personally I haven't had a problem with the display in the dialog.

Right.

But not doing this prepending still doesn't solve the problem that was originally reported: see the screenshot in the issue summary. I see where @marcoscano is coming from when he writes

I have concerns however if it wouldn't make Editor's confusion a little bit worse, since they should never need to be familiarized with terminology such as "Field Formatter: x" or "Viewmode: x"

But exposing those terms at least increases learnability, discoverability and searchability for the content author. If they're not going to be informed that these are different, customizable representations of the same thing, then I think they're going to end up potentially more confused.

I do agree that ideally we wouldn't burden the content author with this. This same problem was raised nearly 5 years ago at DrupalCon Amsterdam 2014, see #2349371: Display selection can be confusing. Which is why #2736741: Treat each view mode in "Rendered entity" formatter as a standalone formatter happened. I've been arguing for years that Entity Embed is super confusing precisely because it doesn't use only view modes. I've proposed 3.5 years ago to remove @EntityEmbedDisplay plugins altogether in favor of just using view modes, in #2628234: Document why Entity Embed Display Plugins are necessary. But over at #2628234-3: Document why Entity Embed Display Plugins are necessary, it was explained to me by the maintainers at the time why this was necessary. Perhaps things have changed since then, and we now can rely only on entity view modes/entity view displays/entity view builders in a 2.x branch of this module?

I think it's clear we don't yet know how to solve this.

I agree that content author should not be bothered with confusing implementation details. Which in the end might mean we need another abstraction layer on top (and hope we can avoid that).

As of
> I've proposed 3.5 years ago to remove @EntityEmbedDisplay plugins altogether in favor of just using view modes, in #2628234: Document why Entity Embed Display Plugins are necessary. But over at #2628234-3: Document why Entity Embed Display Plugins are necessary, it was explained to me by the maintainers at the time why this was necessary.
> Perhaps things have changed since then, and we now can rely only on entity view modes/entity view displays/entity view builders in a 2.x branch of this module?

I read that whole issue to understand it, strongly biased to we-only-need-view-modes. But:

> (My words:) "There are use cases where a predefined set of view modes is not enough, and we need some configuration (think 'fields') associated to a *specific* embedding instance."

...really convinced me that the current additional complexity makes sense. When doing similar use cases using paragraphs, we several times had the situation where we needed additional fields "on the embedding itself".

And, as stated above, given that, we might need more complexity to hide that first complexity from content authors. Sigh.