[HttpFoundation] Fix support of custom mime types with parameters #18255
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.
Should be just if not elseif as you call return in first if.
There was a problem hiding this comment.
Good catch, thank you
| * | ||
| * @return string|null The format (null if not found) | ||
| */ | ||
| public function getFormat($mimeType) | ||
| public function getFormat($extendedMimeType) |
There was a problem hiding this comment.
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.
How would we call the $mimeType without parameters then?
There was a problem hiding this comment.
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 ?
70d2130 to
e75d7f0
Compare
|
👍 |
| @@ -1250,6 +1250,9 @@ public function getFormat($mimeType) | |||
| if (in_array($mimeType, (array) $mimeTypes)) { | |||
| return $format; | |||
| } | |||
| if (isset($canonicalMimeType) && in_array($canonicalMimeType, (array) $mimeTypes)) { | |||
There was a problem hiding this comment.
I would probably rather initialise $canonicalMimeType with null and omit the isset() check.
There was a problem hiding this comment.
But this would force to call in_array() which is slower in case there is no cannonical mime-type, the isset() here prevents this useless check (in_array(null, (array) $mimeTypes)).
Instead of isset() when the variable is initialized with null, could be strict check:
if (null !== $canonicalMimeType && in_array($canonicalMimeType, (array) $mimeTypes)) {}There was a problem hiding this comment.
yes, that check should be added then instead of isset()
|
👍 Status: Reviewed |
|
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 | MIT When 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.