Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[HttpFoundation] Fix BinaryFileResponse content type detection logic#47746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@X-Coder264
Copy link
Contributor

@X-Coder264X-Coder264 commentedSep 30, 2022
edited
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
Tickets-
LicenseMIT
Doc PR-

I've just upgraded to 5.4.13 (from 5.4.8) and a lot of my tests started failing. Upon further debugging I've found the issue to be caused by#47516

Prior to that PR

if (!$this->headers->has('Content-Type')) {$this->headers->set('Content-Type',$this->file->getMimeType() ?:'application/octet-stream');        }

was called before theparent::prepare($request); call.

After that PRparent::prepare($request); was called first and that would call

// Content-type based on the Requestif (!$headers->has('Content-Type')) {$format =$request->getRequestFormat(null);if (null !==$format &&$mimeType =$request->getMimeType($format)) {$headers->set('Content-Type',$mimeType);                }            }

which would set the content type from the request so by the time

if (!$this->headers->has('Content-Type')) {$this->headers->set('Content-Type',$this->file->getMimeType() ?:'application/octet-stream');        }

is executed again in the BinaryFileResponseprepare method thatif statement never evaluates totrue because theContent-Type header was already set (to the wrong one).

This PR fixes this issue by making sure the order of the method calls is correct.

fritzmg and axi reacted with thumbs up emoji
@X-Coder264X-Coder264force-pushed thefix-binary-file-response-content-type-detection-logic branch from2555feb to87f58aeCompareSeptember 30, 2022 14:31
@fabpot
Copy link
Member

Thank you@X-Coder264.

X-Coder264 reacted with thumbs up emoji

@fabpotfabpot merged commita91e213 intosymfony:4.4Sep 30, 2022
@X-Coder264X-Coder264 deleted the fix-binary-file-response-content-type-detection-logic branchSeptember 30, 2022 16:04
@junowilderness
Copy link
Contributor

@X-Coder264 Our automated tests detected this one also. You found the root cause faster than I did. Thank you.

X-Coder264 reacted with thumbs up emoji

@alcohol
Copy link
Contributor

Heh just ran into this one today when upgrading from 4.4.45 to 4.4.46; added a conflict rule for now to our composer.json - cheers for the quick fix!

X-Coder264, xabbuh, and calebhartman reacted with thumbs up emoji

@Trismegiste
Copy link

Trismegiste commentedOct 5, 2022
edited
Loading

Same problem here with an upgrade from5.4.11 to5.4.13

With this simple controller which should return a PNG :

/** @Route("/yolo") */publicfunctionyolo()    {returnnewBinaryFileResponse('/www/var/storage/test/yolo.png');    }

I get this :
image

The content-type of the picture istext/html; charset=UTF-8

It works OK when the picture is in a html page (the browser is fixing the faulty content-type on the fly) but my functional tests do not pass.

If I force the content-type with areturn new BinaryFileResponse('/www/var/storage/test/yolo.png', 200, ['content-type' => 'image/png']); it works.

Will this fix be released in 5.4.14 ?

adevade reacted with thumbs up emoji

@xabbuh
Copy link
Member

Will this fix be released in 5.4.14 ?

yes

Trismegiste reacted with thumbs up emoji

@gimler
Copy link
Contributor

gimler commentedOct 6, 2022
edited
Loading

@Trismegiste i had the same problem quick fix manual add mime type

/** @Route("/yolo") */public function yolo(){    $response = new BinaryFileResponse('/www/var/storage/test/yolo.png');    $mimeTypes = new MimeTypes();    $mimeType = $mimeTypes->guessMimeType('/www/var/storage/test/yolo.png');    $response->headers->set('Content-Type', $mimeType);    return $response;}

@Trismegiste
Copy link

yes

Thank you@xabbuh

@Trismegiste i had the same problem quick fix manual add mime type

Thank you@gimler but I think I'll keep 5.4.11 and upgrade later

@derrabus
Copy link
Member

Thank you@gimler but I think I'll keep 5.4.11 and upgrade later

… or use"symfony/http-foundation": "~5.4.14@dev" as version constraint for the time being. This way you can verify that the upcoming release will address your issue.

axi reacted with thumbs up emoji

@marin246
Copy link

When will this be released?

@xabbuh
Copy link
Member

Patch releases happen once per month.

@X-Coder264
Copy link
ContributorAuthor

Is there an exception to that? This is not some rare/small/obscure issue, this is a pretty big issue with a lot of impact as almost every project uses theBinaryFileResponse which can be seen by the amount of bug reports that are constantly being opened all over the place (here, on Symfony community bundles, on the Laravel framework repo etc.).

Honestly I was a bit surprised that a new patch version was not released immediately after the fix was merged.

ajgarlag reacted with thumbs up emoji

This was referencedOct 12, 2022
@xabbuh
Copy link
Member

releases for all maintained versions containing this fixed have just been released

X-Coder264, swichers, and jjsty1e reacted with hooray emoji

@Trismegiste
Copy link

Tested right now.
5.4.14 release fixed my problem with BinaryFileResponse
Thanks ❤️

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofstof approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

12 participants

@X-Coder264@fabpot@junowilderness@alcohol@Trismegiste@xabbuh@gimler@derrabus@marin246@nicolas-grekas@stof@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp