Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Fix support of custom mime types with parameters#18255
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| if (in_array($mimeType, (array)$mimeTypes)) { | ||
| if (in_array($extendedMimeType, (array)$mimeTypes)) { | ||
| return$format; | ||
| }elseif (isset($mimeType) &&in_array($mimeType, (array)$mimeTypes)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be justif notelseif as you callreturn in firstif.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch, thank you
| * @return string|null The format (null if not found) | ||
| */ | ||
| publicfunctiongetFormat($mimeType) | ||
| publicfunctiongetFormat($extendedMimeType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think it's necessary to change this variable name.$mimeType should be enough. It doesn't matter if the MIME type has parameters or not, both are MIME types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How would we call the$mimeType without parameters then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
From the outside of this method, we always pass a MIME type, so I think that the argument should be called$mimeType. Internally you also check the MIME type without arguments, so maybe that could be called$canonicalMimeType ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 changed.
70d2130 toe75d7f0Comparefabpot commentedMar 25, 2016
👍 |
| if (in_array($mimeType, (array)$mimeTypes)) { | ||
| return$format; | ||
| } | ||
| if (isset($canonicalMimeType) &&in_array($canonicalMimeType, (array)$mimeTypes)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would probably rather initialise$canonicalMimeType withnull and omit theisset() check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But this would force to callin_array() which is slower in case there is no cannonical mime-type, theisset() here prevents this useless check (in_array(null, (array) $mimeTypes)).
Instead ofisset() when the variable is initialized withnull, could be strict check:
if (null !==$canonicalMimeType &&in_array($canonicalMimeType, (array)$mimeTypes)) {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes, that check should be added then instead ofisset()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
updated.
xabbuh commentedMar 25, 2016
👍 Status: Reviewed |
fabpot commentedMar 25, 2016
Thank you @Ener-Getick. |
…ameters (Ener-Getick)This PR was merged into the 2.3 branch.Discussion----------[HttpFoundation] Fix support of custom mime types with parameters| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |FriendsOfSymfony/FOSRestBundle#1399| License | MITWhen using mime types with parameters, ``getFormat`` won't return the expected format as illustrated:```php$request = new Request();$request->setFormat('custom', 'app/foo;param=bar');$request->getFormat('app/foo;param=bar');// will return null as the parameters are removed```So my proposal is to search the format corresponding to a mime type with its raw value or with the its parameters removed.Commits-------f7ad285 [Request] Fix support of custom mime types with parameters
When using mime types with parameters,
getFormatwon't return the expected format as illustrated:So my proposal is to search the format corresponding to a mime type with its raw value or with the its parameters removed.