Problem/Motivation

The Media Embed filter plugin currently removes contextual links from embedded media. The code mentions:

// - Contextual Links do not make sense for embedded entities; we only allow
//   the host entity to be contextually managed.

However, I think the ability to contextually manage items is exactly why I'd rather embed media than insert an image directly in my content. For example, I could have an "Attribution" field on an image bundle, and want to provide users the ability to edit that attribution wherever that image appears. Same idea with some image style implementations that use cropping, if you are editing content, and see a weird crop in an embedded image, it's a much better UX to edit the image in place, than to click over to the media library, brainstorm what title search might correspond to this image, page thru results...

Proposed resolution

Add a checkbox to the settings form for the Media Embed filter that allows users to opt in to contextual links on embedded media.

Remaining tasks

  1. Add the feature
  2. Write tests?

User interface changes

New option on Media Embed filter settings form, defaulting to current behavior.

API changes

NA

Data model changes

Add new field to filter_settings.media_embed
File: core/modules/media/config/schema/media.schema.yml

    show_contextual_links:
      type: boolean
      label: 'Show contextual links on embedded media to users with appropriate permissions if contextual module is enabled.'
CommentFileSizeAuthor
#38 3174252-provide-option-to-show-contextual-links-for-embedded-media.patch5.91 KBtrackleft2
#34 media-embed-contextual-links-option-3174252-4.patch2.02 KBschiavone
#33 3174252-nr-bot.txt90 bytesneeds-review-queue-bot
#3 media-embed-contextual-links-option-3174252-3.patch2.01 KBxaqrox
#2 media-embed-contextual-links-option-3174252-2.patch1.99 KBxaqrox

Issue fork drupal-3174252

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

xaqrox created an issue. See original summary.

xaqrox’s picture

xaqrox
he/him
Primary language English
Location Washington, D.C.
commented
StatusFileSize
new media-embed-contextual-links-option-3174252-2.patch1.99 KB

Here's a patch. I'm not sure if this needs tests, I have very little Drupal testing experience but willing to try if necessary!

xaqrox’s picture

xaqrox
he/him
Primary language English
Location Washington, D.C.
commented
StatusFileSize
new media-embed-contextual-links-option-3174252-3.patch2.01 KB

Oops, patch had wrong paths, this one should work...

staceroni’s picture

staceroni commented

While you are reviewing a long-form article, that has many <drupal-media> image embeds, and you come across a media item with extra fields, you should be able to use ContextualLinks to be able to jump into that Media item immediately.

The comment // Contextual Links do not make sense for embedded entities isn't clear on why it doesn't make any sense.
So many things on a page -- blocks, menus, entity relationship fields -- support contextual links with the same "sense" as how I would like to be able to manage media within the body of a node.

staceroni’s picture

staceroni commented
Status: Active » Needs work

The patch #3 causes a php warning

Notice: Undefined index: show_contextual_links in Drupal\media\Plugin\Filter\MediaEmbed->settingsForm() (line 187 of core/modules/media/src/Plugin/Filter/MediaEmbed.php).

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jvogt’s picture

jvogt commented
Status: Needs work » Needs review

The php error caused by #3 is resolved with a cache flush. I'm not sure how to force a flush on a core update, so hopefully someone else can help out.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Version: 9.5.x-dev » 10.1.x-dev
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests, +Needs subsystem maintainer review

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Tagging for tests as that will need to happen.
But also tagging for subsystem review as they may have different thoughts about if this should be implemented.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

e.r.n.i.e’s picture

e.r.n.i.e commented

I applied the patch #3 successfully (Drupal 10.1).

👏 Thank you @xaqrox! This is an important UI improvement for my use cases.

4kant’s picture

4kant commented

Patch #3 applied cleanly in D 9.5.10 and in D 10.1.5
That´s exactly what I was looking for. Webmasters are now able to edit and therefore also to crop images...
Thanks a lot @xaqrox!

phthlaap made their first commit to this issue’s fork.

phthlaap opened merge request !5626

phthlaap’s picture

phthlaap commented
Issue summary: View changes
Status: Needs work » Needs review

Added tests and fix some issues of config.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs upgrade path

Still needs submaintainer review but relooking and seeing the schema change realize will need an upgrade path + tests

phthlaap’s picture

phthlaap commented

Tests already there. Can you help to suggest what is the upgrade path?

smustgrave’s picture

smustgrave commented

When making a schema change that would appear in config export an upgrade path needs to be included that will add that setting to existing sites, setting to null. Then a simple test for the upgrade path that
- checks config doesn't exist
- run updates
- checks config now exists with default vlaue

phthlaap’s picture

phthlaap commented

Can you please help provide sample code in any module?
Thanks.

phthlaap’s picture

phthlaap commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment.

trackleft2 made their first commit to this issue’s fork.

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Status: Needs work » Needs review

I've updated the test to comply with the latest coding standards.
I've moved the update function to a post_update.php file.
I've add batch functionality to the update.

I've tested that the update works locally and on an existing site by
1. exporting text format config and looking at the media_embed filter section and seeing missing key.
2. running the update
3. exporting text format config and looking at the media_embed filter section and seeing key there.

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented

Created draft change record feel free to edit or update. https://www.drupal.org/node/3483890

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: -Needs tests, -Needs upgrade path +Needs upgrade path tests

With the update hook will need it's own test case as well.

Thanks

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Status: Needs work » Needs review

Added a new fixture database and test for update following this guide https://www.drupal.org/docs/drupal-apis/update-api/writing-automated-upd...

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment on MR.

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented

Not sure we need a full dump like this. This doesn't need to be unique to umami and could probably use one of the existing figures in system.

The issue is that the standard recipe does not have a format that has a media_embed filter enabled.

OK, I'll do it the hard way.

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Status: Needs work » Needs review

I've updated the database fixture to include just what is added into the database when the media module is enabled plus two text formats that have the media_embed filter enabled.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3174252-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

schiavone’s picture

schiavone commented
StatusFileSize
new media-embed-contextual-links-option-3174252-4.patch2.02 KB

Updated the patch for version 10.3.10

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
Status: Needs work » Needs review

Don't use the patch in #34 if you intend to use this module as there is currently no upgrade path for changing the config schema data type.

If you do use the patch and then remove it, you should be able to fix your site configuration so that it matches the new schema by simply resaving the setting after updating the codebase.

schiavone’s picture

schiavone commented

@trackleft2 The only difference between the updated patch in #34 and the patch in #3 is an update of the line number that sets the default setting to show the contextual link. So should the same apply to both patches? If there's a problem with updating the schema should we create an update?

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented

@schiavone the main difference between the patch and the merge request is the core/modules/media/config/schema/media.schema.yml file, in which the data type is set to boolean which creates a small discrepancy in the active config for anyone who updates from the patch to the merge request.

This is because the patch in #34 sets the value as an integer.

I haven't heard of creating an update for migrating from one version of a patch to another, so I wouldn't support that idea.

trackleft2’s picture

trackleft2
Primary language English
Location Tucson, AZ 🇺🇸
commented
StatusFileSize
new 3174252-provide-option-to-show-contextual-links-for-embedded-media.patch5.91 KB

Providing an updated patch without tests against the 11.x branch

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

There's also filter.format.basic_html.yml files in standard and demo_umami profiles that will need to be updated.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented

I don't remember why we thought contextual links did not make sense for embedded media. I think it is because the use cases are relatively complex. Not having contextual links for the simple use cases in core was ok for the moment. When embedding a document link, the contextual links seem like a bit of overkill, but when embedding a slideshow, or something complex like that, it totally makes sense.

My first feeling is it depends on the view mode / media type whether contextual links make sense. At least per media type. Can we change the configuration of the filter to allow which media types/view modes should have contextual links?

Maybe we should even move the configuration to the media type / view mode config so we can also use this for entity references for example?

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.