Problem/Motivation

\Drupal\media\Controller\OEmbedIframeController::render() sets the cache max age to 0 on its response by accident, by doing $response->addCacheableDependency(Url::createFromRequest($request)); when \Drupal\Core\Url does not implement \Drupal\Core\Cache\CacheableDependencyInterface

Steps to reproduce

  • Create a media type using using oEmbed as a media source (using the core "remote_video_media_type" recipe for example)
  • Add a media of this type and display it on a page
  • The response returned by the media.oembed_iframe route (/media/oembed route) will have a max age of 0 and be uncacheable.

Proposed resolution

Seeing changes like https://www.drupal.org/node/3525389 makes me think this should be fixed.
We could use regular cache contexts to achieve the intended effect.

Remaining tasks

Make the change an create a MR.

User interface changes

None.

Introduced terminology

None.

API changes

None.

Data model changes

None.

Release notes snippet

Todo.

CommentFileSizeAuthor
#9 3560260-nr-bot_dfyg3_n0.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3560260

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

arousseau created an issue. See original summary.

arousseau opened merge request !13969

arousseau’s picture

arousseau commented
Status: Active » Needs review
arousseau’s picture

arousseau commented
Issue summary: View changes
smustgrave’s picture

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

Possible to get a test case showing the problem please

arousseau’s picture

arousseau commented
Status: Needs work » Needs review
Issue tags: -Needs tests

I added tests + fixed the case where an exception occurs, while fetching the resource. The max age in that case was permanent, and now is zero as intended.

smustgrave’s picture

smustgrave commented

Going to leave in review but think there has to be a different way to get those tags. If we add a new setting or a contrib does this list will have to be managed and updated each time.

arousseau’s picture

arousseau commented

I added the settings config object as cacheable dependency (which adds the config:media.settings cache tag to the response).
So the response should be invalidated when the config is updated.

I switched to cacheable bad request exceptions, to make use of the cacheability metadata.

Preprocess allows additional cacheability metadata to be provided by other modules see (\Drupal\media_test_oembed\Hook\MediaTestOembedThemeHooks::preprocessMediaOembedIframe())

Pipeline is failing, possibly linked to #3557585: Update to Composer 2.9.2

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3560260-nr-bot_dfyg3_n0.txt91 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.

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

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Status: Needs work » Needs review

Rebased. Tests are now passing.

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative, +Bug Smash Initiative

Believe my original concerns were addressed.

  • longwave committed 3186130d on 11.x 
    fix: #3560260 OEmbedIframeController makes responses uncacheable by...

  • longwave committed 867a3c06 on main 
    fix: #3560260 OEmbedIframeController makes responses uncacheable by...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 867a3c069c9 to main and 3186130d308 to 11.x. Thanks!

Did not backport cleanly to 11.3.x or 10.6.x; not sure this is worth backporting there but if anyone feels strongly then please open backport MRs.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave closed merge request !13969

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.