Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#11 | 3230547-11.patch | 1.48 KB | larowlan |
#10 | 3230547-10.patch | 2.03 KB | karishmaamin |
#6 | interdiff-f0c2b7.txt | 784 bytes | larowlan |
#2 | 3230547-pass.patch | 1.73 KB | larowlan |
#2 | 3230547-fail.patch | 1000 bytes | larowlan |
Comments
Comment #2
larowlanComment #3
larowlanComment #5
amjad1233Would 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.
Comment #6
larowlanPrepare seems to do a lot more, so just made the change to use the constant
Comment #7
phenaproximaI don't think I have anything to complain about here. Seems like a simple, sensible change. Let's bring 'er in.
Comment #8
amjad1233Thanks @larowlan. LGTM as well.
Comment #9
alexpottThis needs a reroll.
Comment #10
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled patch
Comment #11
larowlanReroll
Comment #12
phenaproximaComment #13
phenaproximaAdjusting credit.
Comment #14
alexpottCommitted 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.