Media entities should support contextual links

+++ b/templates/media.html.twig
@@ -13,6 +13,8 @@
+  {{ title_suffix.contextual_links }}
+

Can we print entire title_suffix here?

When was this class added to the Entity module? Do we need to raise minimum version of the dependency?

I tested this with simplytest.me. The patch worked exactly as described.

Ah, video proof! Love it. :) Thank you, @gippy and @chr.fristch!

So, this seems like an issue that is quite possibly affecting the core Media system as well. We should definitely fix it there, since we've pretty much halted development on Media Entity 1.x in favour of core Media. We'll certainly need automated tests for this, but I see no reason why we shouldn't address this once #2831274: Bring Media entity module to core as Media module lands.

I'll be adding this to the list of issues that comprise the core Media MVP: #2825215: Media initiative: Roadmap

This issue is now part of the core Media Initiative. Postponed on #2831274: Bring Media entity module to core as Media module.

#2831274: Bring Media entity module to core as Media module is in! No longer postponed.

+++ b/src/Entity/Media.php
@@ -20,7 +20,7 @@ use Drupal\user\UserInterface;
- *     "view_builder" = "Drupal\Core\Entity\EntityViewBuilder",
+ *     "view_builder" = "Drupal\entity\EntityViewBuilder",

relying on the entity module certainly doesn't fly :) Maybe we could bring this particular class into Drupal core?

Oooh, nice catch. It seems like bringing the view builder from Entity API to core would be a giant pain in the ass in relation to the scope of this issue, though...surely we can do it some other way that doesn't involve moving parts of Entity API into core?

Added a new class: MediaViewBuilder and adapted code from NodeViewBuilder to add contextual links. Also added the title_suffix to classy and stable media.html.twig, so this should be tested in those themes.

1. +++ b/core/modules/media/src/MediaViewBuilder.php
   @@ -0,0 +1,41 @@
   +/**
   + * Class MediaViewBuilder - View builder handler for media.
   + *
   + * @package Drupal\media
   + */
   

   /**
    * View builder handler for media.
    */
   

   Would be sufficient and more in line with other docs in core.

2. +++ b/core/themes/classy/templates/content/media.html.twig
   @@ -20,8 +20,14 @@
   +  {% if label and view_mode != 'full' %}
   

   template_preprocess_media provides no label variable. Its "name" in this case.

Added a new patch with suggestions from @webflo. Still works ;)

1. +++ b/core/modules/media/src/MediaViewBuilder.php
   @@ -0,0 +1,39 @@
   +    if ($entity->id()) {
   

   As far as I know, the only circumstances under which $entity->id() would be falsy is if the entity is new...in which case we should be using the more generalized if (!$entity->isNew()) {...}

2. +++ b/core/modules/media/templates/media.html.twig
   @@ -40,12 +40,11 @@
   -  {% if label and view_mode != 'full' %}
   +  {% if name and view_mode != 'full' %}
        <h2{{ title_attributes }}>
   -      {{ label }}
   +      {{ name }}
   

   I don't understand why this was done, or how it works. Where is name being set? I'm sorry if I'm being dense here, I'm just not sure what this change is doing in the context of this patch.

3. +++ b/core/themes/classy/templates/content/media.html.twig
   @@ -20,8 +20,14 @@
   +  {{ content }}
   

   It looks like we're now echoing content whether or not it is empty, which is a change from the way it was before. Why is that being done? As with my previous comment, I'm not against it but I'm wondering why it was done.

@phenaproxima - thanks for your feedback!
Just a few words for 2 and 3, I'll check 1 later.

2.) The name variable comes from core/modules/media/media.module - there is a preprocess function: template_preprocess_media.

3) If content variable is empty, nothing gets rendered in Twig, so there is no need to check, if it is empty. We will not get an exception. Also this is how it is done in node template for example. No if statement.

Re-tagging. The patch looks good to me, but it still needs tests.

1. +++ b/core/modules/media/templates/media.html.twig
   @@ -37,12 +37,11 @@
   diff --git a/core/modules/media/templates/media.html.twig b/core/modules/media/templates/media.html.twig.orig
   
   diff --git a/core/modules/media/templates/media.html.twig b/core/modules/media/templates/media.html.twig.orig
   similarity index 100%
   
   similarity index 100%
   copy from core/modules/media/templates/media.html.twig
   
   copy from core/modules/media/templates/media.html.twig
   copy to core/modules/media/templates/media.html.twig.orig
   

   oops.

2. +++ b/core/modules/media/tests/src/Functional/MediaContextualLinksTest.php
   @@ -0,0 +1,55 @@
   +    $this->drupalGet('media/' . $media->id());
   +
   +    $this->assertSession()->responseContains('data-contextual-id="media:media=' . $media->id() . ':');
   

   should we check the revision as well? Edit: Ah, see below, looks like there actually are no revision contextual links, not sure if this is added then or not.

I'm fine with adding it just to media in this issue, but I'm wondering if we already have an issue to add it to the parent? Seems fairly save to do and shouldn't really conflict with anything as long as the entity type doesn't define contextual links. If there is no issue yet, can we open one?

Edit: We could also introduce the concept of a contextual links handler, the view builder could call that and we could also let it generate the contextual link definitions automatically?

What I'm wondering about a bit is that we add media_revision contextual link info as well, if I understand this correctly, this works through the contextual links group, but neither node nor media actually have revision contextual links?

Postponed on #2791571: Automatically supply contextual links for entities.

+++ b/core/modules/media/templates/media.html.twig
@@ -37,12 +37,11 @@
-  {% if label and view_mode != 'full' %}
+  {% if name and view_mode != 'full' %}
     <h2{{ title_attributes }}>
-      {{ label }}
+      {{ name }}

This seems out of scope; why was this change made?

Other than that, this looks pretty good...

I have manually tested and this works as expected. No big remarks on the patch, except a very minor observation:

+++ b/core/modules/media/tests/src/Functional/MediaContextualLinksTest.php
@@ -0,0 +1,55 @@
+    'system',
+    'node',
+    'field_ui',
+    'views_ui',
+    'media',
+    'media_test_source',
+    'contextual',

Probably only 'contextual', is needed here. The base class already declares all others, and they all get added in the end.

I also don't mind the change mentioned in #29, even if unrelated. So for me this is +1 RTBC.

Assuming the bot doesn't complain, this looks good :)

+++ b/core/themes/stable/templates/content/media.html.twig
+++ b/core/themes/stable/templates/content/media.html.twig
  */
 #}
 <article{{ attributes }}>
-  {% if content %}
-    {{ content }}
+  {% if name and view_mode != 'full' %}
+    <h2{{ title_attributes }}>
+      {{ name }}
+    </h2>
   {% endif %}
+  {{ title_suffix }}
+  {{ content }}
 </article>

Hm, stable is meant to be stable, especially for a stable module :) I would refer to @laurii on what exactly is allowed to be changed with stable templates.

Tagging for review by front-end experts :)

+++ b/core/themes/classy/templates/content/media.html.twig
@@ -20,8 +20,14 @@
-  {% if content %}
-    {{ content }}
+  {{ title_prefix }}
+  {% if name and view_mode != 'full' %}
+    <h2{{ title_attributes }}>
+      {{ name }}
+    </h2>
   {% endif %}
+  {{ title_suffix }}
+  {{ content }}
 </article>

I'm not sure about this.

I understand we need the title prefix/suffix stuff for contextual links to work, correct?

But this makes more changes, it introduces similar title/name logic that node templates have, including the hardcoded view_mode (in case of node it is the is page flag, but that makes little difference).

What this means is that it basically undoes your own recent change to make the display of the name configurable and it will *always* be there. The thing is that #2912298: Make media name available on manage display did not add any actual test coverage that it works, it merely tests that the API set it to configurable, not that the template actually respects that.

If anything, then we need to show content.name and check for that being visible. thinking about it, I'd even expect this to display the title twice, once in content if configured and then additionally hardcoded.

First of all, I'm changing the category to a bug report as this is a bug where media entities are not compatible with contextual links.

Secondly, I noticed that the templates media module is providing do already support contextual links. Therefore this bug only exists when Stable or Classy are being used.

Looking at the media templates it seems like the templates inside the module have been already been off-sync from Stable and Classy and I'm not sure what is the reason for that. I didn't find any information from #2831274: Bring Media entity module to core as Media module related to that.

According to our BC policy, we are not allowed to make changes to Stable and Classy. However, if we can come up with a non-disruptive way to solve this, the maintainer team could consider making an exception in this case.

I'm not sure if I understand the reason for adding the hardcoded title into the template. Could we remove that since it shouldn't be a requirement for supporting contextual links?

Thanks for pointing me here Berdir! This seems to conflict with #2930788: Do not show name by default in media displays. If we use a view mode to render media in entity reference fields by default, this means you will always get the media title. That will also break the change in #2895382: Hide label, thumbnail, etc. for default display of media file and image references.

I don't know enough about how the contextual links work, but hardcoding the H2 is definitely an issue.

+++ b/core/themes/classy/templates/content/media.html.twig
@@ -20,8 +20,14 @@
+  {{ title_prefix }}
+  {% if name and view_mode != 'full' %}
+    <h2{{ title_attributes }}>
+      {{ name }}
+    </h2>
   {% endif %}
+  {{ title_suffix }}

This might explain why the title is not being shown twice :)
#2931057: Media template should use 'name' instead of 'label'

The patch in #32 contains the same change as #2931057: Media template should use 'name' instead of 'label'. Let's fix the other issue first before we continue here.

This should definitely be solved after #2930788: Do not show name by default in media displays is fixed.

For reference, this is a patch rerolled on the top of #2930788-33: Do not show name by default in media displays.

#2930788: Do not show name by default in media displays landed, so we should be good to go with this now.

Thanks, @marcoscano!

Despite the fact that this patch changes Stable, it is a bug fix and therefore probably justifiable from a BC standpoint, as @lauriii pointed out in #38. So I think this is RTBC.

Testbot glitch

+++ b/core/modules/media/templates/media.html.twig
@@ -32,5 +32,6 @@
+  {{ title_suffix }}

+++ b/core/themes/classy/templates/content/media.html.twig
@@ -21,6 +21,7 @@
+  {{ title_suffix }}

+++ b/core/themes/stable/templates/content/media.html.twig
@@ -13,6 +13,7 @@
+  {{ title_suffix }}

This looks super odd... just printing the suffix without the rest. I guess this is because contextual links just add themselves to the title_suffix. Should we add a comment to the template header to explain this? Or we could change this to {{ title_suffix.contextual_links }} which would be self-documenting. Not sure.

Or we could change this to {{ title_suffix.contextual_links }} which would be self-documenting.

I like the idea of that, but there are two problems I can see:

1. Nothing else in core does that, as far as I know.
2. Anyone trying to alter the title_suffix on media items, or add things to it, will be thwarted. Which means this will probably resurface later.

Personally, I think we should just add a comment in the template header to explain, and leave it at that.

Comment works for me.

There's also the theoretical option of re-thinking those special title suffix/prefix variables, how they're named and placed. But I'm not surehow to exactly make changes there in a BC compatible way.

Btw, the node template only adds those if there is a title, meaning not on the node page. This now adds them always, does this mean that there are now also contextual links on the media page? Or is contextual module already not adding them on the full page?

Anyone trying to alter the title_suffix on media items, or add things to it, will be thwarted.

Well they'll be thwarted trying to alter the title or title_prefix too? Won't these problems surface later too? I think doing the minimum necessary to support contextual links is preferable to adding something which looks super obscure and begs more questions - ie? Where's the title this is suffixing? Where's the prefix?

Added a comment to the templates.

Well they'll be thwarted trying to alter the title or title_prefix too? Won't these problems surface later too? I think doing the minimum necessary to support contextual links is preferable to adding something which looks super obscure and begs more questions - ie? Where's the title this is suffixing? Where's the prefix?

IMHO, stuffing the contextual links into {{ title_suffix }} is the source of the confusion, but that was a decision made long ago. Contextual links should be their own template variable, precisely to eliminate such implicit weirdness. But that would be outside the scope of this issue. Follow-up, maybe?

I think the template comment @marcoscano added in #51 should be enough to reduce themer confusion in the short term until the title_suffix and title_prefix can be properly untangled, which will obviously take a much longer time. So I'm moving this back to RTBC.

Testbot misbehaving

I woke up this morning thinking about the principle of least astonishment and I think the solution here violates that. Because if you are printing title_suffix you would expect both the title and the title_prefix to be printed as well. I think the solution that just prints the contextual links is the better and least surprising solution. I can imagine things in custom and contrib that add opening tags to title_suffix and closing things to title_prefix and this is going to be broken by this change.

I agree with alexpott. Showing the contextual links only is more explicit for now. Moving the contextual links to it's own variable would be way better, but that is a separate issue.

Updated the patch.

Discussed with @phenaproxima on slack. tldr; we agreed that {{ title_suffix.contextual_links }} was the best solution with the least possible side effects.

Here's the discussion:

phenaproxima: @alexpott So about the contextual links...IMHO the least astonishing way to do it is to do what every other template in core does
phenaproxima: @alexpott Because if we have to change it later, in stable, we will be breaking BC
alexpott: @phenaproxima so then you have to add the title and title_suffix - which you can't do.
phenaproxima: @alexpott Why? We can just add {{ title_prefix }} and {{ title_suffix }} with no title between them.
alexpott: But that is also astonishing
phenaproxima: @alexpott But is it less astonishing than just showing the contextual links?
alexpott: In my opinion yes. We have no way for things to arbitrarily dump stuff into templates - contextual is using title_prefix to do this and it is super weird. But just printing title_suffix.contextual to enable contextual links in media is totally unastonishing given how contextual links works. Also printing just title_suffix.contextual is the smallest way to fix the actually bug with the least amount of possible side effects
phenaproxima: @alexpott Well, I just asked another opinion and they agree with you, so I guess I concede this one
alexpott: Which, I think is also preferred given the imponderables of what people do on real sites
phenaproxima: @alexpott I'd rather just get contextual links working than debate it for very long =P
alexpott: @phenaproxima "just get contextual links working" = do smallest possible change with least possible side effects :slightly_smiling_face:
phenaproxima: @alexpott Okay then :slightly_smiling_face: We agree. We should have a follow-up, maybe, to untangle contextual links from the prefix/suffix variables

There are issues about how contextual uses title_suffix in the core queue, for example: #2933695: Make contextual links less intrusive to front end templates maybe we need a more generic one.

#52 - #57 seems reasonable.

The current patch though is still making changes to Stable and Classy, which @lauriii raised in #38 with the previous approach because it's a BC break. I think we'd need frontend framework manager's signoff if we were to go ahead with changing the stable base templates.

To avoid breaking BC for the stable base themes, we could make the change to the Media template itself and copy it to Bartik and Seven, but not change the base themes. (A followup could do that later if we decide the bug is buggy enough to break BC for the base themes.)

A second opinion might be good but let's start with that for now and maybe file a followup. I'll also ping @lauriii for his thoughts again.

The latest patch seems to have addressed concerns that I brought up in my previous comment. I also think that the current approach is fine - we don't have to solve problems outside the scope of this issue.

I think on this issue it would be worth taking the risk to break BC and provide this fix in Classy and Stable. This change is in the safer end of potential changes that could be done (adding a new child element inside a pre-existing wrapper). Also, not fixing this bug in Classy and Stable would cause confusing UX because of missing contextual links on some sites.

It would be great if we could have a small change record explaining the change in the markup to make sure developers are aware that this might affect their sites.

Thanks @lauriii! So then we just need the CR and then this can have its final review.

Created CR: https://www.drupal.org/node/2935088

Not sure if the impact on themers should be more explicit, please advise if so.

Thanks!

Then I think we’re ready.

I added a bit more explicit statement about the impacts:
https://www.drupal.org/node/2935088/revisions/view/10782605/10782949

Alright, the approach has signoff from a frontend maintainer, the CR is in place, and I manually tested and confirmed it resolved the issue without any side effects:

Saving issue credit.

Committed to 8.5.x and published the CR. Thanks!

I think #51 rather than #56 was committed here by accident.

Whoops, good spot. Fixed, thanks.

Patch for backport to 8.4.x-dev at https://www.drupal.org/project/drupal/issues/2948838.