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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan created an issue. See original summary.

larowlan’s picture

Status: Active » Needs review
FileSize
1000 bytes
1.73 KB
larowlan’s picture

Issue tags: +Bug Smash Initiative

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

amjad1233’s picture

+++ 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

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

phenaproxima’s picture

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

Thanks @larowlan. LGTM as well.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

This needs a reroll.

karishmaamin’s picture

FileSize
2.03 KB

Re-rolled patch

larowlan’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.48 KB

Reroll

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
phenaproxima’s picture

Adjusting credit.

alexpott’s picture

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.