Problem/Motivation

\Drupal\media\Controller\OEmbedIframeController::render calls $this->renderer->executeInRenderContext but doesn't consider if any metadata was bubbled in the context.
Themes/modules can implement hook_preprocess for media_oembed_iframe and potentially inject additional attachments or cache metadata by modifying the #cache or #attached keys.

Steps to reproduce

Implement hook_preprocess_media_oembed_iframe and either modify #attachments or #cache
Note that these changes don't bubble.

Proposed resolution

Inspect the context and if there is bubbling, respect it.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 3230772-8.patch5.22 KBlarowlan
#8 interdiff-a51d2a.txt1.03 KBlarowlan
#6 3230772-6.patch4.87 KBlarowlan
#6 interdiff-a5b860.txt2.3 KBlarowlan
3230772-fail.patch2.64 KBlarowlan
#2 3230772-pass.patch4.36 KBlarowlan
#2 3230772-fail.patch2.64 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Active » Needs review
FileSize
3230772-fail.patch2.64 KB
3230772-pass.patch4.36 KB

Pass/fail

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

The last submitted patch, 2: 3230772-fail.patch, failed testing. View results

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

Two questions.

  1. $variables['#attached'] is something I've never seen. I thought that we had to attach things to specific render elements, rather than entire variable arrays passed to preprocess hooks. Could you perchance point me to the place where the theme system handles $variables['#attached']?
  2. The bigger question concerns the bubbling of cacheability metadata. Is that something we have test coverage for? If not, should we add some here?
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext for Australian Digital Health Agency commented
FileSize
interdiff-a5b860.txt2.3 KB
3230772-6.patch4.87 KB

1. See https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/lib/Drupal/C... - basically if any preprocess function changes #cache or #attachments, then this bubbles
2. Done

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work 
+      if (!$context->isEmpty()) {
+        $bubbleable_metadata = $context->pop();
+        assert($bubbleable_metadata instanceof BubbleableMetadata);
+        $response->addCacheableDependency($bubbleable_metadata);
+        $response->addAttachments($bubbleable_metadata->getAttachments());
+      }

One small request: can you add a blank line above this, plus a comment to explain what it's doing (which, as I understand it, boils down to preserving attachments and cacheable metadata added during the trip through the render system)? When that's done, this is RTBC as far as I'm concerned.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext for Australian Digital Health Agency commented
Status: Needs work » Needs review
FileSize
interdiff-a51d2a.txt1.03 KB
3230772-8.patch5.22 KB

Done

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Complaints remaining: zero. Cut and print!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: +Bug Smash Initiative

I think this qualifies as a bug, so I'm tagging it for the Bug Smash Initiative.

ac’s picture

ac
Primary language English
Location Perth
CreditAttribution: ac commented

Confirming this both works and solves a problem. +1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 0cbd31dbb8 to 9.3.x and 5b2ffba9f2 to 9.2.x. Thanks!

This looks like a neat and self-contained bug fix. Backporting to 9.2.x

  • alexpott committed 0cbd31d on 9.3.x 
    Issue #3230772 by larowlan, phenaproxima: OembedMediaController doesn't...

  • alexpott committed 5b2ffba on 9.2.x 
    Issue #3230772 by larowlan, phenaproxima: OembedMediaController doesn't...

Status: Fixed » Closed (fixed)

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