Problem/Motivation

By default, when you add a media field to a content type, it will default to rendering a link to the entity, rather than rendering (e.g.) your image on your content the way a user expects. Furthermore, the process of properly configuring the view mode and image style for the desired display is very complicated. (This is a problem for entity reference fields generally, but worse for media.)

An example of this is when you reference media entities and want to display a image field in a lot of different image styles. Needing a view mode for each image style to display is not very user friendly.
The problem of "I need to display part of a referenced entity as though it were part of the referencing entity" pops up many times in the course of normal Drupal development.

This video illustrates the steps required: https://www.drupal.org/files/issues/rendered-entity-formatter-steps.mp4

Steps required:

  1. Go to 'Manage display' and select 'Rendered Entity' formatter
  2. Configure 'Rendered Entity' formatter (this is where you should get told that you need a image style and view mode?)
  3. Save node view mode with new formatter for media field.
  4. Create new image style.
  5. Configure image style.
  6. Save image style.
  7. Create view mode for media.
  8. Save view mode for media.
  9. Enable new view mode for media bundle (this needs to happen for every bundle enabled on the field).
  10. Configure new view mode for media bundle.
  11. Enable new image style on the image field.
  12. Save the media view mode.
  13. Go back to node view mode.
  14. Configure the media field to use the new view mode.
  15. Save the node view mode.

This is clearly a lot more complicated than configuring the display of core image fields, and more than site builders will figure out on their own!

Proposed resolution

The previous approach created a generic, reusable formatter to display a field from the referenced entity, but this solution raised a number of concerns and limitations (see #103, #123, #136, etc.). (The formatter may be available in contrib instead; see #2894947: Add a formatter to display a rendered field from a referenced entity.)

The new approach focuses on providing better defaults in core, and allowing an image style to be selected through the field UI.

Remaining tasks

- Fix the bugs indicated in #183
- Extend test coverage
- Review
- Collect sign-off & RTBC

CommentFileSizeAuthor
#189 interdiff-181-189.txt10.35 KBmarcoscano
#189 2831943-189.patch22.47 KBmarcoscano
#189 prevent_redirect_after_creating_new_vm.mp41.67 MBmarcoscano
#183 creating_new_viewmode.mp43.41 MBmarcoscano
#183 configure_existing_viewmode.mp42.09 MBmarcoscano
#181 2831943-181.patch17.06 KBchr.fritsch
#4 media_image_file_formatters-2831943-4.patch15.5 KBpguillard
#5 media_image_file_formatters-2831943-5-do-not-test.patch23.69 KBpguillard
#5 media_image_file_formatters-2831943-5-full.patch236.53 KBpguillard
#6 media_image_file_formatters-2831943-6-do-not-test.patch24.14 KBpguillard
#6 media_image_file_formatters-2831943-6-full.patch236.98 KBpguillard
#10 2831943-10-full.patch245.93 KBseanB
#10 2831943-10-formatters-only-do-not-test.patch19.87 KBseanB
#18 2831943-17.patch21.67 KBmarcoscano
#18 interdiff-10-17.txt15.04 KBmarcoscano
#23 2831943-23.patch33.89 KBmarcoscano
#23 interdiff-18-23.txt13.4 KBmarcoscano
#24 media_file_formatters.png30.73 KBmarcoscano
#24 media_image_formatters.png53.89 KBmarcoscano
#32 reference-options.png31.1 KBmallezie
#45 field_formatter_inline.gif1.48 MBmarcoscano
#45 2831943-45.patch14.26 KBmarcoscano
#49 inline_settings_formatter_no_basefields.png28.31 KBmarcoscano
#56 2831943-56.patch16.72 KBmarcoscano
#56 interdiff-45-56.txt7 KBmarcoscano
#59 2831943-59.patch16.94 KBmarcoscano
#59 interdiff-56-59.txt11.48 KBmarcoscano
#63 2831943-63-do-not-test.patch17.47 KBmarcoscano
#63 interdiff-59-63.txt4.36 KBmarcoscano
#66 2831943-66.patch19.53 KBmarcoscano
#66 interdiff-63-66.txt2.99 KBmarcoscano
#68 2831943-68-do-not-test.patch21.05 KBmarcoscano
#68 interdiff-66-68.txt9.82 KBmarcoscano
#69 2831943-69.patch26.19 KBmarcoscano
#69 interdiff-68-69.txt5.69 KBmarcoscano
#74 2831943-73.patch24.8 KBmarcoscano
#74 interdiff-69-73.txt5.58 KBmarcoscano
#76 2831943-76.patch23.59 KBmarcoscano
#76 interdiff-74-76.txt8.34 KBmarcoscano
#78 media entity reference 1.png96.57 KByoroy
#79 label.png36.89 KByoroy
#80 interdiff-76-80.txt4.68 KBmarcoscano
#80 2831943-80.patch22.79 KBmarcoscano
#82 2831943-82.patch27.88 KBmarcoscano
#82 interdiff-80-82.txt6.83 KBmarcoscano
#83 2831943-83.patch28.78 KBphenaproxima
#83 interdiff-2831943-82-83.txt4.42 KBphenaproxima
#89 2831943-89.patch32.66 KBmarcoscano
#89 interdiff-83-89.txt22.85 KBmarcoscano
#92 2831943-92.patch32.66 KBmarcoscano
#92 interdiff-89-92.txt1.21 KBmarcoscano
#94 2831943-94.patch31.98 KBmarcoscano
#94 interdiff-92-94.txt17.57 KBmarcoscano
#99 2831943-99.patch32.37 KBmarcoscano
#99 interdiff-94-99.txt10.1 KBmarcoscano
#101 2831943-101.patch32.22 KBmarcoscano
#101 interdiff-99-101.txt1.95 KBmarcoscano
#104 2831943-104.patch36.31 KBmarcoscano
#104 interdiff-101-104.txt13.39 KBmarcoscano
#106 2831943-106.patch33.26 KBmarcoscano
#106 interdiff-104-106.txt3.23 KBmarcoscano
#110 2831943-110.patch33.23 KBmarcoscano
#110 interdiff-106-110.txt4.8 KBmarcoscano
#111 2831943-111.patch33.25 KBmarcoscano
#111 interdiff-110-111.txt979 bytesmarcoscano
#113 2831943-113.patch33.17 KBmarcoscano
#113 interdiff-111-113.txt1.63 KBmarcoscano
#116 2831943-116.patch33.24 KBmarcoscano
#116 interdiff-113-116.txt8.92 KBmarcoscano
#119 2831943-UX-current-patch.mp42.37 MBWim Leers
#119 2831943-UX-steps-1-and-2.mp4404.05 KBWim Leers
#125 Screenshot_20170712_115324.png15.3 KBamateescu
#126 Screen Shot 2017-07-12 at 12.23.29.png21.09 KBWim Leers
#130 types_or_sources1.png23.98 KBmarcoscano
#135 rendered-entity-formatter-steps.mp49.63 MBseanB
#135 rendered-field-from-referenced-entity-formatter-steps.mp43.81 MBseanB
#136 2831943-entity_reference_entity_view__settings__single.png25.94 KBWim Leers
#136 2831943-entity_reference_entity_view__settings__multiple.png29.73 KBWim Leers
#136 2831943-view_modes-136.patch5.74 KBWim Leers
#137 2831943-view_modes-137.patch6.19 KBWim Leers
#137 interdiff-136-137.txt2.76 KBWim Leers
#137 2831943-entity_reference_entity_view__settings__modal.png272.03 KBWim Leers
#138 2831943-UX-137.mp4995.87 KBWim Leers
#142 formatter.png140.72 KBseanB
#153 Screen Shot 2017-07-14 at 14.51.39.png146.84 KBWim Leers
#153 interdiff.txt2.28 KBWim Leers
#153 2831943-view_modes-153.patch8.42 KBWim Leers
#154 interdiff.txt2.33 KBWim Leers
#154 2831943-view_modes-154.patch8.26 KBWim Leers
#155 interdiff.txt2.91 KBWim Leers
#155 2831943-view_modes-155.patch11.08 KBWim Leers
#155 Screen Shot 2017-07-14 at 15.43.21.png145.38 KBWim Leers
#156 Screen Shot 2017-07-14 at 15.53.10.png214.13 KBWim Leers
#156 Screen Shot 2017-07-14 at 15.53.17.png196.96 KBWim Leers
#156 interdiff.txt2.78 KBWim Leers
#156 2831943-view_modes-156.patch11.58 KBWim Leers
#159 interdiff.txt1.36 KBWim Leers
#159 2831943-view_modes-157.patch11.98 KBWim Leers
#159 Screen Shot 2017-07-14 at 17.23.43.png211.29 KBWim Leers
#162 interdiff.txt5.6 KBWim Leers
#162 2831943-view_modes-160.patch15.62 KBWim Leers
#169 interdiff.txt1.11 KBWim Leers
#169 2831943-view_modes-169.patch15.26 KBWim Leers
#170 interdiff.txt7 KBWim Leers
#170 2831943-view_modes-170.patch18.71 KBWim Leers
#170 Screen Shot 2017-07-17 at 15.41.26.png220.75 KBWim Leers
#170 Screen Shot 2017-07-17 at 15.41.29.png204.88 KBWim Leers
#171 interdiff.txt2.53 KBWim Leers
#171 2831943-view_modes-171.patch20.49 KBWim Leers
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

slashrsm created an issue. See original summary.

pguillard’s picture

Issue summary: View changes
pguillard’s picture

Assigned: Unassigned » pguillard
pguillard’s picture

I suggest to implement formatters for media entities that re-create behavior of existing ones.

Here is a first patch. I still work on it.

Illsutration of what we get :

Only local images are allowed.

Only local images are allowed.

pguillard’s picture

I did :

  1. use isApplicable() f in order to differentiate file and image formatters
  2. added media_hanflers to the formatters plugins
  3. added a weight to respect formatters orders

Naming proposition (Wait for your suggestions), we have :

For a "normal" file field :
- RSS enclosure
- URL to file
- Generic file
- Table of files

For the media file "field" :
- URL to *media* file
- Generic *media* file
- Table of *media* files

For the image "normal" field :
- URL to image
- Image

For the media image "field" :
- URL to *media* image
- *Media* image

Missing/Remarks

  1. The CSS stuff :
    The entity reference style is taking precedence on the "normal" field style we want to imitate :
    .field--type-entity-reference .field__label {
        font-weight: normal;
        margin: 0;
        padding-right: 5px;
    }
    
    .field__label {
        font-weight: bold;
    }
    
  2. I'm not pride of that one, to prevent the default :
      protected function needsEntityLoad(EntityReferenceItem $item) {
        return TRUE;
      }
  3. There is no public method in the MediaHandler to get the source file, so this is what I did to get the file from a Media Entity(Thanks @mtodor)
          $source_field_name = $entity->getHandler()->getConfiguration()['source_field'];
          /** @var \Drupal\file\FileInterface $file */
          $file = $entity->get($source_field_name)->entity;
  4. Some viewElements() are not implemented yet.
    (Missing MediaTableFormatter & MediaImageFormatter)
pguillard’s picture

All viewElements() are now implemented and working.

These patches are still based on 2831937_19-full.patch from #2831937: Add "Image" MediaSource plugin

pguillard’s picture

Status: Needs work » Needs review

The last submitted patch, 4: media_image_file_formatters-2831943-4.patch, failed testing.

seanB’s picture

Status: Needs review » Needs work

There is a lot of duplicate code between formatters. It's probably a good idea to create a MediaFormatterBase class and extend that for the formatters in stead of extending FileFormatterBase.

Haven't tested it yet, will try to do that asap!

seanB’s picture

Status: Needs work » Needs review
FileSize
245.93 KB
19.87 KB

Rebased patch on latest version of file/image plugins and media patch. Added base class for generic functions and fixed some issues.

Based on:
#2831936-47: Add "File" MediaSource plugin
#2831937-24: Add "Image" MediaSource plugin
#2831274-176: Bring Media entity module to core as Media module

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

willyk’s picture

I agree this is an important/needed feature. I will try to assist with the solution

seanB’s picture

Most important thing to do here is adding tests.

phenaproxima’s picture

Wim Leers’s picture

Do you want to split this up into an issue for File and one for Image, because #2831937: Add "Image" MediaSource plugin has not yet landed? Or do we want to keep these together? I'm fine with either approach.

Here's an initial review.

  1. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaFileFormatterBase.php
    @@ -0,0 +1,48 @@
    +    if (parent::isApplicable($field_definition) && $field_definition->getSetting('target_type') == 'media') {
    

    ===

  2. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaFileFormatterBase.php
    @@ -0,0 +1,48 @@
    +      /** @var MediaType[] $bundles */
    

    FQCN

  3. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaFileFormatterBase.php
    @@ -0,0 +1,48 @@
    +      $definitions = \Drupal::service('plugin.manager.field.formatter')->getDefinitions();
    

    Must be injected.

  4. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaFileFormatterBase.php
    @@ -0,0 +1,48 @@
    +        if ($definition['class'] == static::class) {
    

    ===

  5. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaGenericFileFormatter.php
    @@ -0,0 +1,58 @@
    +    $elements = array();
    ...
    +      $elements[$delta] = array(
    

    [], here and in many other places.

  6. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaGenericFileFormatter.php
    @@ -0,0 +1,58 @@
    +          'tags' => $file->getCacheTags(),
    

    This is not enough; let's use Renderer::addCacheableDependency() instead.

  7. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaGenericFileFormatter.php
    @@ -0,0 +1,58 @@
    +      // Pass field item attributes to the theme function.
    +      if (isset($item->_attributes)) {
    +        $elements[$delta] += array('#attributes' => array());
    +        $elements[$delta]['#attributes'] += $item->_attributes;
    +        // Unset field item attributes since they have been included in the
    +        // formatter output and should not be rendered in the field template.
    +        unset($item->_attributes);
    +      }
    

    This is … puzzling. Yet I see it also exists for EntityReferenceLabelFormatter,

    DateTimeFormatterBase</c ode> and others.
    
    Out of scope to fix here.
    </li>
    
    <li>
    <code>
    +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaImageFormatter.php
    @@ -0,0 +1,283 @@
    + *   quickedit = {
    + *     "editor" = "image"
    + *   },
    

    <3

    We'll need test coverage to prove this works though. Fortunately there's already existing test coverage :)

  8. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaImageFormatter.php
    @@ -0,0 +1,283 @@
    +   * Constructs an ImageFormatter object.
    

    MediaImageFormatter

phenaproxima’s picture

Status: Postponed » Needs work

#2831937: Add "Image" MediaSource plugin has landed, so this is no longer postponed.

marcoscano’s picture

Component: file system » media system
Assigned: pguillard » marcoscano

Working on this

marcoscano’s picture

Still needs work, but freeing up the issue in case someone else can/wants to continue from here.

Addressed from #15:

1: OK
2: OK
3: How to do it inside the static method? I also saw that EntityReferenceEntityFormatter::isApplicable() also uses the global service in the same situation.
4: OK
5: OK
6: OK
7: nothing to do here
7b: pending tests
8: OK

Also, in this patch, I:
- renamed "media_handlers" to "media_sources" in the annotation and all places that referred to it
- fixed ::getHandler() in all places that were using it (and used ::getSource() instead)
- renamed some variables to stop using bundle / handler and start using type / source.

Still missing the tests. If nobody can work on them before that, I can start implementing them around beginning next week I believe.

marcoscano’s picture

Assigned: marcoscano » Unassigned
naveenvalecha’s picture

Status: Needs work » Needs review

Did you forget to change the issue status?

//Naveen

marcoscano’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Not really, at this point I don't think it's worth a review until the tests are done (although if anyone has time and wants to do it, all reviews are welcome!)

marcoscano’s picture

Assigned: Unassigned » marcoscano

Working on this.

marcoscano’s picture

Assigned: marcoscano » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
33.89 KB
13.4 KB

First shot at the tests, not sure if the coverage is enough, feedback appreciated.

I also added some missing schema definitions for the image formatters.

marcoscano’s picture

Also, while testing this manually, I noticed that now the Entity Reference formatters are mixed with the new ones, and I wonder if we want to keep them all, rename some of them, or something else...

For a media field referencig file media entities:
file formatters

For a media field referencing image media entities:
media image formatters

For me it feels a bit confusing to have all of them there. It's not evident for a site builder how a "rendered entity" option will show differently from "image" for example.

tstoeckler’s picture

So I think that leads to the question that is already posed in the issue summary: Do we need dedicated formatters in the first place, or is the Rendered entity formatter sufficient? I mean if you use the Rendered entity formatter with a view mode that only shows the image field of the media entity in the desired image formatter isn't the result the same? I get that from a site builder's perspective that is slightly more complex to understand, than selecting e.g. Link to file directly in the node display, but every formatter we add here also comes with a maintenance burden.

And I wonder if there other ways to solve the usability issue. We could, for example, provide a bunch of view modes for the media entity in the Standard profile that match the respective formatters, so that the settings summary of the Rendered entity formatter would read e.g. Rendered as File link.

I'm not saying that providing dedicated formatters is an abolute no-go, but I'm not sure that we've sufficiently explored other alternatives yet.

Wim Leers’s picture

#25 raises an excellent question!

phenaproxima’s picture

Issue tags: -D8Media +Media Initiative

Re-tagging.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima
Status: Needs review » Needs work

Re #25...damn, why didn't we think of that? It's a terrific idea! We discussed it on IRC today, and we have decided to proceed as follows:

  • Media will provide a new view mode for media items, called "Embedded".
  • It will also provide Embedded view displays for each media type it includes, currently File and Image.
  • The Embedded view displays will only display the source field for each media type, with sane default configuration.
  • Media will add a bit of code (hook_entity_view_display_presave, I think) which will set up new media reference fields to display the referenced media item with the "Rendered entity" formatter, using the Embedded view mode as a sane default. Lightning already does something similar, so we can easily borrow and adapt that code.

The task of implementing this (as a proof of concept patch) has fallen to me, so I'm assigning this issue to myself.

Wim Leers raised the concern that this level of configurability might be counterproductive and possibly confusing, so he floated the idea of having a formatter which hard-codes the use of the Embedded view mode. I will not be implementing that in the proof of concept, but it's certainly a possibility to consider as we iterate on this. The general consensus is that we'd rather have fewer formatters than more, if possible.

It's also worth noting that, once the patch is uploaded, we'll want UX review on it, and then we'll have to iterate/follow-up on anything that comes out of that.

Wim Leers’s picture

One addition to #28: this will also help the Text Editor/CKEditor/Filter integration, because that can then reuse this same EntityViewDisplay, which means that media embedded either via a Field or in rich text will look exactly the same!

seanB’s picture

Currently users have multiple options for formatters (Generic file / URL to file / Table). When replacing this with 1 view mode, you will be losing options. Technically the user can change the settings on the Media view mode, but this will be harder to configure. Especially when you want to use different display options for different fields. You then have to create a view mode for each type of display.

So while I think it's ok to use view modes, I think we should at least have a view mode for each option currently available to users (Generic file / URL to file / Table). A possible issue for this approach is that other media types (like facebook or twitter) will also be able to enable the view modes for Generic file (I think view modes are created per entity type). This could lead to confusion.

So yeah, now that I think about it, I'm not 100% sure the view mode approach is actually better for the user.

marcoscano’s picture

Me too I'm in favor or making the transition from "using file and image fields" to "using media files and images" as smooth as possible (from a end-user standpoint).

TBH I'm not entirely convinced either way, I can see pros and cons on both approaches. As @seanB said in #30, maybe having several viewmodes by default could be a good compromise.

Another option is to have a submodule (or even a contrib module?) just for providing some additional formatters, I don't know...

In any case, it may be a good idea to have the PoC mentioned in #28 implemented, so we can compare and probably iterate from there.

mallezie’s picture

FileSize
31.1 KB

Is the main problem then not that we should have some kind of inline view mode adjustment possibility for embedded entity.
What happens now (IMO) is that you reference a media entity on an other entity. So the format is 'rendered entity'. There you can then choose in which view mode you want the referenced entity to be shown.
To adjust those options, or add some you need to go to the media entity configuration and adjust / add view modes there.
IMO the main problem here is that those 2 configurations are decoupled. Which can be confusing / difficult for the end user, and a (rather large) difference in the current image / file field configuration.
Could adjusting the UI for this (on a more general level) not solve this?

Currently we have this as options for the referenced entity formatter.

reference options

If we adjust this to be able to edit / add those view modes 'inline' through a modal (for example). This could possible ease the disconnect in the configuration?

marcoscano’s picture

Maybe we could borrow some ideas from https://www.drupal.org/project/field_formatter to make this configuration of the "rendered entity" formatter easier?

Wim Leers’s picture

Another option is to have a submodule (or even a contrib module?) just for providing some additional formatters, I don't know..

This.

Moving Media into core is IMHO not about moving every Media feature into core. It's about moving the 90% use case into core, and providing solid APIs for contrib to interact with. Providing e.g. a "Table" formatter definitely feels like a <10% use case, and hence feels like it belongs in contrib.

Some historical context: We did something similar with rich/WYSIWYG text editing when we brought that into Drupal core 4–5 years ago (I led that initiative — back before it was called "an initiative"). We have the editor.module that provides an API for pluggable text editors. ckeditor.module provides the default experience. We don't use CKEditor's native link dialog because it's far too complex, and doesn't integrate with Drupal well. So editor.module provides a standardized, hook_form_alter()-able link dialog that covers the 90% use case. The 10% use case, to add title, class, id and so on attributes is provided by a contrib module that uses hook_form_alter(): https://www.drupal.org/project/editor_advanced_link. It has 8.5K installs out of 155K Drupal 8 sites, or 5.5%. Essentials/90% in core, edge cases/10% in contrib.

seanB’s picture

While I completely agree that table display is probably not the most used feature, we are trying to replace core file/image with media (eventually). If we are going to remove features, this could lead to issues in the upgrade path. I don't mind removing contrib media features, removing core file/image features is another issue though.

I like the idea of integrating something like field_formatter. This has the best of both worlds really:
- It's not a "dedicated" formatter for media, but generic for entity references
- It allows the user to configure file/image display from the parent entity, which makes it easier for sitebuilders
- Other entity reference fields could also benefit from this

Looking at field_formatter it's not really that complex. We could start with only a single formatter to fix the common use cases?

Wim Leers’s picture

If we are going to remove features, this could lead to issues in the upgrade path.

We could have an update of the Media contrib module start having a hook_requirements() that analyzes the site's configuration, notices they're using e.g. the table formatter, and then warns them they'll have to install module media_special_formatters to continue to use that.

phenaproxima’s picture

I am not against the idea of simplifying the formatter configuration for site builders and end users, but here's the thing: we are working under a deadline. The goal is to land it, not finesse it. If the path of least resistance is to package a couple of simple view modes and a bit of glue code with Media, then that is what, IMHO, we should do -- even if it's a little more complex for the end user, initially. I'm pretty certain we will be able refine it later and migrate to better options, as needed, but for now we just need to get it done as quickly as possible and with a minimum of fuss.

So basically, as @marcoscano said in #31, let us implement the PoC from #28 and proceed from there. In my opinion, under these circumstances, adding any complexity in the name of simplifying the UI is the last thing we should do. It won't be the greatest UX in the world, but it will be far from the biggest UX WTF in Drupal core.

seanB’s picture

Just posting this here. Phenaproxima, marcoscano and I had a short discussion on IRC. We agreed a new field_formatter type of solution is probably best. Hopefully we can reach consensus on this and move the issue forward.

3:06 PM <phenaproxima> seanB: I think the goal should be getting SOMETHING in, even if the UX is a bit sub-optimal.
3:07 PM <seanB> phenaproxima: Then why not write a test for the formatters, those are already done, are exactly what file/image does
3:08 PM <phenaproxima> seanB: Only because it would be easier, I think, to migrate *to* them than to migrate *from* them, if we switched from those to view modes.
3:08 PM <seanB> phenaproxima: I think that is what we are aiming for. Changing it to view modes will probably get some resistance. But maybe we should ask xjm.
3:08 PM <phenaproxima> seanB: I'm open to that.
3:09 PM <seanB> phenaproxima: Or maybe GaborHojtsy can also weigh in on this one.
3:09 PM <phenaproxima> seanB: I'm in favour of asking, for sure :)
3:10 PM <seanB> phenaproxima: I believe creating a new field_formatter type formatter is the best solution. The view modes is the easy/clean solution with the least amount of code. The specific media formatters for file/image mimick best what's currently in core.
3:10 PM <phenaproxima> seanB: Can you elaborate a little more on that field_formatter solution?
3:11 PM <seanB> phenaproxima: There is this https://www.drupal.org/project/field_formatter
3:12 PM <seanB> phenaproxima: We create a generic entity reference formatter that allows you to pick a field from the referenced entity, and select how to display that.
3:12 PM phenaproxima: generic, flexible and easy to configure for sitebuilders.
3:12 PM <phenaproxima> seanB: I like it.
3:13 PM <seanB> phenaproxima: field_formatter has I think 2 different formatters. We should just stick to 1 that server to 90% use case.
3:14 PM <phenaproxima> seanB: Should we port it in from Field Formatter?
3:14 PM <seanB> phenaproxima: We should most definitely use that as a starting point. 
3:14 PM phenaproxima: Maybe few tweaks.
3:14 PM <phenaproxima> seanB: I am open to that. We should probably consult with marcoscano, WimLeers, and GaborHojtsy
3:15 PM <marcoscano> Hey! Me too I believe porting the inline-settings formatter from Field Formatter would be a huge win and would benefit not only media
3:16 PM <seanB> phenaproxima: Ok wel if at least we agree, that already helps
3:16 PM <marcoscano> although I'm unaware of the overhead that this implies time-wise
3:16 PM <seanB> phenaproxima: And we have marcos on board
3:16 PM <marcoscano> :)
3:17 PM <phenaproxima> seanB: Okay, if we can get Wim and/or Gabor on board, I'm in too.
3:17 PM <seanB> phenaproxima: marcoscano: Going back and forth over the solutions takes the most time I think. It is not that much work
3:18 PM <marcoscano> In any case, I believe from the 3 options we have (1: just adding up new formatters, 2: viewmodes "loosing" some core features, and 3: porting part of Field Formatter)
larowlan’s picture

My 2c: I agree with @tstoeckler in #25

I'm not convinced we need to duplicate all the formatters - on all the D8 projects I've used media on (building sites with D8 is my day job), we always just configure the rendered entity view modes for the media entity.

Because then it looks the same everywhere - views, entity embed, field display, ds....so on.

I get that there is a learning curve, but we should be teaching people scalable site building in my opinion.

Will ask some others to chime in.

seanB’s picture

Thanks for your feedback!

I'm very curious how you handle the different image styles/responsive images? Do you create a view mode for each image style and/or responsive images setting?

larowlan’s picture

Yeah, one media entity view mode for each different styling, to suit the components in the style guide.

seanB’s picture

In the current situation:
- a user creates an image style
- configures it directly on the parent entity field.

In the new situation:
- a user creates an image style
- creates a corresponding view mode
- configures the view mode on the image field of the media item
- configures the new media view mode on the parent entity field.

I agree the 2 extra steps are no problem for experienced drupal users, I think this is way to complex for less experienced / new users though.

marcoscano’s picture

Echoing #42.

Quoting from #2825215: Media initiative: Essentials - first round of improvements in core:

Allow site builders to start creating entity reference (ER) fields to Media entities instead of Image fields, without compromising features or UX for content authors and site visitors. This requires adding:

And also:

When all the above is done, change Standard profile to ship with ER fields to Media entities instead of File/Image fields. Don’t force existing sites to migrate. They can keep using files as we don’t remove any of that from core. We can provide optional migration path later, but don’t require it until D9.

Now that we have more visibility on the options available, I start to feel that, while the viewmodes approach is totally viable for experienced users, maybe it won't fulfill 100% what we wanted at the beginning ("without compromising features or UX for content authors"). I fear that, while the media entities in core will make easier to deal with advanced assets (remote and non-file assets), we are probably making it harder to deal with the simpler use cases (file and image).

marcoscano’s picture

Just summarizing the alternatives brought up on this issue:

1) to stuff old and new formatters together, as showed in #24
2) to provide only a trimmed list and leave the less used ones to contrib (#34, #36)
3) to keep the formatters we currently have, and use specific viewmodes to show rendered media entities, (#25, #28, #37, #39)
4) to create a new entity reference formatter porting part of Field Formatter in core (#33, #35, #42)

marcoscano’s picture

So this is a very rough proof-of-concept of option 4 (which is my favorite btw :). Not really ready for review (just copied the formatter over and adjusted minor details), but it serves to illustrate we are talking about.

This would be the idea of the worflow for a simple image field:
field formatter inline

It's also true that the long list of default fields from the media entity doesn't help, and users may be confused and not know that they need to choose a field called "Image". I still believe this would be the easiest option for this basic case, though.

Wim Leers’s picture

#39: Thanks for sharing your experience on actual sites, that's super helpful!

#42

In the new situation:
- a user creates an image style
- creates a corresponding view mode
- configures the view mode on the image field of the media item
- configures the new media view mode on the parent entity field.

Why can't this be created automatically upon installing Media and/or adding a Media field to an entity type bundle?


I think ultimately this is partially a UX decision, partially a "technical MVP" decision. The reason I agree strongly with @tstoeckler's proposal in #25 is simply that it seems the technical MVP _and_ it seems simpler for site builders too. Especially if we can ensure that the necessary media view display is auto-created.

seanB’s picture

#45 +1, this looks really nice. Maybe we could limit the formatter to only display the source field? This already removes a ton of options. Although this would make the formatter media specific instead of useful for other entity reference fields.

#46

Why can't this be created automatically upon installing Media and/or adding a Media field to an entity type bundle?

It happens all the time a user decides to add an extra image style after a field is created (at least to me). The fact that I have to create/configure a new view mode on each media type related to an image source for each image style feels like a step backwards. I don't see how these extra steps make it simpler for site builders?

Wim Leers’s picture

A view mode is per media bundle though, not per image style. So I'm confused by #47.

Anyway, I defer to the UX team and Entity API folks like @tstoeckler on this.

marcoscano’s picture

Re: #47:

Maybe we could limit the formatter to only display the source field? This already removes a ton of options. Although this would make the formatter media specific instead of useful for other entity reference fields.

Alternatively, we could filter out the basefields, making this configurable, but defaulting to only show non-basefields. An example of how that could work:

Inline settings formatter with no basefields

seanB’s picture

#48

A view mode is per media bundle though, not per image style. So I'm confused by #47.

Well yes, but let's say I have a node type News with a media field allowing a media image bundle. I have 3 view modes on the node type that display my media image with 3 separate image styles. Since my media bundle can only contain 1 image style per view mode, that means I also have to create a view mode for each image style on the media bundle.

When I add a node type Events with another media field that also needs a separate image style for it's images, I need to create yet another view mode on the media bundle for this.

Having a formatter like the one in #45 / #49 means I don't have to create 4 view modes on the media bundle to do this, which seems a lot easier.

#49 I like it!

phenaproxima’s picture

Another Wednesday, another IRC meeting. And in this meeting, @seanB convinced me that #45/#49 is the way to go:

  • My main reason for initially supporting a view mode-based approach was because it's virtually all configuration and doesn't have to clear any technical barriers. Thus keeping us from overrunning our deadline.
  • However, the UX of view modes, as @seanB pointed out, is deeply sub-optimal. Core can, and should, do better.
  • A generic, reusable formatter like @marcoscano proposed would be quite useful to far more than just Media. It is a problem that has been solved several times before in contrib -- why, just this week, I have been working with Entity Embed's implementation of a similar concept. The problem of "I need to display part of a referenced entity as though it were part of the referencing entity" pops up many times in the course of normal Drupal development. This is core's opportunity to solve that problem.
  • In this engineer's opinion, the UX of such a formatter beats view modes by an order of magnitude.
  • As @marcoscano proved in #45, the technical barrier is not very high at all. The patch needs work (as noted), but it is relatively short and coherent.

For all these reasons, I think I'm in agreement with my compatriots that implementing a new, generic formatter is what we should do.

phenaproxima’s picture

Also as discussed in IRC, we need to get a framework manager opinion about this approach. So I'm tagging this issue for that.

seanB’s picture

Issue tags: +Needs usability review

Tagging for usability review...

dawehner’s picture

I get that there is a learning curve, but we should be teaching people scalable site building in my opinion.

Yeah I think the trick that you get a nice formatter by default without any further interaction. The inline settings one certainly removes the huzzle of having a gazillion levels of view modes. On a last project I've been we first went with view modes everywhere, but it turns out, they have a certain overhead when it comes down to rapid prototyping.
Switching to the inline settings one made our life much easier.

Bojhan’s picture

Issue tags: -Needs usability review

This was discussed on product review by Gabor, Roy, Bojhan, Adam and Andrei.

Thanks for working on this, its exciting to see soo much attention given to such a small detail. However it is key for ensuring UX parity, which obviously we want and if possible even a bit better :)

Supplying a checkbox on the view mode configuration, that hides the basefields. Is to us the right choice, and should be the default.

The only improvement we want, is to have a better label for this. Many users will not know what basefields are. Take a look at https://www.drupal.org/docs/develop/user-interface-standards/checkboxes for best practices.

marcoscano’s picture

Thanks @Bojhan for the feedback! Even if we still want to hear additional feedback from framework managers, let's get this moving.

Here is an improved patch from #45. I discussed with seanB this morning how to deal with empty / default values for the field names and formatters, and we kind of agreed that a good approach could be:

- "Show basefields" is off by default (we probably need a better name for it)
- If a user selects this formatter, but does not configure which field / formatter they want and click save, we save the first field and the first formatter available on the target entity. This way even if the fields on the target entity change, the next time the user comes to this form, the information will be consistent to what they saw when saving.
- We should update the settings summary to reflect this information (this is still to be implemented)
- To be discussed: what should we do when there are only basefields on the target entity? With media this shouldn't happen because media needs a source field, but if this is to be used as a generic formatter, we should deal with that case too. Maybe we disable the checkbox, forcing it to be off?

So the next steps here would be:
- Review the code / refactor when needed
- Decide on a better label for the checkbox
- Implement the summary settings
- Decide how to deal with target entities without non-basefields (and implement it)
- Write tests

seanB’s picture

Thnx @marcoscano! Some ideas to move forward:

  • The label for the checkbox could be 'Show all fields'? It's true that this leaves the question for users: Which fields are shown when I don't select this? I'm not sure if this would be confusing or not, users probably won't care about this. If the field you want is not shown, just use the checkbox and al fields are available.
  • Maybe we should also add the label as an option in all situations. This way the label is selected by default when you have no custom fields.
seanB’s picture

Title: Re-create image and file field-like formatters on top of media entity » Create generic entity reference formatter to show fields from referenced entity
Issue summary: View changes

Ok, just did a first review. A lot of supernits. Sorry about that. Overall this looks really good and useful! Also updating the IS and title since we agreed to build this as a generic entity reference formatter. We will probably need a followup to implement default config for file/image media types when this gets in.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    + * Plugin implementation of the 'link' formatter.
    

    Probably a copy / paste thing, but this is not the 'link' formatter.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +   * Constructs a FieldFormatterWithInlineSettings object.
    

    This is a EntityReferenceInlineSettingsFormatter object.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +   * Gets list of supported fields.
    

    Gets a list ... or 'Get the list' ...

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +   * Get all available formatters by loading available ones and filtering out.
    

    Filtering out what exactly? Maybe we could just say: 'Get all available formatters for a field storage definition.'?

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +   * Ajax submit callback for field name change.
    

    I think this is not a submit callback but onChange. We could just say: 'Ajax callback for field name change.' This seems to be the standard in core to document Ajax callbacks.

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +  public static function onFieldNameChange(array $form, FormStateInterface $form_state) {
    ...
    +  public static function onFormatterTypeChange(array $form, FormStateInterface $form_state) {
    ...
    +  public static function onShowBasefieldsChange(array $form, FormStateInterface $form_state) {
    

    These methods seem to do the same thing, can we just merge them into a generic ajax callback that updates the settings in the form?

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +   * Ajax submit callback for formatter type change.
    ...
    +   * Ajax submit callback for "show basefields" setting change.
    

    Same here, remove 'submit' from the comment.

  8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +    // Name of the field this formatter is currently displaying.
    

    This comment doesn't seem to make sense?

  9. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +    $formatted_field_name = $this->getSettingFromFormState($form_state, 'field_name');
    

    The var name confused me, it's not a formatted field name, it's the key of the field to format. Maybe $selected_field_name or something? I don't know.

  10. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +    // If the field selected is a basefield but the show_basefields has just
    +    // been unchecked, we need to reset the defaults.
    

    Just as a reminder. If we change the label 'Show basefields' we should probably change the var names as well. Good to see this use case is handled though.

  11. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +    $formatter_options = $this->getAvailableFormatterOptions($field_storage);
    

    Can we move this line to the section of the code where it is actually being used? It also seems to be used only conditionally so we could move it into the if statement.

  12. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +      '#title' => $this->t('Show basefields'),
    

    Rename. Added a suggestion in my previous comment but I am open to other suggestions :)

  13. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +        'callback' => [get_called_class(), 'onShowBasefieldsChange'],
    ...
    +      '#submit' => [[get_called_class(), 'rebuildSubmit']],
    ...
    +      // Note: We cannot use ::foo syntax, because the form is the entity form
    +      // display.
    ...
    +        'callback' => [get_called_class(), 'onFieldNameChange'],
    ...
    +      '#submit' => [[get_called_class(), 'rebuildSubmit']],
    ...
    +        // Note: We cannot use ::foo syntax, because the form is the entity form
    +        // display.
    ...
    +          'callback' => [get_called_class(), 'onFormatterTypeChange'],
    ...
    +        '#submit' => [[get_called_class(), 'rebuildSubmit']],
    

    We could use static::class and probably remove the comments.

  14. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +   * Gets entity view display for a bundle.
    ...
    +  protected function getViewDisplay($bundle_id) {
    

    Get the entity view display for a bundle.

    This to me does not accurately describe what going on though. We get or create a custom EntityViewDisplay from the formatter settings. Maybe rename the method to getFormatterEntityViewDisplay and move the code to create the EntityViewDisplay for the formatter in a separate method?

  15. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +
    

    Useless newline.

  16. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +      $display = EntityViewDisplay::create([
    +        'targetEntityType' => $this->fieldDefinition->getSetting('target_type'),
    +        'bundle' => $bundle_id,
    +        'status' => TRUE,
    +      ]);
    +      $display->setComponent($this->getSetting('field_name'), [
    +        'type' => $this->getSetting('type'),
    +        'settings' => $this->getSetting('settings'),
    +        'label' => $this->getSetting('label'),
    +      ]);
    +      $this->viewDisplay[$bundle_id] = $display;
    

    As mentioned, move the creation of the EntityViewDisplay to a separate method createFormatterEntityViewDisplay?

  17. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,453 @@
    +    // @TODO If there are no values set, we should inform the user that the
    +    // first field and first formatter will be used when saving this
    +    // configuration.
    

    Do we inform the user that the first field/formatter will be used, or just show the summary of the settings that will be saved (fetched from the first field/formatter)? I think we agreed to do the latter but just checking :).

marcoscano’s picture

Assigned: phenaproxima » Unassigned
Status: Needs work » Needs review
FileSize
16.94 KB
11.48 KB

Thanks @seanB for reviewing!

I like the idea of always showing the label. This also solves what to show in the summary when there is no selection :) The only issue I found is that I wasn't able to implement a generic way of deciding wether the label is called 'name' or 'title', but I'm sure there's a way of doing that.

For the naming of the checkbox, I agree that "Show all fields" is better. Maybe we could include a description to further clarify that with examples? (suggestion in the patch)

I also have moved the checkbox to be shown after the field_name dropdown, and not before.

All points from #58 were addressed, except:

6: onFieldNameChange and onFormatterTypeChange return different parts of the form. Do you mean we should have a single callback and detect which part of the form to return based on the triggering_element? I thought having a dedicated callback for each element triggering the ajax would be clearer, but I don't have strong feelings about that.
10: I believe the confusion would exist only for end users. Anyone looking at the code probably knows what a basefield is, and naming this setting like that is actually more accurate IMHO. I think if we only change the label is enough.

Next steps:
- #58-6, #58-10
- Figuring out how to detect if the label basefield is 'name' or 'title'
- More reviews
- Reach consensus on naming / UX possible issues
- Write tests

Status: Needs review » Needs work

The last submitted patch, 59: 2831943-59.patch, failed testing. View results

marcoscano’s picture

Status: Needs work » Needs review

Reviews are still welcome at this point

marcoscano’s picture

Assigned: Unassigned » marcoscano

I am going to start working on the tests for this, as part of Drupal Summer Barcelona sprints.

marcoscano’s picture

I knew there should be a better way of doing that :)

Will be working on the tests next.

The last submitted patch, 45: 2831943-45.patch, failed testing. View results

The last submitted patch, 56: 2831943-56.patch, failed testing. View results

marcoscano’s picture

This should fix existing tests

seanB’s picture

Looked at the interdiff and patch for #59. Confirming everything of #58 was addressed except nr 6, 10 and 17. I'm ok with keeping #58.10 as is. This then only leaves 6 and 17. Below some more feedback regarding #58.17. Also some extra remarks/questions.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,459 @@
    +    $bundle_info = \Drupal::service('entity_type.bundle.info');
    

    We should inject this service.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,459 @@
    +        \Drupal::service('entity_field.manager')->getFieldDefinitions($entity_type_id, $value)
    ...
    +    }
    

    The service is injected in the constructor. *edit: Already fixed in #63.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -175,7 +176,7 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +   * Get all available formatters for a field storage definition..
    

    Remove the extra dot at the end.

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -267,55 +268,52 @@ public static function rebuildSubmit(array $form, FormStateInterface $form_state
    +      '#description' => $this->t('Allows basefields to be selected, such as author, language, creation date, etc.'),
    

    Are basefields mentioned somewhere else in the interface. If sitebuilders are not familiar with the concept, I don't think this is the place to start. Maybe call them default fields, since the other fields are created custom by the sitebuilder?

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -390,45 +387,51 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +      $summary[] = $this->t('Field not configured. The entity label will be used.');
    

    Do we know for sure the label is always the first field in the list? Shouldn't we try to get the first option with the same logic as getAvailableFieldNames(FormStateInterface $form_state).

    Now I'm looking at it, can't we just pass a boolean argument $show_basefields to getAvailableFieldNames()? This makes it easier to reuse.

    And do we have to mention that the field is not configured? Does it matter?

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -390,45 +387,51 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
           $summary[] = $this->t('Formatter %type used.', ['%type' => $type]);
    ...
    -      // @TODO Change this to inform the name of the first formatter.
           $summary[] = $this->t('Formatter not configured. The first formatter will be used.');
    

    Same here, does it matter it is not configured? Can't we just fetch the name of the first formatted and show 'Formatter %type used.'. I think this TODO was valid.

marcoscano’s picture

Issue summary: View changes
FileSize
21.05 KB
9.82 KB

This is still failing EntityReferenceFormatterTest::testAccess but I'm uploading it so we can move on with further reviews.

Thanks @seanB for reviewing!

About #67-6 and #67-7, I believe the label would be always the first one because we are adding it at the very beginning of getAvailableFieldNames(), and I thought that this would be enough for avoiding having to duplicate that logic inside settingsSummary(). But it's true that it's probably more robust to detect the first field and the first formatter, so I have changed to that.

Remaining:
- #58-6
- More reviews
- Reach consensus on naming / UX possible issues
- Fix tests / Write tests

marcoscano’s picture

Assigned: marcoscano » Unassigned
Issue summary: View changes
FileSize
26.19 KB
5.69 KB

Let's see what's the testbot opinion about this.

tstoeckler’s picture

I was thinking about the whole show_basefields thing and was wondering how we could avoid that.

I played around with this a bit and I then thought about FieldDefinitionInterface::isDisplayConfigurable(). That is a boolean on field definitions that we have to configure whether or not a field shows up in the Manage display section of Field UI. So one possibility would be to remove the show_basefields setting and instead always limit the list of fields to those that have isDisplayConfigurable() set to TRUE. That automatically includes any fields created in the UI plus any that have been explicitly "whitelisted" by the entity type.

Concretely, in contrast to the current patch this would additionally expose a number of fields (assuming the show_basefields setting is set to FALSE). Here's an exhaustive list with all entity types in core:

  • Aggregator feed
    • Checked (i.e. when it was last checked)
    • URL
  • Aggregator feed item
    • Link
    • Author
    • Posted on
  • Contact message
    • Message (yes, although it seems rather far-fetched to use contact messages in an entity reference field, if you were to do that you would surely expect to be able to select the actual message as a field, which is impossible with the current patch)
  • Thumbnail
  • Media
    • Thumbnail
    • Authored by
    • Authored on
  • Taxonomy term
    • Description

Additionally, if Language module is installed, all translatable entity types would have an additional Language field as an option and with the Content Moderation module installed, all revisionable entity types (for now only Node and Custom Block, but hopefully more in the future) would have Moderation state as an additional option.

Also, we would still need to manually add the label key field to the list of fields, just like we do now, because of the weirdness of how entities are rendered on their own page or not, the label/title/... fields cannot be made configurable in the UI (yet, see also #2353867: Title does not appear as a field in Manage Display for more info on that).

But apart from the label fields (and any extra fields that the module defines, but I mean those are weird anyway...) I like about my suggestion that the list of fields that are available for selection automatically match those fields that show up under that entity type's Manage display page and that we respect the entity type's decision on which fields make sense for configuration in the UI.

Thoughts?

dawehner’s picture

I played around with this a bit and I then thought about FieldDefinitionInterface::isDisplayConfigurable(). That is a boolean on field definitions that we have to configure whether or not a field shows up in the Manage display section of Field UI. So one possibility would be to remove the show_basefields setting and instead always limit the list of fields to those that have isDisplayConfigurable() set to TRUE. That automatically includes any fields created in the UI plus any that have been explicitly "whitelisted" by the entity type.

I really like this idea! Especially with the additional bit of including the label it feels exactly what we want.

phenaproxima’s picture

So one possibility would be to remove the show_basefields setting and instead always limit the list of fields to those that have isDisplayConfigurable() set to TRUE

Absolutely +1 for this idea. We should avoid exposing the concept of "base fields" to users if we can avoid it. But we do want to expose certain base fields for sure -- in the case of Media items, the thumbnail is a base field and it is most definitely something that should be displayable.

So yes -- I think we should expose all viewable base fields (and standard configurable fields), and leave it at that.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks, @marcoscano and @seanB! This is great progress.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    + * Plugin implementation of the 'entity_reference_inline_settings' formatter.
    

    I'm not sure how I feel about naming this formatter "inline_settings". That seems kind of vague to me. What about "referenced_entity_field" or something?

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +   * Entity view display.
    

    Should be "The entity view display".

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +  protected function getFieldDefinition(FieldStorageDefinitionInterface $field_storage_definition) {
    +    return BaseFieldDefinition::createFromFieldStorageDefinition($field_storage_definition);
    +  }
    

    I don't understand why we need this method at all. Can't we simply call BaseFieldDefinition::createFromFieldStorageDefinition() when we need to? What value is added by this wrapper method?

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +   *   If FALSE, all basefields except for the label will be skipped.
    

    Nit, possibly supernit: It's "base fields", not "basefields".

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +    $field_names = [$label_key => $this->entityFieldManager->getBaseFieldDefinitions($entity_type_id)[$label_key]->getLabel()];
    +    $target_bundles = $this->fieldDefinition->getSetting('handler_settings')['target_bundles'] === NULL ? array_keys($this->entityTypeBundleInfo->getBundleInfo($entity_type_id)) : $this->fieldDefinition->getSetting('handler_settings')['target_bundles'];
    

    Kinda nitpicky, but can these lines be split up or otherwise made a little more readable? They're very hard to grok right now. We should also check that the $entity_type even *has* a label key, rather than blindly assume that it does. Because if it doesn't, this will fatal :)

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +  public static function isApplicable(FieldDefinitionInterface $field_definition) {
    +    $entity_type = \Drupal::entityTypeManager()
    +      ->getDefinition($field_definition->getTargetEntityTypeId());
    +    return $entity_type->isSubclassOf(FieldableEntityInterface::class);
    +  }
    

    Is this necessary? This is a field formatter, which means that it will automatically be joined to a fieldable entity type. So I'm not sure what this gains us.

    Not only that, but I think this might be wrong -- getDefinition() will return EntityTypeInterface, which never intersects FieldableEntityInterface. So as far as I can tell, this isApplicable() method will *never* return true...? Please tell me I'm crazy.

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +    }, array_combine(array_keys($formatters), array_keys($formatters)));
    

    Can we do the array_keys() call once, before we get here, as a micro-optimization?

  8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +    $filtered_formatter_instances = array_filter($formatter_instances, function (FormatterInterface $formatter) use ($field_definition) {
    +      return $formatter->isApplicable($field_definition);
    +    });
    

    Maybe a bit of a pipe dream, but IMHO we should add this to FormatterPluginManager directly if it doesn't already exist there. Could be useful! But I definitely don't want to block this patch on that. Could be a follow-up.

    Also...isn't isApplicable() a static method?

  9. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +   * @param array $form
    +   *   The form.
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   *   The form state.
    +   *
    +   * @return array
    +   *   The replaced form substructure.
    

    From what @tim.plunkett tells me, we don't have to document params and returns for AJAX callbacks, since they all follow a standard format.

  10. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +        'above' => $this->t('Above'),
    +        'inline' => $this->t('Inline'),
    +        'hidden' => '- ' . $this->t('Hidden') . ' -',
    +        'visually_hidden' => '- ' . $this->t('Visually Hidden') . ' -',
    

    I wish we could re-use this from somewhere...

  11. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +        'view_mode' => '_custom',
    

    I think this is a constant somewhere that we can reuse (EntityViewModeInterface?)

  12. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +    $display = EntityViewDisplay::create([
    +      'targetEntityType' => $this->fieldDefinition->getSetting('target_type'),
    +      'bundle' => $bundle_id,
    +      'status' => TRUE,
    +    ]);
    

    Er...can't we just call entity_get_display()? It already goes through the "create if not exists" rigmarole so we don't have to.

  13. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +      $show_basefields = $this->getSetting('show_basefields');
    +      $field_name_options = $this->getAvailableFieldNames($show_basefields);
    +      $target_entity_type_id = $this->fieldDefinition->getSetting('target_type');
    +      $field_storage_definitions = $this->entityFieldManager->getFieldStorageDefinitions($target_entity_type_id);
    +      $field_name = key($field_name_options);
    +      $summary[] = $this->t('The field %field_name will be used by default.', ['%field_name' => $field_name]);
    

    $field_storage_definitions doesn't appear to be used.

  14. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +      $summary[] = $this->t('Formatter %type used.', ['%type' => $type]);
    

    If $type is the formatter's plugin ID, we're exposing implementation details to user. I'd rather we displayed the human-readable label of the formatter instead.

  15. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -0,0 +1,494 @@
    +      $summary[] = $this->t('Formatter not configured. The first formatter will be used.');
    

    I'm not sure how I feel about just defaulting to the first available formatter. IMHO, we should simply render nothing if no formatter is configured, rather than do something that might make the site builder go WTF. I defer to the UX team on this, though.

We are also going to need JS tests, I think, of the configuration workflow for this formatter.

marcoscano’s picture

FileSize
24.8 KB
5.58 KB

#70: +1! Thanks for pointing that out, it will probably be the simplest and most effective solution.

Patch updated to take that into account.

marcoscano’s picture

@phenaproxima cross-posted! sorry the patch numbering is now incorrect :)

marcoscano’s picture

Status: Needs work » Needs review
FileSize
23.59 KB
8.34 KB

Thanks @phenaproxima for reviewing!

Re #73:

1: Pending
I'm not against renaming this, but once it includes refactoring in a lot of places, ideally we should get consensus before doing that. Options I can think of so far include:
- entity_reference_inline_settings
- entity_reference_referenced_entity_field
- entity_reference_referenced_field
- entity_reference_rendered_field
- entity_reference_child_entity_field
- entity_reference_target_field
- other options?
I think I personally prefer "entity_reference_rendered_field" or "entity_reference_target_field" but I don't really have strong opinions on this.

2: Done

3: Done

4: We don't have anything related to base fields anymore :)

5: Done

6: Pending

7: Done

8: Let's try to restrict the scope of this ticket :)

9: Done

10: Pending. I couldn't find how, but if anyone knows, please let me know!

11: Pending. I couldn't find any. \Drupal\views\Plugin\views\field\EntityField::getFormatterInstance also uses '_custom'.

12: Pending. Couldn't make it work with your suggestion.

13: Yes, it's being used some lines down in the next if statement :)

14: Done

15: Done. I have removed this case entirely because it was never going to be reached anyways. I can't think of a legitimate use case where a user can select a field name and save the configuration with the formatter setting empty. So they are either both selected or both empty, in which case this default fallback was redundant.

So, carrying over the next steps:
- Finish addressing the pending points from #73
- Finish addressing #58-6
- Improve / review tests
- Get final blessing from UX / Framework managers

phenaproxima’s picture

Status: Needs review » Needs work

Thanks, @marcoscano! This continues to get better and better.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -31,7 +31,7 @@
    +   * The entity view displa.
    

    Whoops, it's "displa" now :)

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -214,9 +211,9 @@ protected function getAvailableFormatterOptions(FieldStorageDefinitionInterface
    +      return $formatter::isApplicable($field_definition);
    

    Ah, here's the thing -- I was thinking that, since isApplicable() is static, we don't need to instantiate the formatters *at all*. We can simply loop through the plugin definitions and use the 'class' element to call isApplicable, then use the 'label' element when building $options.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceInlineSettingsFormatter.php
    @@ -420,27 +381,28 @@ public function settingsSummary() {
    +      $summary[] = $this->t('Field "%field_name" displayed.', ['%field_name' => $field_name]);
    

    It seems to me that $field_name is the machine name of the field. Could we get the full definition of the target field and use its human label instead?

yoroy’s picture

FileSize
96.57 KB

I tested #69 in simplytest and added a media reference field to the article content type. In the display options I could only display as a link to entity, no image was available to display.

I assume this part of the UI is part of what's happening here (correct me if I'm wrong of course)

- I think we want to use "Media types" instead of "bundles"
- It was unclear to me what and where the sorting would be applied to. Is this about multi-value fields?

yoroy’s picture

FileSize
36.89 KB

False alarm on not finding a way to actually show the referenced media image.

This select list has tripped me up before. I think it's the default "label" that makes me ignore it. Anyway, Field UI--, carry on! :)

marcoscano’s picture

Status: Needs work » Needs review
FileSize
4.68 KB
22.79 KB

Addressed here: #73-6, #77-1, #77-2 and #77-3.

Pending:
- #58-6: Decide between multiple or single ajax callbacks
- #73-1: Naming the formatter
- #73-8: Create a follow-up?
- #73-10: Re-use label options
- #73-11: Find constant for 'view_mode' => '_custom'
- #73-12: Use entity_get_display()
- Review/improve tests
- Get final UX / Framework managers approval

marcoscano’s picture

Just a reminder (because otherwise I'll probably forget this later), when / if this issue gets in, the Field Formatter contributors should get attribution for this work.

According to git log, the following people worked on the file FieldFormattersWithInlineSettings:

dawehner
sanja_m
slashrsm
Leksat
webflo

But probably the maintainers could double-check if this information is accurate.

marcoscano’s picture

Discussed with @phenaproxima in IRC that a JS test wouldn't hurt here, even if other entity_reference functional tests for formatters are WebTests. So this new patch includes a basic JS test that goes to the display config page, clicks around and configures the formatter.
Please let me know if we should cover something I didn't think of in this test.
Also, I couldn't avoid using ::assertWaitOnAjaxRequest(), if anyone has some ideas on how to avoid that, I'd appreciate.

Pending:
- #58-6: Decide between multiple or single ajax callbacks
- #73-1: Naming the formatter
- #73-8: Create a follow-up?
- #73-10: Re-use label options
- #73-11: Find constant for 'view_mode' => '_custom'
- #73-12: Use entity_get_display()
- Review/improve tests
- Get final UX / Framework managers approval

phenaproxima’s picture

This addresses the following points from #73:

10. I changed EntityViewDisplayEditForm::getFieldLabelOptions() from a protected instance method to a public static method so we could reuse the options. I'm not sure if this constitutes an API change, but if it is, it's both non-disruptive and useful, so I think it's justified.

11. Turns out the constant is defined on EntityDisplayBase, so I used that.

12. Done. Was able to ditch an entire method by doing that! :)

Remaining tasks:

- #58-6: Decide between multiple or single ajax callbacks
- #73-1: Naming the formatter
- #73-8: Create a follow-up?
- Review/improve tests
- Get final UX / Framework managers approval

chr.fritsch’s picture

Status: Needs review » Needs work

I tested patch #83 and it introduced an issue with the display. Now it renders the media item in "full content", instead of just the configured field.

I also checked #82 and it worked smoothly

phenaproxima’s picture

I think I know what's up with this, and it leads me to a question: I'm not sure we need to bother with view displays or display modes at all. Why not simply use EntityViewBuilderInterface::viewField()?

$this->entityTypeManager->getViewBuilder($target_entity_type)->viewField($target_entity->get($target_field), $formatter_settings)

If we do this, we should also change isApplicable() to ensure that the referenced entity type has a view builder at all.

effulgentsia’s picture

Get final ... Framework managers approval

I'm in favor of the concept of this issue: I think it makes a lot of sense to have an ER formatter that can display a single field from the referenced entity, without needing an extra view mode on the referenced entity type to do it. However, I'd still like to review the patch itself before removing the "Needs framework manager review" tag. Please continue refining it in the meantime.

marcoscano’s picture

Assigned: Unassigned » marcoscano

Working on it

xjm’s picture

For context for #86, a patch can still be marked RTBC if the only thing blocking it is a "Needs framework/release/product manager review" tag, since those are committer reviews. Sounds like we have signoff on the direction mentioned in #86. Thanks everyone!

marcoscano’s picture

Assigned: marcoscano » Unassigned
Status: Needs work » Needs review
FileSize
32.66 KB
22.85 KB

Another round of refinement :)

In this patch:
- The tests were expanded and now also catch the regression introduced by #83, which I reverted here. (I now have the feeling that the Kernel test might be redundant, but I left it there anyways)
- While creating the test I found a bug that if the user selected the formatter but never configured it, our defaults weren't applying correctly when rendering the field. Fixed this too.
- Applied the suggestion from #85, to directly use EntityViewBuilderInterface::viewField() and remove two methods. Thanks @phenaproxima!
- #58-6: We discussed this on IRC and agreed that there are advantages for leaving one callback per element
- #73-1: Done, renamed the plugin to entity_reference_referenced_entity_field
- #73-8: @phenaproxima is going to create a follow-up, if necessary
- I also abstracted our logic of "when no field configured, default to the first displayable field" to a separate method, because we were having to repeat it in several places.

I believe with this (and assuming no other test failures occur), the remaining tasks are only to address the reviews yet-to-come. Thanks everyone!

marcoscano’s picture

There are a couple of remarks I wanted to get feedback on as well:

1) The implementation allows for "chainable" formatters, in the sense that it's possible to render OOTB, for example, the picture of the author of a media item referenced from a node. Manual testing for this functionality works, but I couldn't make a test where the schema checker doesn't complain. Do we want test coverage for this use case? If so, any hints on how to avoid the schema checker failing on the chained formatters missing schema definitions?

2) Would it make sense to implement a media-specific customization of this formatter, so that when users add a media field to an entity, they wouldn't need to adjust the display settings? By default the referenced entity's label is being displayed, but all media entities will have a source field, so I think it could make sense to try to show this field by default, if nothing was configured.

Status: Needs review » Needs work

The last submitted patch, 89: 2831943-89.patch, failed testing. View results

marcoscano’s picture

Status: Needs work » Needs review
FileSize
32.66 KB
1.21 KB
Wim Leers’s picture

First: thank you, @marcoscano, for the massive push forward that you are giving to this issue! And thanks to @seanB for extensive reviews :)

I really like this idea!

+1! @tstoeckler+++++++++++++++

#85:

I'm not sure we need to bother with view displays or display modes at all. Why not simply use EntityViewBuilderInterface::viewField()?

You've been misled — I remember thinking the same thing while working on Quick Edit. Check the docs of \Drupal\Core\Entity\EntityViewBuilderInterface::viewField(). You still need view modes.
EDIT: ah, this patch gets around it by declaring a custom mode, and allowing the formatter settings to be configured.


  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceReferencedEntityFieldFormatter.php
    @@ -0,0 +1,413 @@
    +      'field_name' => '',
    +      'type' => '',
    

    These defaults seem to make little sense.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceReferencedEntityFieldFormatter.php
    @@ -0,0 +1,413 @@
    +      'label' => 'above',
    

    If we're rendering a single field, why would we want that field label to show up? I'd think that the default would not be above but hidden?

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceReferencedEntityFieldFormatter.php
    @@ -0,0 +1,413 @@
    +      $field_name = key($this->getFieldName());
    +      $formatter_type = key($this->getFormatterType($field_name));
    

    Using key() makes me nervous, because it depends on the array cursor.

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceReferencedEntityFieldFormatter.php
    @@ -0,0 +1,413 @@
    +   *   List of fields that are supported keyed by field machine name.
    

    What are the values then?

  5. +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceAdminTest.php
    @@ -401,6 +404,7 @@ public function testAvailableFormatters() {
    +      'entity_reference_referenced_entity_field',
    

    Why not just entity_reference_field?

  6. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceFormatterTest.php
    @@ -402,6 +407,128 @@ public function testLabelFormatter() {
    +  public function testRenderReferencedEntityFieldFormatter($label_option, $expected_output) {
    +
    +    $field_storage = FieldStorageConfig::create([
    
    +++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceReferencedEntityFieldFormatterTest.php
    @@ -0,0 +1,150 @@
    +    ]);
    +
    +  }
    ...
    +    $assert_session->pageTextNotContains('Child test node');
    +
    +  }
    

    Supernit: extraneous \n.

  7. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceFormatterTest.php
    @@ -402,6 +407,128 @@ public function testLabelFormatter() {
    +    $field_storage->save();
    ...
    +    $field_config->save();
    

    Nit: $field_storage can be removed, because nothing uses it. Same for $field_config.

My #1 concern is still the same: by allowing a field to be rendered not using an existing view mode, but instead allowing the default formatter for that referenced field to be configured, that means you'll have to duplicate all of that work when you're either viewing Media items directly, or when you're embedding Media items in your text (via the Text Editor + CKEditor).

I'm not going to hold this up, but it's a concern that I fear will come to bite us further along in the process.

marcoscano’s picture

@Wim Leers thanks for reviewing!

1: Would you mind elaborating a bit more? Is it the empty string the issue or the lack of an actual value? Other formatters in core seem to do the same, and I can't really think of an alternative way of doing it. Thanks!
2: Done.
3: Done.
4: Done.
5: Done.
6: Done.
7: Done.

Wim Leers’s picture

#94.1 Oh I didn't realize other formatters do the same. Never mind that then! Changes look good.

dawehner’s picture

These defaults seem to make little sense.

Could/should we use NULL to make an explicit hint, that there is no value selected?

You've been misled — I remember thinking the same thing while working on Quick Edit. Check the docs of \Drupal\Core\Entity\EntityViewBuilderInterface::viewField(). You still need view modes.
EDIT: ah, this patch gets around it by declaring a custom mode, and allowing the formatter settings to be configured.

So could/should we make it easier to create custom view modes? Could EntityReferenceReferencedEntityFieldFormatter maybe use internally an actual view mode, but we just autogenerate it under the hood, save it as well as use it, if available? I'm not sure how much value this would add though, because we want to keep the UX of defining settings inline, as everything else is a bit of a pain.

If we're rendering a single field, why would we want that field label to show up? I'd think that the default would not be above but hidden?

+1 for removing the label. To be honest the label is most of the time hidden anyway on actual sites, its just not what the endusers really expect to see.

Why not just entity_reference_field?

What about entity_referenced_field to distinct from a entity_reference_field itself?

marcoscano’s picture

Thanks @dawehner and @Wim Leers for the feedback!

Do you have any thoughts on #90 ?

phenaproxima’s picture

  1. +++ b/core/config/schema/core.entity.schema.yml
    @@ -362,3 +362,19 @@ field.formatter.settings.entity_reference_label:
    +  label: 'Entity reference referenced entity field formatter display format settings'
    

    This is a bit long. Can it just be "Rendered field from referenced entity settings"?

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +      $field_name = $this->getFieldName()['machine_name'];
    +      $formatter_type = $this->getFormatterType($field_name)['machine_name'];
    +      $formatter_settings = [
    +        'type' => $formatter_type,
    +        'settings' => $this->getSetting('settings'),
    +        'label' => $this->getSetting('label'),
    +      ];
    

    It seems like we could do all this once, before we enter the loop -- am I mistaken?

    Also, we should check that $entity->hasField($field_name) before trying to render it, if it doesn't exist. This could be the case for an entity reference field that references multiple bundles, which have different fields.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +  protected function getAvailableFieldNames() {
    

    This essentially returns a list of select options, so can we rename the method? Something like getAvailableFieldOptions()?

    Also, this method is a bit heavyweight. Can the results be cached ($this->availableFieldOptions or similar)?

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +    $target_bundles = empty($this->fieldDefinition->getSetting('handler_settings')['target_bundles']) ? array_keys($this->entityTypeBundleInfo->getBundleInfo($entity_type_id)) : $this->fieldDefinition->getSetting('handler_settings')['target_bundles'];
    

    I think we could shorten this a bit:

    $settings = $this->fieldDefinition->getSetting('handler_settings');
    $target_bundles = $settings['target_bundles'] ?: array_keys($this->entityTypeInfo->getBundleInfo($entity_type_id));
  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +    foreach ($target_bundles as $value) {
    

    Can $value be renamed $bundle, for clarity?

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +      $bundle_field_names = array_map(
    +        function (FieldDefinitionInterface $field_definition) {
    +          if ($field_definition->isDisplayConfigurable('view')) {
    +            return $field_definition->getLabel();
    +          }
    +        },
    +        $this->entityFieldManager->getFieldDefinitions($entity_type_id, $value)
    +      );
    

    I find the use of array_map() a little awkward here; it seems like it would be a little more readable as a simple foreach loop.

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +    return $form['fields'][$form_state->getStorage()['plugin_settings_edit']]['plugin']['settings_edit_form']['settings'];
    

    I think we could replace $form_state->getStorage()['plugin_settings_edit'] with just $form_state->get('plugin_settings_edit').

  8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +    return $form['fields'][$form_state->getStorage()['plugin_settings_edit']]['plugin']['settings_edit_form']['settings']['settings'];
    

    Same here.

  9. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +  public static function rebuildSubmit(array $form, FormStateInterface $form_state) {
    

    Aren't submit functions supposed to take $form by reference?

  10. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +    $selected_field_name = $this->getSettingFromFormState($form_state, 'field_name');
    

    A maybe-bad idea -- we could simply extend getSetting() to take an optional FormStateInterface object, and if that FormStateInterface is passed, it gets the setting from there. Otherwise it just calls parent::getSetting(). Slick, eh? =P

  11. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +    $form['label'] = [
    +      '#type' => 'select',
    +      '#title' => $this->t('Label'),
    +      '#options' => EntityViewDisplayEditForm::getFieldLabelOptions(),
    +      '#default_value' => $this->getSettingFromFormState($form_state, 'label'),
    +    ];
    

    If we're hiding the referenced field's label -- an idea which I support -- I guess we can remove this entirely.

  12. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +    if ($selected_field_name && !empty($field_storage)) {
    

    Why the empty check on $field_storage? It seems to me that if $field_storage is empty, that is a full-scale error condition that we should not be papering over.

  13. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +    $field['label'] = !empty($field_name_options[$field_name]) ? $field_name_options[$field_name] : NULL;
    

    I think we should consider it an error condition if $field_name_options[$field_name] is not set. I can see no conceivable reason why that would be a legitimate scenario.

  14. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,416 @@
    +      if (empty($field_storage_definitions[$field_name])) {
    +        return $formatter;
    +      }
    

    Again, I don't understand why $field_storage_definitions[$field_name] would ever be empty. We should treat that as an error condition.

marcoscano’s picture

Thanks @phenaproxima for reviewing!

Re #98:

1: Done.
2: Done.
3: Done.
4: I'm not sure about that. I added the empty() check because there are scenarios where the result from $this->fieldDefinition->getSetting('handler_settings') does not have ['target_bundles'] and it fatals if we use it directly. (For example, what we do in the kernel test)
5: Done.
6: Done.
7: Done.
8: Done.
9: Done.
10: Done.
11: Oh, I had understood that we were only making the _default value_ to be hidden, but not remove it entirely. I can see legitimate scenarios where the host label isn't wanted but the referenced one is.
12: Done.
13: Done.
14: Well in this case I don't think the check is redundant. $field_storage_definitions is probably never empty but we can't guartantee it'll always have a key matching $field_name, which is an arbitrary string passed in by the caller.

phenaproxima’s picture

Thank you, @marcoscano.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -150,7 +160,11 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +    if ($this->availableFieldOptions) {
    

    This should be isset() because otherwise, if we run through this entire function and yield an empty array, subsequent calls will run through all the heavy lifting again.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -341,20 +356,37 @@ public function settingsSummary() {
    +    if ($form_state->hasValue([
    +      'fields',
    +      $field_name,
    +      'settings_edit_form',
    +      'settings',
    +      $key,
    +    ])) {
    

    For readability, let's just set up the $key array before the calls to $form_state->hasValue() and $form_state->getValue(). That way we can just say $form_state->hasValue($key) and $form_state->getValue($key).

Regarding #98-14: $field_name is an arbitrary string passed in to a protected method (i.e., not part of the API) by calling code that can and should be expected to know what it's doing. We expect the field storage definition to exist, and if it doesn't, I don't see how that is ever anything but an error.

I won't insist on it, but my preferred approach to that kind of error handling is "garbage in, exception/fatal out". Especially in protected non-API methods. Allowing error conditions to happen is the best way to discover, trace, and fix bugs, so I'm still in favor of not doing the empty check.

marcoscano’s picture

@phenaproxima you have convinced me :)
This patch addresses all remarks from #100.
Thanks!

phenaproxima’s picture

Oh, this is looking so goooood...I had a tough time finding things to complain about!

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,440 @@
    +        $build[$delta] = $this->entityTypeManager->getViewBuilder($entity->getEntityTypeId())->viewField($entity->get($field_name), array_filter($formatter_settings));
    

    I'm not sure why we need the array_filter() call? It seems to me that everything in $formatter_settings should be passed in.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,440 @@
    +  protected function getAvailableFieldOptions() {
    

    Can we get a few more inline comments in this method explaining what's going on? It's rather dense to read through.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,440 @@
    +   *   The field formatter labels keys by plugin ID.
    

    Should be "labels, keyed by plugin ID."

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,440 @@
    +    $field_definition = BaseFieldDefinition::createFromFieldStorageDefinition($field_storage_definition);
    +    $formatters = $this->formatterPluginManager->getOptions($field_storage_definition->getType());
    +    $options = [];
    +    foreach ($formatters as $formatter_id => $formatter_options) {
    +      $formatter_definition = $this->formatterPluginManager->getDefinition($formatter_id);
    +      /** @var \Drupal\Core\Field\FormatterInterface $formatter_class */
    +      $formatter_class = $formatter_definition['class'];
    +      if ($formatter_class::isApplicable($field_definition)) {
    +        $options[$formatter_id] = $formatter_definition['label'];
    +      }
    +    }
    +    return $options;
    

    This seems rather heavyweight. Can we cache it by field type?

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,440 @@
    +  public static function rebuildSubmit(array &$form, FormStateInterface $form_state) {
    

    Sorry to do this, but can this be renamed for clarity to rebuildOnSubmit?

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,440 @@
    +      if ($formatter = $this->formatterPluginManager->getInstance($options)) {
    +        $settings_form = $formatter->settingsForm([], $form_state);
    +      }
    

    So...what are we going to do if the formatter is not instantiated for some reason? Should we throw an exception or...? Silently swallowing what looks to me like an error condition seems like potentially poor UX...

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,440 @@
    +    if ($this->getSetting('field_name')) {
    +      $summary[] = $this->t('Field "%field_name" displayed.', ['%field_name' => $field_name['label']]);
    +    }
    +    else {
    +      $summary[] = $this->t('The field "%field_name" will be used by default.', ['%field_name' => $field_name['label']]);
    +    }
    

    I don't think we need to draw a distinction between the chosen field and the default. Can/should we just collapse this logic so that it says something like "Displaying %field_name", regardless of whether a field has been chosen?

  8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,440 @@
    +    $formatter_type = $this->getFormatterType($field_name['machine_name']);
    +    if ($this->getSetting('type')) {
    +      $summary[] = $this->t('Formatter "%type" used.', ['%type' => $formatter_type['label']]);
    +    }
    +    else {
    +      $summary[] = $this->t('The "%type" formatter will be used by default.', ['%type' => $formatter_type['label']]);
    +    }
    

    Same here.

    In fact -- could we collapse the entire summary to a single sentence, like "Displaying %field_name using the %type formatter"? It would simplify this method and, in my opinion at least, be a lot clearer to users.

  9. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,440 @@
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   *   The form state.
    

    Nit: The description should be prefixed with (optional).

effulgentsia’s picture

I've looked at this patch a bit since #86, and I like it more and more the more I look at it :)

However, in discussing it with @xjm, I've become a bit more concerned with it from a product management and release management standpoint, though I still like it a lot from a framework management perspective, despite Wim's concern at the bottom of #93, which I'll write more about later. Therefore, I'm adding those tags to the issue, to discuss this more thoroughly the next time we have a call with a framework manager, product manager, and release manager all present.

I know 8.4-alpha is coming up, but I want to make sure we don't rush this into core without thoroughly considering the concerns that have been raised, such as Wim's in #93. Because once this is in core, we'll be expecting site builders to make good decisions on when to use this new formatter vs. when to use the "Rendered entity" formatter, and while there are nuanced pros/cons of each, at least on the surface, it appears like two ways of accomplishing roughly the same thing. This reminds me of how we currently have Fields displays vs. Content displays in Views, and of #2665694: [meta] Make entity view mode row style the preferred way to build a view (with feature parity for site builders) trying to steer site builders away from the former.

My biggest concern right now is that this issue is currently listed in the "Must-have" list of https://www.drupal.org/node/2825215#followup-roadmap (item #6, with its former title of "Re-create image and file field-like formatters on top of media entity."). I'd like us to consider reclassifying this issue as a "Should have" or "Could have" instead. I think the way this made it into the "Must-have" list is as an implementation for satisfying the "Allow site builders to start creating entity reference (ER) fields to Media entities instead of File fields, without compromising features or UX for content authors and site visitors." sentence in #2825215: Media initiative: Essentials - first round of improvements in core. However, if we solely use the "Rendered Entity" formatter, don't we satisfy this already? I see how this issue improves UX for site builders, but does it solve anything for content authors or site visitors? In other words, what would be the downside to shipping 8.4 without this formatter, letting it stay in contrib (https://www.drupal.org/project/field_formatter), and letting sites and distributions depend on that contrib module if they want to?

Regardless of whether or when this makes it into core, I think there's really great work in here that improves on https://www.drupal.org/project/field_formatter, and if I were building sites or distributions, I would certainly appreciate having this module available to me, so thank you!

marcoscano’s picture

FileSize
36.31 KB
13.39 KB

My 2cts about the concern raised in #103:

Although it's true that the sentence:

Allow site builders to start creating entity reference (ER) fields to Media entities instead of File fields, without compromising features or UX for content authors and site visitors

refers to "content authors and site visitors", I believe we should extend it to site builders as well. In fact, the whole effort contained in #2786785: Media in Drupal 8 Initiative is, in my opinion, most beneficial to site builders (and especially those working on smaller projects). Big development teams can already create great experiences for content authors and site visitors using what exists in contrib today, we are not really creating anything new for them. That's why I believe that improving the site builder experience as much as we can now is a must-have in the context of this media-in-core plan.

This is actually one of the reasons I think it would be nice if we could solve #90-2 as well here. In this patch I have created a proof-of-concept for that. Although it works, I'm not happy with the implementation because I couldn't find a clean way of detecting if the field has just been created or if it's an existing field being updated (in which case we shouldn't mess with the formatter settings). Any feedback on how to improve that would be appreciated.

Also, this patch addresses from #102:

1: Done
2: Done
3: Done
4: Done
5: Done
6: I agree to make the error visible, but should we thow an exception? Wouldn't be better to just show an error message and let the user choose another formatter?
7: Done
8: Done
9: Done

Thanks everyone!

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs followup

I think it's out of scope to solve #90-2 in this issue. Let's defer it to a follow-up -- it'll likely need its own UX review, implementation, and tests. There is no reason to block this issue on that.

Regarding @effulgentsia's input in #103 -- damn, that's a great point. Does it really block Media from being stable in the unlikely event that we don't have this formatter committed in time for 8.4.0? Personally, I don't think it does. #2831940: Create file field widget on top of media entity is a far more menacing issue. If it will help move things along, I'm OK with changing this issue's priority to "should-have" in #2825215: Media initiative: Essentials - first round of improvements in core. We definitely want to complete this issue because it is absolutely better UX for site builders -- but I don't think it needs to be a blocker to declaring Media stable.

All that being said, we need to get this done! From a technical perspective, apart from #102-6, I see no reason to keep this from RTBC any longer. To address that point, I think that showing an error is a perfectly good response, so let's go with that. It is the only remaining point of review before I'm ready to sign off on this.

marcoscano’s picture

Status: Needs work » Needs review
FileSize
33.26 KB
3.23 KB

I'm OK with deferring #90-2 to a follow-up.

This patch has that hook removed. (By the way, #102-6 with an error message instead of an exception was already implemented in #104, so we should be good to go)

Thanks!

seanB’s picture

Status: Needs review » Needs work

I agree this is not a blocker for Media. Besides that, it seems like a huge improvement for sitebuilders, so I do think this should get in. I've had a lot of situations where this could be useful (even back in Drupal 7 with file entity and it's view modes). I also agree with marcoscano in #104 that we should also try to create feature parity for sitebuilders if media is replacing file/image. But "should" is the magic word here.

Hard to find something but just some small things.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,455 @@
    +    // Field on the target entity this formatter is currently displaying.
    +    $selected_field_name = $this->getSetting('field_name', $form_state);
    +    if (!$selected_field_name) {
    +      // The first field is used as default in case of no value set.
    +      $selected_field_name = array_keys($field_name_options)[0];
    +    }
    

    Can't we use getFieldName() for this?

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,455 @@
    +    $field_name_options = $this->getAvailableFieldOptions();
    ...
    +      '#options' => $field_name_options,
    

    If we replace the code from the first point with $selected_field_name = $this->getFieldName() then we can directly call $this->getAvailableOptions(); here.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,455 @@
    +      $formatter_options = $this->getAvailableFormatterOptions($field_storage);
    +      $formatter_type = $this->getSetting('type', $form_state);
    +      $settings = $this->getSetting('settings', $form_state);
    +      if (!isset($formatter_options[$formatter_type])) {
    +        $formatter_type = array_keys($formatter_options)[0];
    +        $settings = [];
    +      }
    

    Same here, can we use $this->getFormatterType()?

marcoscano’s picture

Thanks @seanB for reviewing!

Re: #107:

::getFieldName() and ::getFormatterType() are not aware of what's in $form_state, because they are used in ::settingsSummary(), for example. That's why we are not using these helpers in the form constructor, where we need to carry over the values from $form_state.

marcoscano’s picture

Status: Needs work » Needs review
FileSize
33.23 KB
4.8 KB

Dumb me! we can always carry over the $form_state manually. Sorry for that.

This should address #107.

marcoscano’s picture

And now correctly using the result from the helper function...

seanB’s picture

Sorry, but I have 2 supernits. After this I will RTBC.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,451 @@
    +      $formatter_options = $this->getAvailableFormatterOptions($field_storage);
    ...
    +        '#options' => $formatter_options,
    

    Skip the var and move $this->getAvailableFormatterOptions($field_storage) directly to '#options'

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,451 @@
    +      $settings = $this->getSetting('settings', $form_state) ?: [];
    ...
    +          'settings' => $settings,
    

    Skip the $settings var as well.

marcoscano’s picture

No worries, here we go!

marcoscano’s picture

Issue tags: -Needs tests, -Needs followup
phenaproxima’s picture

Simply glorious. Glorious, I say! All I found was, essentially, nitpicks.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +  /**
    +   * The list of supported fields, prepared as options (machine_name => label).
    +   *
    +   * @var array
    +   */
    +  protected $availableFieldOptions;
    

    Nit: This type hint should maybe be string[], since it's an array of strings.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +  /**
    +   * The list of available formatters, keyed by field type.
    +   *
    +   * @var array
    +   */
    +  protected $availableFormatterOptions;
    

    Can we go into a little bit more detail about what the array looks like? As in, is it machine_name => label? If so, can the type hint be string[]?

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +   * @param string $plugin_id
    +   *   The plugin_id for the formatter.
    

    Description should be "The formatter's plugin ID."

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +   * @param \Drupal\Core\Entity\EntityFieldManager $entity_field_manager
    +   *   The entity field manager.
    +   * @param \Drupal\Core\Field\FormatterPluginManager $formatter_plugin_manager
    

    These should be type hinted to interfaces where possible.

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, EntityFieldManager $entity_field_manager, FormatterPluginManager $formatter_plugin_manager, EntityTypeManagerInterface $entity_type_manager, EntityTypeBundleInfoInterface $entity_type_bundle_info) {
    

    Not sure about $formatter_plugin_manager, but $entity_field_manager should be type hinted to the interface.

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +      /** @var \Drupal\Core\Field\FieldDefinitionInterface $field_definition */
    +      foreach ($this->entityFieldManager->getFieldDefinitions($entity_type_id, $bundle) as $field_machine_name => $field_definition) {
    

    The @var comment is unnecessary; PHPStorm should be able to figure out that getFieldDefinitions() returns a list of FieldDefinitionInterfaces.

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +  public static function onFieldNameChange(array $form, FormStateInterface $form_state) {
    

    I could be wrong, but isn't $form passed to AJAX callbacks by reference?

  8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +        drupal_set_message('Could not instantiate the formatter plugin, please choose a different formatter.', 'error');
    

    The error message should be translated.

  9. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +    $summary[] = $this->t('Displaying the field "%field_name", using the formatter "%type".', [
    

    Correct me if I'm wrong, but won't the % placeholder cause the replaced variables to be wrapped in em tags? If so, we probably don't need the quotes around %field_name and %type.

  10. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +    if (!$form_state) {
    +      return parent::getSetting($key);
    +    }
    +
    +    $field_name = $this->fieldDefinition->getName();
    +    $form_state_key = [
    +      'fields',
    +      $field_name,
    +      'settings_edit_form',
    +      'settings',
    +      $key,
    +    ];
    +    if ($form_state->hasValue($form_state_key)) {
    +      return $form_state->getValue($form_state_key);
    +    }
    +
    +    return parent::getSetting($key);
    

    Supernit: Rather than call parent::getSetting($key) twice in this method, we could simply make it so that it looks like this:

    if ($form_state) { return $stuff_from_form_state_if_present; } return parent::getSetting($key);

  11. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +   * @see \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceReferencedEntityFieldFormatter::getAvailableFieldOptions()
    

    Whoa, do we need the fully qualified class name here? Not sure what the coding standards dictate here. My preference would be just "@see ::getAvailableFieldOptions()"

  12. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +      $field_name = array_keys($field_name_options)[0];
    

    Micro-optimization: We could just do key($field_name_options).

  13. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +   * @see \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceReferencedEntityFieldFormatter::getAvailableFormatterOptions()
    

    Not sure we need the FQCN here.

  14. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,449 @@
    +      $formatter_type = array_keys($formatter_options)[0];
    

    Micro-optimization: We can use key($formatter_options).

  15. +++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceFieldFormatterTest.php
    @@ -0,0 +1,143 @@
    + * Tests the output of entity reference autocomplete widgets.
    

    I'm pretty sure this ain't testing autocomplete widgets! :)

marcoscano’s picture

1: Done.
2: Done.
3: Done.
4: FormatterPluginManager doesn't directly implement an interface, so I think it's still the best option here?
5: Done.
6: Done.
7: Done.
8: Done.
9: Done.
10: Done.
11: It seems so: https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
12: We were using key() originally, but in #93-3 @Wim Leers advised against it :)
13: Same as 11
14: Same as 12
15: Done.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I don't agree with @Wim Leers about key() (the array is never exposed to any external code, so we can be pretty certain about the position of the cursor), but it's definitely not important enough to block this patch any longer!

RTBC on the assumption that Drupal CI will pass it. Phenomenal work, @marcoscano et. al!

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

#103++

The one thing I would add: if we add this new formatter, we should make it work only for media. Then at least its scope is minimal, and we don't end up enabling (and hence implicitly encouraging) the use of this formatter.


#105:

Does it really block Media from being stable in the unlikely event that we don't have this formatter committed in time for 8.4.0? Personally, I don't think it does.

No, that's what #103 is saying too: we should consider making this a should-have or could-have, which means this doesn't need to happen for Media to be stable.

I see no reason to keep this from RTBC any longer

We're adding new functionality that affects not only Media, but also the wider ecosystem and site building experience. We need to consider that impact too.

#107:

I agree this is not a blocker for Media.

Seems like we're all on the same page then.


Patch review:

  1. +++ b/core/config/schema/core.entity.schema.yml
    @@ -362,3 +362,19 @@ field.formatter.settings.entity_reference_label:
    +field.formatter.settings.entity_reference_field:
    
    --- /dev/null
    +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    
    +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,450 @@
    + *   field_types = {
    + *     "entity_reference"
    + *   }
    

    While beautifully clean code (👏), there still is a risk in adding this to core directly, to be used on any entity reference field.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFieldFormatter.php
    @@ -0,0 +1,450 @@
    +        drupal_set_message($this->t('Could not instantiate the formatter plugin, please choose a different formatter.'), 'error');
    

    This is an error message I'd expect for a PHP exception, not a message shown to the site builder.

  3. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceFormatterTest.php
    @@ -402,6 +407,125 @@ public function testLabelFormatter() {
    +  public function testRenderFieldFormatter($label_option, $expected_output) {
    

    Test coverage is missing for the following cases:

    1. field 'view' access not allowed
    2. entity 'view' access not allowed
    3. entity 'view label' access not allowed
  4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceFieldFormatterTest.php
    @@ -0,0 +1,143 @@
    +  protected $testParentNode;
    ...
    +  protected $testChildNode;
    

    Nit: "parent" and "child" are rather confusing here. "referencing" and "referenced" would be less ambiguous.

  5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceFieldFormatterTest.php
    @@ -0,0 +1,143 @@
    +      'body'      => [
    

    Supernit: too many spaces here :)

  6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceFieldFormatterTest.php
    @@ -0,0 +1,143 @@
    +    $this->testParentNode = $this->createNode([
    +      'title' => 'Parent test node',
    +      'field_related_content' => [[
    +        'target_id' => $this->testChildNode->id(),
    +      ],
    +      ],
    +    ]);
    

    Nit: something wrong with indentation here.


For my next comment, I'm doing manual testing, so I can try to formulate helpful suggestions.

Wim Leers’s picture

In a nutshell, my concern is that we're adding a new entity field formatter to Drupal core, as stable, while it's entirely new, and enables site builders to not follow best practices.

Thumbnail formatter

I applied this patch and did manual testing. There already is a Media-specific Thumbnail formatter, added in #2831274: Bring Media entity module to core as Media module. Why is that not enough for the MVP? @effulgentsia said at #2831274-200: Bring Media entity module to core as Media module:

This screenshot shows that the regular "Rendered Entity" formatter already in core can successfully display what needs to be displayed for a simple use-case. People can install contrib modules for other formatters.

Rendered entity formatter

If I use the Rendered entity formatter, it uses the Default view mode by default. Well then yes it looks like crap, because this is the default entity view display (abbreviated):

id: media.image.default
targetEntityType: media
bundle: image
mode: default
content:
  created:
    …
  field_media_image:
    …
  thumbnail:
    …
  uid:
    …
hidden:
  langcode: true

But we could just change that to be a little less bizarre — because we ship it with core:
core/modules/media/config/install/core.entity_view_display.media.file.default.yml. Or, we can add an additional view mode, called embedded, ship that. Especially combined with #2865184: Allow MediaSource plugins provide default field form/view display settings, which was a follow-up of #2831274: Bring Media entity module to core as Media module anyway.

Proposal

I think using view modes can become a hassle-free process:

  1. core ships with core.entity_view_display.media.file.embedded and core.entity_view_display.media.image.embedded — i.e. embedded view modes/displays for the Media Types it creates by default ("File" and "Image")
  2. make embedded the default when referencing media entity types
  3. like #2865184: Allow MediaSource plugins provide default field form/view display settings proposes, let the @MediaSource annotation specify default formatter & widget settings. The default formatter settings would then be:
     *   default_formatter_settings = {
     *     "label" = "hidden",
     *     "settings" = {
     *       "view_mode" = "embedded",
     *       "link" = FALSE,
     *     },
     *     "third_party_settings" = {},
     *     "type" = "entity_reference_entity_view",
     *   },
    

UX comparison

Let's see how the site builder UX compares:

UX of current patch
See 2831943-UX-current-patch.mp4.
(Yes, #90.2 would improve this slightly, but rather than doing #90.2, we could do #2865184: Allow MediaSource plugins provide default field form/view display settings.)
UX of just steps 1 and 2 in my proposal
(assuming step 3 would have to be a follow-up)
See 2831943-UX-steps-1-and-2.mp4.
UX when my entire proposal is implemented
There's nothing to do, nothing to be confused by, because it'd be configured sensibly by default. The only thing that remains is customization.

So I'm personally not convinced that the current patch is necessarily the best way to do this. I'm not convinced that an view mode/view display-based approach is necessarily more difficult.

Conclusion

This patch does great work — and it will certainly not be wasted: if it doesn't land in core here, then it should definitely make its way back to https://www.drupal.org/project/field_formatter.

But the fundamental problem (which I thought of only now!) with the new formatter that the current patch is adding remains: it effectively removes many of the awesome capabilities of Media: the fact that Media items can have fields. This means you can have richly annotated media… but then you'll actually lose that richness when embedding/referencing media, because you are forced to pick a single field. Yes, this is not so much a problem with the default File and Image media types. But it's going to be a problem with @MediaSources that bring remote media into Drupal: this pattern works great for File and Image, but it doesn't "scale" to more complex media sources. And isn't that the key purpose of Media: a well-established pattern to bring remote media from various sources into Drupal, allowing them all be treated in a similar way?

Wim Leers’s picture

marcoscano’s picture

Thanks Wim for such in-depth review!

I think I agree with most of what has been said, although I feel there may be different perspectives on the table, each with different nuances, I'll see if I can elaborate on those here.

I think we are trying to address the potential concerns of at least 3 different scenarios:

Scenario 1:
A non-experienced site-builder adds a "media image" field to a node and expects it to render out-of-the box with sensible defaults.

Scenario 2:
A non-experienced site-builder wants to change a setting of the media field formatter (let's say, the image style) and, seeking for a smooth transition, we want this process to be as similar to what it is today when dealing directly with file/image core fields.

Scenario 3:
An averagely-experienced site-builder wants to take the most out of media entities (or any entity in general) and wishes to be able to show a single field from the referenced entity without having to create additional viewmodes for that. The experience in the media contrib issue queue seems to indicate that this scenario happens more often than one would think. An indicator of this is the number of times we answer support requests/bugs with: "Have you tried the Field Formatter module?". Below a non-exhaustive search just to bring up some examples:
https://www.drupal.org/node/2856093#comment-11961124
https://www.drupal.org/node/2850169#comment-11945015
https://www.drupal.org/node/2759069#comment-11358025
https://www.drupal.org/node/2833757#comment-11975148
https://www.drupal.org/node/2621950#comment-10636622
https://www.drupal.org/node/2773489#comment-11447341

I believe this issue initially originated with scenarios 1 and 2 in mind, and sometime along the way we also started thinking of scenario 3, because the "Field Formatter" approach seemed a good solution for scenario 2 too.

My main question at this point is: do we really need to choose what to address? or in other words: are the two approaches proposed here incompatible?

We seem to agree that scenarios 2 and 3 are not "urgent" or blockers for the media initiative roadmap. In this case, why don't we try to address scenario 1 with Wim's proposal from #119, but try in parallel to move this new formatter to a stable point, where we would be comfortable enough to add it to core and fully address then scenarios 2 and 3 at that point?

Eventually, we would have the best of both worlds: things would just make sense out of the box for basic usages, but for the more advanced use-cases, we would still be providing an extremely flexible tool for configuring the display.

One last remark on the thought that by introducing this formatter we could be encouraging site builders "not to follow best practices". I'm not sure I agree with that, especially if the default ends up being the rendered entity with a specific viewmode. All we would be doing extra is adding a new option on the select list, which users may or may not use. I feel that we already have a lot of core formatters that are not very self-explanatory or useful to the 80% use case, so I don't understand why this new one, once stable and well-tested, would be much more problematic.

In short:

1) +1 for #119 in the context of the media roadmap, in the short term.
2) Let's get this in as well, because it solves legitimate needs (not only related to media).

marcoscano’s picture

Sorry, one more thing :)

If I'm getting it right, #2865184: Allow MediaSource plugins provide default field form/view display settings is proposing to allow media source plugins to specify form/display settings for their source fields. Although that might be useful, it doesn't fully address scenario 1 as expected.

Once a "media image" field is just a standard entity_reference field, out-of-the box the display for this field will render it showing the media item's label, because that's the default formatter for this field type. An attempt to tackle this is being discussed in the follow-up #2893716: Pre-configure formatter "Rendered Field from Referenced Entity" when referencing media entities. We may want to include that need to the proposal in #119 to fully address Scenario 1, so that after adding a media field to a node, for instance, by default Drupal would render the media item, instead of showing its label.

larowlan’s picture

Status: Needs work » Needs review

First of all, thanks for the efforts here -

However, I agree with my earlier comments in #39, which gel with @WimLeers' review in #119

It sounds like we're trying to solve a UX issue by throwing more code at the problem, and in doing so adding a different way to achieve a lesser outcome.

If discoverability of view-modes is the issue, could we make the 'Rendered as Default' link 'Default' to the view mode configuration. We could even make it launch in a modal (quite easily). I agree with Wim, shipping sensible defaults solves much of this issue.

In addition, we already have an API for 'preconfigured fields', namely \Drupal\Core\Field\PreconfiguredFieldUiOptionsInterface::getPreconfiguredOptions which allows defining pre-defined options - including formatter configuration. From the docblock:

 /**
   * Returns preconfigured field options for a field type.
   *
   * @return mixed[][]
   *   A multi-dimensional array with string keys and the following structure:
   *   - label: The label to show in the field type selection list.
   *   - category: (optional) The category in which to put the field label.
   *     Defaults to the category of the field type.
   *   - field_storage_config: An array with the following supported keys:
   *     - cardinality: The field cardinality.
   *     - settings: Field-type specific storage settings.
   *   - field_config: An array with the following supported keys:
   *     - required: Indicates whether the field is required.
   *     - settings: Field-type specific settings.
   *   - entity_form_display: An array with the following supported keys:
   *     - type: The widget to be used in the 'default' form mode.
   *   - entity_view_display: An array with the following supported keys:
   *     - type: The formatter to be used in the 'default' view mode.
   *
   * @see \Drupal\field\Entity\FieldStorageConfig
   * @see \Drupal\field\Entity\FieldConfig
   * @see \Drupal\Core\Entity\Display\EntityDisplayInterface::setComponent()
   */
  public static function getPreconfiguredOptions();

So if \Drupal\Core\Field\FieldTypePluginManager::getUiDefinitions fired an alter hook, media module could intervene and declare some formatter configuration already, then someone adding a 'media' reference via the UI could get this preconfigured. It would require some minor changes to \Drupal\field_ui\Form\FieldStorageAddForm::submitForm to support adding 'settings' keys under 'entity_view_display' in getPreconfiguredOptions.

Also, to address the question of 'what harm is there in adding this as well' (paraphrasing) in #121, there is a maintenance burden. If there are UX issues with the existing view-mode approach, then we should address them separately instead of adding another way to achieve something similar.

I realise a lot of work has gone into this so far, apologies to those who had been chasing me for a review, I've been on leave (offline) for ~2 weeks. As Wim points out, this can be used in the field_formatters contrib project if the consensus is that core isn't the home for it.

rikki_iki’s picture

I think the suggestions in #119 and #123 would be a huge improvement, and agree with the sentiment of improving the original approach before adding new ones.

Though I do like this field formatter :) it would be super handy for way more than just media/file entities, but agree contrib would be a better home for it.

amateescu’s picture

Issue summary: View changes
FileSize
15.3 KB

So if \Drupal\Core\Field\FieldTypePluginManager::getUiDefinitions fired an alter hook, media module could intervene and declare some formatter configuration already, then someone adding a 'media' reference via the UI could get this preconfigured. It would require some minor changes to \Drupal\field_ui\Form\FieldStorageAddForm::submitForm to support adding 'settings' keys under 'entity_view_display' in getPreconfiguredOptions.

This is very close to how I imagined we should expose the media ecosystem in Field UI, with one important addition: we should have a dedicated Media group in the field list dropdown instead of putting everything under Reference, and, within that Media category we can expose each media source as an individual option. It would look something like this:

Note that Media is no longer an option in the Reference group, and when #2831943: Users expect rendered media, not links, and configuring the entity reference display and view mode properly is challenging, #2831940: Create file field widget on top of media entity, #2831941: [PP-1] Re-create Image field widget on top of media entity are done, we can even hide File and Image from Reference completely (with no_ui = TRUE in the field types definition).

Wim Leers’s picture

#121:
For scenario 2, an easy solution would be to add a "configure view modes" link, just there already is a "configure image styles" link for ImageFormatter and MediaThumbnailFormatter.

I'm not sure I agree with that, especially if the default ends up being the rendered entity with a specific viewmode.

Using view modes is the best practice, so I'm not sure what you mean?

All we would be doing extra is adding a new option on the select list

The current options are: A) Rendered entity, B) Entity ID, C) Label, D) Thumbnail. This is adding another one: E) Rendered field from referenced entity. That's five choices. That's a lot to explore. If anything, I'd argue we want to remove the D) Thumbnail option. I doesn't work for all media sources anyway, so it can quite easily result in a broken experience. That means we'd end up with just 3 choices: A) Rendered entity, B) Entity ID, C) Label. Then suddenly the difference is very clear: for a Media field, there's really only one clear choice if you want your media to visually show up.


#122: good catch — then I guess I'm saying I'd also like to see the next logical step happen, which can actually rely on the same annotation metadata.


#123:

In addition, we already have an API for 'preconfigured fields'

:O Who knew!? So glad you commented here! :D (I just did some archeology, apparently I helped make this a reality: #2446511: Add a "preconfigured field options" concept in Field UI — lol. I totally forgot.)

Also, to address the question of 'what harm is there in adding this as well' (paraphrasing) in #121, there is a maintenance burden. If there are UX issues with the existing view-mode approach, then we should address them separately instead of adding another way to achieve something similar.

This. Exactly this. Rather than improving what we have, we're adding new work-arounds, which implies more code to maintain, more choices to be made by the site builder, and hence resulting in more confusion.

Also my apologies for not speaking up in more detail sooner — I was working on the file_usage criticals that also affect the Media Initiative.


#125

This is very close to how I imagined we should expose the media ecosystem in Field U

👏❤️ — we don't have enough Andrei in these Media issues!

with one important addition: we should have a dedicated Media group in the field list dropdown instead of putting everything under Reference, and, within that Media category we can expose each media source as an individual option. It would look something like this:

I think you mean Media Types (= bundles), not Media Sources (= plugins).

In any case: what if you want to allow multiple media types? That's what the UI currently allows you to choose in one of the steps in the multi-step form:

I guess you're saying that's an edge case, and for that you can just choose "Other…" in the "Reference" <optgroup>, and then configure that manually? That'd make sense to me.

amateescu’s picture

I think you mean Media Types (= bundles), not Media Sources (= plugins).

Indeed, I meant media types, @Berdir pointed out the same thing in IRC :)

for that you can just choose "Other…" in the "Reference" <optgroup>, and then configure that manually?

Actually, an "Other..." option in the Media <optgroup> which will have the the Media entity type preselected would make more sense IMO, because we want to keep them closer together.

Wim Leers’s picture

Right, that makes sense!

seanB’s picture

#126

For scenario 2, an easy solution would be to add a "configure view modes" link, just there already is a "configure image styles" link for ImageFormatter and MediaThumbnailFormatter.

To render a media image in a node you need to first create a image style, then create a media view mode for the image type, and after that pick the new view mode for the 'Rendered entity' formatter in the node. Should we then expose both links ('configure view modes' / 'configure image styles') for the ' Rendered entity' formatter?

What if you have multiple image types in the same field? You should then configure the view mode for each bundle for it to work, so not sure where the link 'configure view modes' should go in that situation?

If anything, I'd argue we want to remove the D) Thumbnail option. I doesn't work for all media sources anyway, so it can quite easily result in a broken experience.

Every media item should have a thumbnail? But I guess it's better to also solve this with the generic entity reference formatter, so removing this sounds fine.

marcoscano’s picture

FileSize
23.98 KB

I may be digressing a bit, but re #125:

source or type

If they are supposed to be the media types available, how can we dynamically update this information with the types that really exist? (and not show File/Image if, for example, the site has only a "youtube" media type).

A simpler approach would be to discover all source plugins available (with something like MediaSourceManager::getDefinitions()) and expose them all.. but still, we would be exposing options that potentially don't make sense, depending on the site configuration.

Wim Leers’s picture

To render a media image in a node you need to first create a image style, then create a media view mode for the image type, and after that pick the new view mode for the 'Rendered entity' formatter in the node.

(Assuming you meant s/image type/media type/ :))

  1. 3 image styles are created by default
  2. the default and teaser entity view displays for article Nodes both use some of these default image styles
  3. therefore for File and Image Media Types, we could ship with default configuration
  4. and for newly created ones, it's not quite as laborious as you indicate, but yes, probably we want to improve that — this is an existing UX problem, which is fine!

The point is that we're not introducing more configuration options in the UI, which by definition makes the UI more complex: there is a new Drupalism to know about.

so not sure where the link 'configure view modes' should go in that situation?

/admin/structure/display-modes/view#media — then in a follow-up make that UI better, which helps all entity types/the entire ecosystem.

#130: Yes, see #127, @amateescu meant Media Types, not Media Sources. To your second question: we could just iterate over all bundles (= Media Types), and generate a preconfigured option for each of them? I think you were raising this on the assumption that this was about Media Sources, but it's indeed about Media Types.

seanB’s picture

The point is that we're not introducing more configuration options in the UI, which by definition makes the UI more complex: there is a new Drupalism to know about.

The way I see it, the fact that media now has view modes is already a new Drupalism for sitebuilders. This formatter tries to make that easier.

dawehner’s picture

Damnit my comment got lost.

I think we are facing a deeper problem here: We treat media as part of content modelling, even for most people working on content types, they really just want to pull in media. They don't care how its treated internally. This is quite a Drupalism.

I like the idea of having "media" as its own things while adding fields, as this hides this detail.
On top of that I had the hope that we could do something similar for view displays based upon the source fields somehow. IMHO putting it into configuration would just solve one part of the equation.

Could the media plugins, much like they create source fields also create view displays automatically?

On the other hand, is there an initiative to make view displays easier to deal with/create? It requires a lot of Drupal knowledge to know how to use them. At the given point in time its a tough decision to let the media initiative be blocked based upon some major UX of core right now.

Wim Leers’s picture

Could the media plugins, much like they create source fields also create view displays automatically?

That's exactly what I thought of when I read #2865184: Allow MediaSource plugins provide default field form/view display settings! I thought I'd said it in my comments of the past 24 hours, but apprently I did not.

At the given point in time, I

Comment is cut off.

seanB’s picture

I created video's to compare all steps for a common use case that is the most problematic (at least for me) when using 'rendered entity':
Display a media image attached to a node in a new image style.

Rendered Entity formatter steps:

  1. Select 'Rendered Entity' formatter
  2. Configure 'Rendered Entity' formatter (this is where you should get told that you need a image style and view mode?)
  3. Save node view mode with new formatter for media field.
  4. Create new image style.
  5. Configure image style.
  6. Save image style.
  7. Create view mode for media.
  8. Save view mode for media.
  9. Enable new view mode for media bundle (this needs to happen for every bundle enabled on the field).
  10. Configure new view mode for media bundle.
  11. Enable new image style on the image field.
  12. Save the media view mode.
  13. Go back to node view mode.
  14. Configure the media field to use the new view mode.
  15. Save the node view mode.

Rendered field from referenced entity formatter steps:

  1. Select 'Rendered field from referenced entity' formatter
  2. Configure 'Rendered field from referenced entity' formatter (you will see the link to create a new image style here already)
  3. Create new image style.
  4. Configure image style.
  5. Save image style.
  6. Go back to node view mode.
  7. Select 'Rendered field from referenced entity' formatter
  8. Configure the media field to use the new image style.
  9. Save the node view mode.

The new formatter is a big improvement. The main questions are:

  • Is the new formatter the best/only way to improve the UX for this issue?
  • What are the alternatives when improving UX for entity reference fields with rendered entities / view modes?
Wim Leers’s picture

From the Media meeting on IRC a few hours ago:
16:22:23 <effulgentsia> So my proposal (or rather, my rehash of what larowlan and others are proposing) is: sane defaults to solve scenario #1, links within formatter settings and formatter settings summary to solve scenario #2, and advertising field_formatter module to solve scenario #3.
(He's referring to #121 by @marcoscano.)

IMO this even addresses scenario 3 for many/most users, because wishes to be able to show a single field from the referenced entity without having to create additional viewmodes for that is a consequence of that being hard to find today. I think this changes that.


This patch does 2 things:

  1. it changes the default configuration for the image and file Media Types' default displays, so that they have sensible defaults right out of the box

    (In doing this, it also fixes the very broken default displays that we currently have.)

  2. it updates EntityReferenceEntityFormatter::settingsForm() to link to the corresponding view display configuration forms — if only one bundle is allowed, it'll link to that, otherwise it'll provide a link for every allowed bundle:
    Single allowed bundle
    Multiple allowed bundles

    (I just noticed that this is what @mallezie proposed all along, in #32.

Wim Leers’s picture

And both @mallezie in #32 and @larowlan in #123 mention that these links could launch in modals. They indeed can. This does that.

#136 fixed the discoverability of the connection: you no longer need to know all the concepts and their place in the menu tree. This (#137) goes a step further: it brings it front and center to the user! And in doing so, it helps all entity types, and the entire Entity/Field ecosystem.

(That's the image Media Type's Default entity display config form in a modal, triggered from the article Content Type's entity display config form.)

Wim Leers’s picture

FileSize
995.87 KB

#135 has screencasts to compare HEAD vs the formatter approach of #116. It's clear that it's super painful in HEAD :)

In this comment, I have screencasts to compare the formatter approach of #116 to the "improved view mode UX" approach of #137.


Compare the UX of:

formatter approach from the patch in #116

vs

"improved view mode UX" approach of #137

Wim Leers’s picture

I just discovered a fundamental flaw in the formatter approach that I don't think we noticed before: because it forces you to choose a field, you have to choose either File Media Type's File field or Image Media Type's Image field.

In other words: the formatter approach does not work at all when you allow multiple Media Types to be referenced from your Media field! (i.e. when there's >1 target bundle)

Wim Leers’s picture

Note: what I don't show in the screencast of #138, is that as soon as you click the gear icon for the image field in the Image Media Type's display, its AJAXy magic updates NOT that form in the modal, but the one of the parent page! So to use this, we'd need to sort out that AJAX problem. I'm hoping AJAX system maintainers @tim.plunkett or @effulgentsia might be able to help with that.

The last submitted patch, 136: 2831943-view_modes-136.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanB’s picture

FileSize
140.72 KB

#139 That is indeed a problem for the formatter. Nice find!

I like where #137 is going. Nice work Wim Leers! I have 2 things I would like to see addressed:

  • We should show a help text to make users aware that changes in the view mode are generic. This could lead to unintended changes. So this should be really clear to users.
  • You should be able to add a new view mode as well. Adding view modes is not a common task, so we should make it easy for users to configure a new display. Something like this?
    Formatter changes

When these 2 things are fixed I guess we can do everything the new formatter could do from this screen AND MORE!

Status: Needs review » Needs work

The last submitted patch, 137: 2831943-view_modes-137.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

marcoscano’s picture

This is looking great! Thanks everyone (and Wim in particular) to reaching this point.

I echo what seanB said in #142, in the sense that if 1) the user can also create view modes from the modal, and 2) there is a clear message indicating that the viewmode will be changed everywhere, then we are probably solving most UX oddities we were worried about!

But I think we should also not forget #90.2 ([2893716], which we should be able to close at this point, or re-purpose it at least). Basically I believe it would be important to allow the Rendered Entity formatter to be selected automatically when an ER field pointing to a media entity is created (instead of the default "Label"). From #126 I understand this is feasible, so we should be OK on this point too!

Thanks everyone!

larowlan’s picture

Note: what I don't show in the screencast of #138, is that as soon as you click the gear icon for the image field in the Image Media Type's display, its AJAXy magic updates NOT that form in the modal, but the one of the parent page! So to use this, we'd need to sort out that AJAX problem. I'm hoping AJAX system maintainers @tim.plunkett or @effulgentsia might be able to help with that.

I'm guessing there is an ID collision, i.e. the form uses #prefix/#suffix that generates a fixed ID wrapper.

In other words: the formatter approach does not work at all when you allow multiple Media Types to be referenced from your Media field! (i.e. when there's >1 target bundle)

Yeah, I think that is a showstopper for the formatter unfortunately.

So to summarize what I understand the remaining steps are

  • We need \Drupal\Core\Field\PreconfiguredFieldUiOptionsInterface::getPreconfiguredOptions to support 'settings' keys for view display (form display can come at same time or in a different issue)
  • We need an alter hook on the result of that so modules can intervene
  • We need sensible default view modes
  • We need an alter hook in media that intervenes and sets the formatter to 'rendered entity' using its preconfigured view modes
  • We continue with the modal approach but address the ajax bug

If that's correct, can we update the issue summary?

acbramley’s picture

My 2c:
My first reaction to this was that it sounded like a great idea! More power to the builders! And I've had quite a few situations where a simple formatter would've saved me time and complexity with view modes for various entity reference fields. But as I started reading through the issue comments I began to come around to the idea that maybe it's not such a good idea. My thoughts now have been pretty well summed up so far by @Wim Leers and @larowlan.

#137 looks great! Much more intuitive! What about being able to add a new view mode from that modal as well?

jibran’s picture

We need \Drupal\Core\Field\PreconfiguredFieldUiOptionsInterface::getPreconfiguredOptions to support 'settings' keys for view display (form display can come at same time or in a different issue)

Instead of doing this why not fix #552604: Adding new fields leads to a confusing "Field settings" form.

larowlan’s picture

Instead of doing this why not fix #552604: Adding new fields leads to a confusing "Field settings" form.

One is about field settings, the other is about formatter settings

jibran’s picture

One is about field settings, the other is about formatter settings

\Drupal\Core\Field\PreconfiguredFieldUiOptionsInterface::getPreconfiguredOptions is only used in FieldItems. It is never used for formatter. We should add a new function/interface for formatters.
Let me explain my comment. 🙂
In #123 and #124 and #126 you guys were discussing field settings as well. Or is that a wrong context?
#123

So if \Drupal\Core\Field\FieldTypePluginManager::getUiDefinitions fired an alter hook, media module could intervene and declare some formatter configuration already, then someone adding a 'media' reference via the UI could get this preconfigured. It would require some minor changes to \Drupal\field_ui\Form\FieldStorageAddForm::submitForm to support adding 'settings' keys under 'entity_view_display' in getPreconfiguredOptions

#125

This is very close to how I imagined we should expose the media ecosystem in Field UI, with one important addition: we should have a dedicated Media group in the field list dropdown instead of putting everything under Reference, and, within that Media category we can expose each media source as an individual option.

I'm just saying this would be easier to do after #552604: Adding new fields leads to a confusing "Field settings" form because it removes the field storage form step and \Drupal\field_ui\Form\FieldStorageAddForm::submitForm can pass the values directly to field setting form and we don't have to touch \Drupal\Core\Field\PreconfiguredFieldUiOptionsInterface::getPreconfiguredOptions.

larowlan’s picture

We're talking about something different - here is the signature I'm talking about

/**
   * Returns preconfigured field options for a field type.
   *
   * @return mixed[][]
   *   A multi-dimensional array with string keys and the following structure:
   *   - label: The label to show in the field type selection list.
   *   - category: (optional) The category in which to put the field label.
   *     Defaults to the category of the field type.
   *   - field_storage_config: An array with the following supported keys:
   *     - cardinality: The field cardinality.
   *     - settings: Field-type specific storage settings.
   *   - field_config: An array with the following supported keys:
   *     - required: Indicates whether the field is required.
   *     - settings: Field-type specific settings.
   *   - entity_form_display: An array with the following supported keys:
   *     - type: The widget to be used in the 'default' form mode.
   *   - entity_view_display: An array with the following supported keys:
   *     - type: The formatter to be used in the 'default' view mode.
   *
   * @see \Drupal\field\Entity\FieldStorageConfig
   * @see \Drupal\field\Entity\FieldConfig
   * @see \Drupal\Core\Entity\Display\EntityDisplayInterface::setComponent()
   */
  public static function getPreconfiguredOptions();

There is a settings key for field config and field storage config. But none for entity_view_display and entity_form_display, both of which support settings. That is what we need. This allows media to do this in the alter hook

function media_field_preconfigured_options_alter(&$options) {
  // Use the rendered entity formatter instead of the default of label.
  $options['field_ui:entity_reference:media']['entity_view_display']['type'] = 'entity_reference_entity_view'  
  // Use the preconfigured default view mode.
  $options['field_ui:entity_reference:media']['entity_view_display']['settings'] = [
    'view_mode' => 'embedded',
  ];
}
Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Unfortunately, AJAX system maintainers @tim.plunkett or @effulgentsia didn't chime in — so that means I've started to dig in.

This is now my #1 focus. Will report back when I have news.

larowlan’s picture

Wim Leers’s picture

#152: yes, but how to work around that?

We can't really make the ID dynamic easily, because it's also hardcoded in field-ui-table.html.twig. It's possible some CSS or JS is relying on it.
So I tried to modify field_ui.js to prefix the wrapper's selector with the form ID. So that effectively the AJAX command is scoped to this form. Unfortunately, just modifying drupalSettings.ajax is not enough — at that point those have already been converted to Drupal.ajax.instances, which are pretty hard to reach at that point.
Then it dawned on me that this is a general problem that probably quite a lot of our AJAXy forms suffer from. So modifying this form's JS doesn't actually help a whole lot — most AJAXy forms don't actually have custom JS. So… let's fix it in ajax.js!
Turns out that if you go to the root, it suddenly becomes far, far simpler. And to ensure I don't break BC, I made it an opt-in thing. This means that the solution is now declarative:

Before
      '#ajax' => [
        'callback' => '::multistepAjax',
        'wrapper' => 'field-display-overview-wrapper',
…
After
      '#ajax' => [
        'callback' => '::multistepAjax',
        'scope' => 'form',
        'wrapper' => 'field-display-overview-wrapper',
…

Now it works:

… except there's still quite a bit of brokenness:

  1. Note the "Update" and "Cancel" buttons in the dialog's bottom bar. They should actually live inside the table row. They're automatically being moved there. Will need to find a work-around for that.
  2. The "Save" button in the dialog doesn't trigger an AJAXy save, but instead reloads the entire page.
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
8.26 KB

Fixed the causes for the test failures & coding standards failures, so that there is an actually reviewable patch (albeit it's definitely not yet ready!).

Now continuing the AJAXy stuff.

Wim Leers’s picture

One of the key usability flaws in existing Field UI is that it always says Manage display. Which means it's extremely confusing to figure out what exactly it is that you're looking at. You have to either inspect the URL, or look at the primary & secondary tabs.

That means this problem is made worse by the patches I've been posting (#136–#153). The screenshot in #153 clearly illustrates this problem.

So, here's one important usability improvement: make the title actually accurate!

Wim Leers’s picture

Added the ability to create a new view mode.

(The same problem exists here as for #153: the "Save" button in the dialog doesn't trigger an AJAXy save, but instead reloads the entire page.)


The last submitted patch, 155: 2831943-view_modes-155.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 156: 2831943-view_modes-156.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
11.98 KB
211.29 KB

Note the "Update" and "Cancel" buttons in the dialog's bottom bar. They should actually live inside the table row. They're automatically being moved there. Will need to find a work-around for that.

Solution found!

This is Drupal.behaviors.dialog.prepareDialogButtons() (in dialog.ajax.js) at work. That uses the selector '.form-actions input[type=submit], .form-actions a.button' to find buttons which it should "lift" out of the form in the dialog, into the buttons bar at the bottom of the dialog. But .form-actions means "key actions for the entire form" — which is not true of those "Update" and "Cancel" buttons. They only apply to that single row in the form.
Turns out simply removing '#type' => 'actions' is enough to fix that :)

Thanks to @drpal for helping out with this one!

@drpal also pointed out a small ES6 style problem in my patch, fixed that. (It transpiles to the same ES5 code as before.)

Next: figuring out how to make that "Save" button be AJAXified.

drpal’s picture

@Wim Leers

👍 I was just about to leave a note about a conversation.

Status: Needs review » Needs work

The last submitted patch, 159: 2831943-view_modes-157.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
5.6 KB
15.62 KB

This updates the test coverage so that it passes again given the title changes made in #155.

Note to self: \Drupal\system\Tests\Menu\AssertBreadcrumbTrait::assertBreadcrumbParts() is extremely brittle, don't ever come near that again.

xjm’s picture

Title: Create generic entity reference formatter to show fields from referenced entity » Users expect rendered media, not links, and configuring the entity reference field properly is challenging
Category: Task » Bug report
Priority: Normal » Major

So this issue no longer adds a formatter, and we're refocused on the major usability bug that the formatter was intended to fix. After chatting with seanB, I'm retitling the issue to reflect the bug.

xjm’s picture

Title: Users expect rendered media, not links, and configuring the entity reference field properly is challenging » Users expect rendered media, not links, and configuring the entity reference display and view mode properly is challenging
xjm’s picture

Issue summary: View changes

Trying to update the IS to be more current.

Aside:

+++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
@@ -244,12 +245,14 @@ public function form(array $form, FormStateInterface $form_state) {
-    $form['actions']['submit'] = [
-      '#type' => 'submit',
-      '#button_type' => 'primary',
-      '#value' => $this->t('Save'),
-    ];
+//    $form['actions']['submit'] = [
+//      '#type' => 'submit',
+//      '#attributes' => ['foo' => 'bar'],
+//      '#button_type' => 'primary',
+//      '#value' => $this->t('Save'),
+//    ];

wut?

webchick’s picture

I will say, wearing my product management hat, that video in the issue summary made me audibly shriek. ;) I mean, that's a *fantastic* level 99 question on The Ultimate So You Think You Can Drupal™ final exam, but for key functionality that's an 80%+ use case..? Nope, sorry.

However, since the module is planning to be shipped hidden in 8.4, I don't think fixing this needs to block that. The only way end users get themselves into this situation is if they turn on some other module that turns on this functionality, and ideally that module presents a much, much better UX. And if not, worst-case, it's no worse than Media in contrib is now, and people have apparently been dealing with this already for however many months/years.

I do think that fixing this should be required for the module to be stable/unhidden in 8.5+, however. It's unreasonable to expect people who are not Drupal experts to come up with the steps outlined in the video on their own. Let's see what other UX folks/product managers have to say.

While it's bound to be better, I can't really judge the new approach based on a screenshot; it would be helpful if it had a similar list of steps which could compare/contrast against the original.

xjm’s picture

I filed #2895001: Use the bundle label (e.g. "Media type") instead of "Bundles" in the Entity Reference field configuration for #78. I think the "How to order the add field dropdown for media references?" discussion above can also be spun off into its own issue, but I didn't do that yet. It's like we're back in the day converting term reference fields to entity reference. ;) Party like it's 2015.

Bojhan’s picture

Thanks for all the hard work. I think Angie's concerns are very valid, and hopefully we can resolve them in the move from hidden to visible.

We as a community, heavily rely on a core concept (Field UI) that has a high amount of UX issues, it's difficult to orientate and use. I don't fully understand how this can't be solved with a specific display or better defaults. Lets make sure we raise a solid followup.

I do fear, we end up with a lot more "bundles" "entity references" etc. wording that our less experienced audience doesn't know and conflicts with our UI standards on language. We should by default not allow these to sip into our UI's.

Wim Leers’s picture

FileSize
1.11 KB
15.26 KB

#165: That hunk was for local debugging, I accidentally included it in #162. Sorry for scaring you :)

Wim Leers’s picture

What was still a problem in the patches above, was that the "Configure view modes" links point to the default view mode, not the one that is actually selected. This patch ensures that the links auto-update to point to the view display matching the currently selected view mode.

(This lifts some AJAXy bits from @marcoscano's awesome patch!)

This makes the patch pretty feature-complete.

Wim Leers’s picture

The "Save" button AJAXification was harder than expected (see #153.2 + #159). The root cause: \Drupal\field_ui\Form\EntityDisplayFormBase::form()'s "Save" button was simply being overwritten in \Drupal\Core\Entity\EntityForm::buildForm(). Moving it to EntityDisplayFormBase::actions() so that it overrides the default from EntityForm::actions() fixes that.

And with that, this is now pretty much ready for usability evaluation.

Wim Leers’s picture

Component: media system » entity system
Assigned: Wim Leers » Unassigned
Issue tags: +Entity Field API, +Usability
Related issues: +#2895382: Hide label, thumbnail, etc. for default display of media file and image references

#163#168: thank you so much for bringing clarity to this issue, whether it's required for Media or not, and so on!

I think this issue is now pretty much entirely outside the realm of Media. The Media initiative would benefit from this, but so would every other site builder. This is now very much an Entity/Field API usability issue.


The one thing that remains to be done for Media after we make this first and foremost an Entity/Field API issue is this thing that Bojhan said in #168:

I don't fully understand how this can't be solved with a specific display or better defaults. Lets make sure we raise a solid followup.

That's exactly what @larowlan, myself and others have been saying too. So created #2895382: Hide label, thumbnail, etc. for default display of media file and image references for just that.

The last submitted patch, 171: 2831943-view_modes-171.patch, failed testing. View results

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Re: #172:

I don't fully understand how this can't be solved with a specific display or better defaults. Lets make sure we raise a solid followup.

That's exactly what @larowlan, myself and others have been saying too. So created #2895382: Default File & Image displays look broken: improve them for just that.

I don't think @Bojhan was referring to that issue at all; in fact, he's saying the opposite. He's saying that a specific display or better defaults does not solve the issue; that the field UI itself complicates matters; when reviewers test this issue, they run into out-of-scope usability issues.

However, we don't need to file new followups unless we have specific suggestions that aren't covered by all the issues that already exist:
https://www.drupal.org/project/issues/search/drupal?project_issue_follow...

xjm’s picture

Status: Needs review » Needs work

#2895382: Hide label, thumbnail, etc. for default display of media file and image references is in now, so this will need a reroll now.

Though, actually, I wonder if we shouldn't pursue #2893716: Pre-configure formatter "Rendered Field from Referenced Entity" when referencing media entities first, since that is almost always going to be what someone wants anyway. Then we patch entity reference formatters to allow selecting the display of the rendered entity, which is a worthwhile UX improvement for all entity reference fields, along with a link underneath the display selection to create a new display. Even a simple link to create a new view mode (with no modal magic) would be worthwhile.

Edit: Wait. #2893716: Pre-configure formatter "Rendered Field from Referenced Entity" when referencing media entities isn't what I thought it was at all. I thought it was proposing defaulting to plain old "Rendered entity". I'll comment there.

xjm’s picture

Oh, one other note from #168 to @Bojhan:

I do fear, we end up with a lot more "bundles" "entity references" etc. wording that our less experienced audience doesn't know and conflicts with our UI standards on language. We should by default not allow these to sip into our UI's.

This patch is not introducing any of that; those are existing core issue for Entity Reference that have existed since 2014. I do agree that we should not surface these bugs where they were not surfaced before, but the terminology bugs you're referring to also affect Taxonomy and other Entity Reference fields, and they're not in the same part of the Field UI being changed by this patch.

@marcoscano clarified in #2893716: Pre-configure formatter "Rendered Field from Referenced Entity" when referencing media entities that that issue was a followup to use the earlier version of this patch but was no longer relevant. Can we have another small spin-off issue to simply make Rendered entity the default formatter when you add a new Media entity reference field to something, now that #2895382: Hide label, thumbnail, etc. for default display of media file and image references makes the Rendered entity exactly what someone wants plus or minus some view mode and image style configuration? That will be another iterative improvement that has its own scope.

Bojhan’s picture

@xjm Sorry, I should have clarified - I do think that defaults help, but they do not solve the fundamental issues that are caused by the Field UI.

I know labels are part of our legacy, but realistically they are surfacing more and more. I don't like the stance, its legacy - let's not deal with it/postpone work on it. We need to attack it at some point, and for that to happen it needs to at some point become a priority.

xjm’s picture

I know labels are part of our legacy, but realistically they are surfacing more and more. I don't like the stance, its legacy - let's not deal with it/postpone work on it. We need to attack it at some point, and for that to happen it needs to at some point become a priority.

I'm not saying to postpone the issues, at all. In fact, I'm in the process of adding some existing issues like #1953568: Make the bundle settings for entity reference fields more visible. and #2895001: Use the bundle label (e.g. "Media type") instead of "Bundles" in the Entity Reference field configuration to the media roadmap since they would be UX regressions if we added Media files and images to Standard without addressing them.

What I am saying, however, is that they are out of scope for this issue because this issue is about providing better UX for Media/Entity Reference field formatters, so usability issues that are not about the Media field formatters should be filed as separate issues (or since a lot of the issues were already filed when we added entity reference and then further converted taxonomy fields, discussion can move to the existing issues and we can kickstart work on them again). This is why I filed #2895001: Use the bundle label (e.g. "Media type") instead of "Bundles" in the Entity Reference field configuration as a separate issue and linked the other, existing related issues that @yoroy ran across in his review, which were in the step before he got to the formatter screen.

Thanks @Bojhan!

Bojhan’s picture

I appropriately updated the priority of that one, to reflect the severity I think it deserves. Thanks.

chr.fritsch’s picture

Just a reroll, tests are still failing

suranga.gamage’s picture

Looks great so far,
Going trough it manually some extra tweaks/todo's

- The 'add a new view mode' submit still redirects me to admin/structure/display-modes/view
- The Form in the modal shows a 'custom view display mode' underneath the form, from a UX point of view I think this shouldn't be there.

To be validated as bugs

The new view modes added via the UI don't seem to render the ajax, possibly because they aren't auto listed as used.

Steps to reproduce
- Add a new entity reference field
- Format the display mode and add a new view mode via the ajax overlay
- Try to format the new view mode via the overlay (doesn't open)

Expected: It should open.

Proposed solution:
- Check or a slightly different form has a better UX experience.

marcoscano’s picture

I was thinking it's time to revamp this issue! :)

First off, thanks again @Wim Leers for the great work on the patch!

I'm not sure what the next steps really are, so I'll just propose what I think makes the most sense.

As per #171:

And with that, this is now pretty much ready for usability evaluation.

It looks like the feedback from the UX team would be important before we keep moving on. To make that easier, I have created 2 videos showing the patch in #181 in action:

Video 1: Configuring an existing viewmode

Video 2: Creating a new viewmode and configuring it

On these videos I have used nodes and a standard entity_reference field to make it explicit that this doesn't really affect only media entities and fields anymore.

On the second video, we can see that there are stil some outstanding issues to resolve:
1) Prevent viewmode creation modal form from redirecting the user to the viewmodes overview page. (0m14s)
2) After you create a new viewmode, you can't configure it straight away, you have to enable display settings on it first. (0m34s)
3) When you try to submit the main form, it is now AJAXyfied, so you are never sure if you saved the page. This may be confusing because it changes the way that page worked so far. (0m58s and also 2m02s)
4) When accepting the new config for the viewmode you created inside the modal, the field fieldset closes without taking your changes into account. The viewmode settings themselves are saved though. (1m34s)

Although they all seem issues worth working on, I think they don't prevent us from understanding the new proposed workflow, and evaluating the UX aspect of it. Based on this, @yoroy, @Bojhan, @Gábor Hojtsy, could you please let us know if you have feedback beyond solving the above mentioned bugs?

Also, marking as NR just to trigger the testbot on #181, although we already know that another task needed here is:

5) Fix failing tests and implement test coverage for the new functionality.

Anything else should be considered as next step here?

marcoscano’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 181: 2831943-181.patch, failed testing. View results

chr.fritsch’s picture

Issue tags: +Needs usability review

Thanks @marcoscano for the great summary of the status.

Tagged issue with "Needs usability review"

marcoscano’s picture

Issue summary: View changes

So this was discussed in the UX meeting of November 21st, 2017: https://www.youtube.com/watch?v=QuLm3n2VFrs

I couldn't make it to be there, but thanks a lot for the UX team for dedicating time to this, and specially to @phenaproxima to be "representing" the issue! :)

Quoting @phenaproxima from slack about the feedback received:

So, with regard to the view mode stuff, the decision seems to be that smoothing over view modes is the right way to go. So the single-field formatter is dead

The patch we currently have is quite good, but still needs some refinement. There are bugs, and we want to make Media smarter about "pre-cooking" view modes for new media types.

But, I think the path forward is clear. Basically, improve the patch we have, and pre-package an "embedded" view mode for media items, which we automatically customize for new media types.

The good news is that the pre-cooking of the "default" viewmode for media types that ship with core (file and image) was already fixed in #2895382: Hide label, thumbnail, etc. for default display of media file and image references, so the path forward, if I understood well, would mainly consist of fixing the bugs indicated in #183.

Thanks everyone!

yoroy’s picture

We discussed this issue during a usability hangout. The question was to give guidance on which direction to pursue:

  1. A custom formatter that renders some default version of the referenced media
  2. Use the existing way to do this: view modes.

Spoiler: we decided to go for option 2.

The custom formatter would add an option to the list of formatters in Field UI. Simple & direct. Also: relatively hard-coded, not configurable/extensible.

Using view modes: If you want to show the thumbnail version of a referenced Image media type in your Article content type, then you have to first set up that thumbnail-only view mode on the Image media type before you can select that as the way to render the Image media when shown inside an Article.

If we handled it like that we’d introduce a very Drupal-y UX problem of creating a backwards workflow (see point 5 here): having to go do something somewhere else before you can return to do the thing you wanted to do in the first place.

Still, it would be good to use the view modes approach here instead of the custom formatter. The custom formatter is a limited fix, if you’d want to change the display of a referenced media item you would still have to (find out how to) use the view modes way.

The patch already removes the need to go elsewhere first by providing links to create & configure a view mode for the referenced media from inside the Field UI for the content type that has the reference. Instead of having to go somewhere else this basically nests a part of Field UI for the media item in a modal on top of the Field UI for the referencing item.

We concluded that we do want to go with the view modes approach. Another addition that could help make this work a bit simpler is providing a simple view mode with the media entity that can be used as the default for this.

marcoscano’s picture

Assigned: Unassigned » marcoscano
FileSize
1.67 MB
22.47 KB
10.35 KB

I'm working trying to fix the bugs identified in #183.

Still WIP, but in this patch:
- We fixed the case where no target bundles were selected (so all should be available options in the "configure" links)
- We are skipping the combination of entity type / bundle / viewmode in the configure links for the same form we are editing, because it would be weird to open the modal with the same form we are currently editing on the main page.
- Fixed the redirection after creating a new viewmode from the modal. (#183.1)

Current WIP state: video

At this point, I realize that maybe we shouldn't use the same route controller (entity.entity_view_mode.add_form) when creating the viewmode inside the modal, because:
1) We want this viewmode to be created directly with "Custom settings" enabled (see #183.2)
2) Ideally, after creating a viewmode, we would want the modal window to stay open and show the configuration form for that viewmode that was created. That knowledge is really out of the reach of \Drupal\field_ui\Form\EntityDisplayModeAddForm, and I have no idea how to implement that with the current approach.

Maybe it would be easier to load inside the modal a new custom form with its own submit function, which would save the new viewmode with correct settings, and redirect to the edit form inside the modal?

I'm open to ideas on how to best deal with this. Thanks!

phenaproxima’s picture

One word that came up in the UX hangout was "pre-cooking". That pretty much says it all -- sane defaults to make users' lives easier, while taking advantage of the more flexible architecture of view modes.

How about if we did the following?

1) Media could ship a view mode, called 'embedded' (or similar), as part of its standard config. This would ship with Media, not Standard.
2) Upon creation of a new media type, Media would automatically create a new view display for the media type, in the embedded view mode, and enable it.
3) The source field, and only the source field, would be added to the new view display, using the default display options defined by the media source -- as implemented in #2865184: Allow MediaSource plugins provide default field form/view display settings. (This approach would mean that issue will block this one.)
4) This could all easily be done with hook_media_type_insert().

We could very quickly have a proof-of-concept for this. I suspect the UX would be quite good, and it would solve the immediate problem in time for 8.5.x. Then we could work on creating view modes/displays inline in a follow-up.

What say you all?

marcoscano’s picture

As discussed on slack with @phenaproxima:

There are actually two needs being addressed here:

1) A media-related one, whose purpose is to make life easier for site builders that want to reference media entities and expect "things to just work". For these users, we agreed that the combination of #2928699: Add an alter hook for the pre-configured field UI options and implement it in the Media module + #2895382: Hide label, thumbnail, etc. for default display of media file and image references is already a great improvement and is probably enough for the 80% use case.

2) The poor Field UI usability flow that obliges a user to go elsewhere to create a viewmode before being able to configure it inside the "Rendered entity" formatter. This is what the current patch is trying to address.

mallezie’s picture

And both things can happen. 1 is easiest, and could solve the media problem for now. And when 2 is done, 1 still is a valuable addition.

Wim Leers’s picture

@marcoscano pinged me for #189, but I think @tim.plunkett is better positioned to provide feedback TBH :)

phenaproxima’s picture

Per #191, I think we are in agreement that while Media benefits enormously from this, it is primarily a Field UI problem, not a Media problem. Media exposes it as the glaring UX flaw that it is, but Media's immediate needs in this area will be addressed by the much simpler #2928699: Add an alter hook for the pre-configured field UI options and implement it in the Media module. I don't think this needs to be considered a blocker to showing Media in the UI, so we might want to adjust #2825215: Media initiative: Essentials - first round of improvements in core too. I leave that to the product and/or release managers.

So I think this issue needs a rescope, plus a new summary and title to go with it. It's not strictly in Media's wheelhouse.

marcoscano’s picture

Re: #191 and #194, I posted a video of the scenario described in #191.1 here:

https://www.drupal.org/project/ideas/issues/2825215#comment-12382849

xjm’s picture

Thanks @marcoscano and @phenaproxima. I posted about whether or not this still needs to be a "must-have" here:
#2825215-123: Media initiative: Essentials - first round of improvements in core

leslieg’s picture

Testing this as part of #2018BostonMediaSprint. Testing in Chrome on a Mac (running mac os High Sierra 10.13.2) Patch in #189 applied to 8.5.0-dev. Patch in #189 applied to 8.5.0-dev.

As a site user I easily saw how to add a new view mode in manage display for an entity reference media field, however I was confused when I didn't then see the view mode as an option in the dropdown. I refreshed the page and my new view mode was then available to select.

My next thought was I needed to configure the new view mode but did not know where to do that. I completely missed the link to configure the new view mode. It was not obvious to me that the "Default" term had been replaced by my new view mode label in the link "Configure x view mode"

Once I figured that out it wasn't clear if I should click the link to configure my new view mode or click Update to save my choice of view mode.

I selected the link to configure my new view mode, however nothing changed (the screen just flashed briefly). UI seems to be broken at that point.
note - the Update option did work as expected.

marcoscano’s picture

Assigned: marcoscano » Unassigned
xjm’s picture

This no longer needs committer review. We'll want to revisit this before we add Media to Standard.

Thanks everyone!

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.