Problem/Motivation

\Drupal\media\Controller\OEmbedIframeController::render doesn't set a content-type on the response.
But the page is text/html. Browers guess this but it causes issues for third-party code - e.g. Tome cannot ascertain if the response contains HTML correctly - see #3104436: [PP-1] Oembed style oembed.frame is not exported

If you contrast the controller with \Drupal\Core\Render\MainContent\HtmlRenderer::renderResponse, you'll note it sets a header.

Steps to reproduce

Fetch the media oembed route in a sub request, note there is no content-type header on the response

Proposed resolution

Set the content type header

Remaining tasks

Add tests

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 3230547-11.patch1.48 KBlarowlan
#10 3230547-10.patch2.03 KBkarishmaamin
#6 3230547-6.patch1.75 KBlarowlan
#6 interdiff-f0c2b7.txt784 byteslarowlan
#2 3230547-pass.patch1.73 KBlarowlan
#2 3230547-fail.patch1000 byteslarowlan

Comments

larowlan created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs review
StatusFileSize
new 3230547-fail.patch1000 bytes
new 3230547-pass.patch1.73 KB
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Bug Smash Initiative

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

amjad1233’s picture

amjad1233
MJED
Location Brisbane
commented 
+++ b/core/modules/media/src/Controller/OEmbedIframeController.php
@@ -133,7 +133,9 @@ public function render(Request $request) {
+    $response = new HtmlResponse('', 200, [

Would it not be good to $response->prepare($request); as it does the pretty much the same thing you're trying to achieve?

If not instead of 200 it would be good to use HtmlResponse::HTTP_OK;

Although not very fuss about it.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
StatusFileSize
new interdiff-f0c2b7.txt784 bytes
new 3230547-6.patch1.75 KB

Prepare seems to do a lot more, so just made the change to use the constant

phenaproxima’s picture

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

I don't think I have anything to complain about here. Seems like a simple, sensible change. Let's bring 'er in.

amjad1233’s picture

amjad1233
MJED
Location Brisbane
commented

Thanks @larowlan. LGTM as well.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

This needs a reroll.

karishmaamin’s picture

karishmaamin commented
StatusFileSize
new 3230547-10.patch2.03 KB

Re-rolled patch

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3230547-11.patch1.48 KB

Reroll

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Adjusting credit.

alexpott’s picture

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

Committed and pushed 0a5beb304b to 9.3.x and 5a2387514e to 9.2.x. Thanks!

I credited @karishmaamin - thanks for the attempted re-roll. In future it is a good idea to run any tests changed locally. It gives a you a good idea if the re-roll has worked as expected.
I also credited @amjad1233 for a review that contributed to the patch.

  • alexpott committed 0a5beb3 on 9.3.x 
    Issue #3230547 by larowlan, karishmaamin, amjad1233: \Drupal\media\...

  • alexpott committed 5a23875 on 9.2.x 
    Issue #3230547 by larowlan, karishmaamin, amjad1233: \Drupal\media\...

Status: Fixed » Closed (fixed)

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