Umami missing some Media "plumbing" found in Standard profile

#2883813: Move File/Image media type into Standard once Media is stable has some background info on the reasoning behind why this is on the profile and not the module.

> (Site-note: I'm confused as to why the sane 90% default behaviour in Standard is controlled on a per-profile basis, and not a default set by the module for all profiles, so tagging "Media Initiative" as well to get some input about that in case we want to solve it the opposite way instead, since I imagine all other contrib/custom profiles will hit this as well.)

Because it is something that the profile wants to control. Maybe right now you'd want to have the same basics in umami as there is in standard. But that would only be temporary. What Umami wants to provide is media view modes and image style configuration that matches its configuration for its current non-media approach. Because at some point, it will have to switch its current configuration to actually *use* media in all its content structures. And then what standard now contains would just get in the way, just like it gets in the way of our custom install profile.

This is something that only got in with 8.5.x, see #2883813: Move File/Image media type into Standard once Media is stable. So in our custom install profile, we have to add that patch in 8.4 to not end up with duplicate media types and other configuration we don't want/have differently.

cross-posted with marcos :)

This should deal with the missing media types.

However, for the second issue, it may not be that straightforward... Is there a reason a full-page article is using the "full" viewmode instead of the "default"? There is code in the media module to ensure that media-reference fields are displayed by default (using "sane" defaults) in the "default" viewmode. For the other "custom" viewmodes, I fear there is little we can do there? Maybe I'm just missing something here... please let me know if so!

Sorry some unintended changes slipped into the previous patch.

After speaking with @markconroy he explained me the reasoning behind it and it makes sense :)

So after playing around a bit I couldn't find any straightforward (or at least reasonable) solution for this issue without modifying the API. So I proposed #2940100: Add an alter hook to allow modules to modify default field settings when added to default form/view displays as a tool to reach this goal. If in the end that issue is wont-fixed, we would need to figure out some more hackish ways of copying those settings to the full viewmode when a media field is created.

This patch shows the idea in action, it apparently works as expected.

Also, I've spent quite a while on the failing test, but still couldn't figure out what's going on. Apparently when the path module is present and we install optional config, bad things happen inside \Drupal\KernelTests\AssertConfigTrait::assertConfigDiff. Still need to keep diving, but if anyone wants to take a stab at it, feel free!

(Triggering the testbot just to have the more recent failure available)

Also, forgot to mention that the new hook is implemented inside demo_umami.profile in this patch, but an argument could be made that we should probably implement it directly in media.module instead.

Consider the following scenario:
- A user installs Drupal 8.5.0 vanilla, standard profile
- Then enables the "Full content" display in admin/structure/types/manage/article/display
- Then enables the media module
- Then adds an entity_reference "media" field to article nodes

All of a sudden all work done on issues such as
#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
#2934162: Provide better defaults to media form displays
#2934850: Media Images should be rendered at a reasonable size by default
#2865184: Allow MediaSource plugins provide default field form/view display settings
#2930788: Do not show name by default in media displays
(and probably others) is lost, because these improvements don't propagate from default to full.

So at this point I believe the question we should ask is:
"Should we assume that media fields on nodes should always be made visible by default on the full viewmode as well, if this display exists?"

OK, this should fix the failing test. (This issue has surfaced #2940205: Some media default configuration does not install as expected, but doesn't really depend on that)

I'm triggering the testbot, but this should still be postponed on #2940100: Add an alter hook to allow modules to modify default field settings when added to default form/view displays

Since #2939563: Un-hide Media from the UI is not being added to 8.5, we see this as a beta blocker for 8.6 only, not an immediate beta blocker.

Not sure we should be adding a field_ui hook in an Umami demo profile issue :) How would this affect those using the API to create fields? Looks like there is one thing to fix the default experience and then another to fix creating new fields (that are not present in the demo).

@marcoscano,

After speaking with @markconroy he explained me the reasoning behind it and it makes sense :)

So after playing around a bit I couldn't find any straightforward (or at least reasonable) solution for this issue without modifying the API. So I proposed #2940100: Add an alter hook to allow modules to modify default field settings when added to default form/view displays as a tool to reach this goal. If in the end that issue is wont-fixed, we would need to figure out some more hackish ways of copying those settings to the full viewmode when a media field is created.

Standard has default (sane) settings for the default view mode. If a new view mode is switched on (in Standard), that view mode will also get those (sane) settings. That's the point of the default view mode - share settings among all view modes. However, once the new view mode is switched on, then any new changes to default view mode will not be propagated to the new view mode. That's fine, and as I should be.

we would need to figure out some more hackish ways of copying those settings to the full viewmode when a media field is created.

I'm not sure we need to figure this out. When any field is created, standard does not transfer the settings for that field from default view mode to teaser (or any other view mode); it only does it to new view modes when they are enabled. This is the same as what happens with Umami. Our view modes configure our settings they way we want them, and new fields will not be automatically added to those view modes. We have to do that manually and export the config.

I think (and to be honest, I don't know a huge amount about this current issue (just reading it now)) we might just need to configure our full view mode settings for media when we move to media for Umami, and then export that config for the next version.

Maybe it all boils down to a matter of expectations, from a product point of view.

If I understand it correctly, this issue was open because the media "out-of-the-box experience" in Umami (which is using the full view display) is different from the media "out-of-the-box experience" in standard (which doesn't use any custom viewmode, so defaulting to the default display). If we really want both experiences to be equivalent, we need to either make Umami stop using the customized display, or use some specific code to propagate the settings from default to full.

That's the point of the default view mode - share settings among all view modes.

I'm still learning about all this problem space, but after going through #2844203: Improve/Simplify situation around Default/Full view modes/view displays and related issues, I feel that this statement was true at the beginning, but probably not 100% accurate anymore :), especially seeing that "Default" is exposed as an option in many places on the UI, the standard profile doesn't ship with full view displays for articles and pages, etc.

@markconroy

> I think (and to be honest, I don't know a huge amount about this current issue (just reading it now)) we might just need to configure our full view mode settings for media when we move to media for Umami, and then export that config for the next version.

I think I very much agree with this and also what @amateescu wrote in the other issue. I don't think Umami should care much about media until it actually switches fully to media itself.

Umami doesn't *need* to be 100% identical to Standard, it doesn't serve the same purpose. Standard needs to work well as a starting point to build any D8 site. Umami is a specific example, there is no reason to specifically support enabling media and using it unless it itself switches to using Media. Which it should do, as soon as we unhide it and it's good enough to work out of the box without contrib modules. Which will hopefully be the case in 8.6?

So I think either this should be converted to a (probably postponed for now, unless we want to start trying to see how well it works. Might make more sense to test it with Umami first before switching standard?) task to convert umami to use media or it should be closed and a new issue for that should be created. Maybe that actually exists already?

That said, I generally do not recommend to enable and customize the the full view display as it IMHO breaks expectations of what should happen when new fields are added. I would like to better understand why it was done for Umami, maybe you can add that to #2844203: Improve/Simplify situation around Default/Full view modes/view displays and your general thoughts on that issue and my suggestions there?

Postponing this for now. We are focussing on 8.5 for the next few days, and will not be enabling media in Umami until at least 8.6.

+++ b/core/profiles/demo_umami/config/optional/core.entity_form_display.media.audio.default.yml
@@ -0,0 +1,58 @@
+id: media.audio.default

+++ b/core/profiles/demo_umami/config/optional/core.entity_form_display.media.file.default.yml
@@ -0,0 +1,58 @@
+id: media.file.default

+++ b/core/profiles/demo_umami/config/optional/core.entity_form_display.media.image.default.yml
@@ -0,0 +1,60 @@
+id: media.image.default

+++ b/core/profiles/demo_umami/config/optional/core.entity_form_display.media.video.default.yml
@@ -0,0 +1,58 @@
+id: media.video.default

+++ b/core/profiles/demo_umami/config/optional/core.entity_view_display.media.audio.default.yml
@@ -0,0 +1,29 @@
+id: media.audio.default

Is there a reason why we don't put these media types in media module itself?

Or at least one of them.

The issue here was reported because with umami there are no types, so the add field thing is a bit naff.

But copying these to every profile? Seems like a better solution might be for media to ship with one sensible type - or just file and image as they are commonly part of core.

They can be in the config/optional so they won't get installed if file/image module aren't enabled.

@larowlan: We actually moved it, see the issue in #1. It's just as likely that default will get in the way for an install profile. For example in thunder, media types are prefixed since the beginning, and you don't want to have different types whether you installed with media in core or media_entity before. The update path for example also had to explicitly work around that by cleaning up again and deleting those types again.

In our install profile, we have multiple image media types/bundles, also prefixed. I don't think there is a "sensible default" that everyone wants. Standard *is* the sensible default for plain new installations. Other install profiles should do their own thing. I'd rather move more things into standard than the other way round, for example admin views like files/content, because it is really annoying to try and provide customized versions in install profiles due to config dependencies.

The default comment type is for example also in standard, this is pretty similar.

Thanks Berdir, that makes sense

We should try to get this done for 8.6

So I would propose we just enable media and add the "standard" media types to Umami. Then in #2954378: Use Media images in Umami demo we can adjust the displays and the data import.

+1 for getting this in before 8.6. We should make it easy for people installing Umami to play with media, even if we haven't yet started using it ourselves.

Try to fix the tests

Here is a new patch that adds the new oEmbed media type.

Reviewing this shows we have taken the configuration for the five media types in standard identically with a couple of exceptions:

core.entity_form_display.media.file.default.yml and core.entity_form_display.media.image.default.yml have a dependency on path module in Umami that is not present in Standard.

This is because there is an order of operations with Standard that can cause no media items to be created on install - if path is uninstalled before media in installed, no media types are created. The config for media types in Standard are a little inconsistent whilst the best way forward on this is found. The Umami versions are consistent in requiring Path, and this specific issue is not a concern with Umami, where Media is enabled at logon.

See https://www.drupal.org/project/drupal/issues/2934991#comment-12410875 for further details on the Media in Standard discussion.

core.entity_view_display.media.file.default.yml and core.entity_view_display.media.image.default.yml are functionally identical but have hidden: name in a different order.

The Umami versions are in alphabetical order so seem more correct.

Following the above config review, I've A/B tested 8.6.x Standard install with media and responsive image subsequently installed with an Umami install with this patch installed.

The media module is enabled in Umami, and all the configured media types available in Standard are available in Umami.

Therefore marking this RTBC.

Adjusting credit.

Committed 5e07c02 and pushed to 8.6.x. Thanks!