Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpFoundation] Fix support of custom mime types with parameters #18255

Merged
merged 1 commit into from Mar 25, 2016

Conversation

Projects
None yet
6 participants
@GuilhemN
Copy link
Contributor

commented Mar 21, 2016

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:

$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.

@GuilhemN GuilhemN force-pushed the GuilhemN:MIMETYPE branch from 83629ab to b0a1702 Mar 21, 2016

@GuilhemN GuilhemN changed the title [Request] Fix support of custom mime types with parameters [HttpFoundation] Fix support of custom mime types with parameters Mar 21, 2016

if (in_array($mimeType, (array) $mimeTypes)) {
if (in_array($extendedMimeType, (array) $mimeTypes)) {
return $format;
} elseif (isset($mimeType) && in_array($mimeType, (array) $mimeTypes)) {

This comment has been minimized.

Copy link
@stloyd

stloyd Mar 22, 2016

Contributor

Should be just if not elseif as you call return in first if.

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Author Contributor

Good catch, thank you

*
* @return string|null The format (null if not found)
*/
public function getFormat($mimeType)
public function getFormat($extendedMimeType)

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Mar 23, 2016

Member

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.

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 23, 2016

Author Contributor

How would we call the $mimeType without parameters then?

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Mar 23, 2016

Member

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 ?

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 23, 2016

Author Contributor

👍 changed.

@GuilhemN GuilhemN force-pushed the GuilhemN:MIMETYPE branch 2 times, most recently from 70d2130 to e75d7f0 Mar 23, 2016

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 25, 2016

👍

@@ -1250,6 +1250,9 @@ public function getFormat($mimeType)
if (in_array($mimeType, (array) $mimeTypes)) {
return $format;
}
if (isset($canonicalMimeType) && in_array($canonicalMimeType, (array) $mimeTypes)) {

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 25, 2016

Member

I would probably rather initialise $canonicalMimeType with null and omit the isset() check.

This comment has been minimized.

Copy link
@fabpot

This comment has been minimized.

Copy link
@stloyd

stloyd Mar 25, 2016

Contributor

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)) {}

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 25, 2016

Member

yes, that check should be added then instead of isset()

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 25, 2016

Author Contributor

updated.

@GuilhemN GuilhemN force-pushed the GuilhemN:MIMETYPE branch from e75d7f0 to f7ad285 Mar 25, 2016

@xabbuh

This comment has been minimized.

Copy link
Member

commented Mar 25, 2016

👍

Status: Reviewed

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 25, 2016

Thank you @Ener-Getick.

@fabpot fabpot merged commit f7ad285 into symfony:2.3 Mar 25, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 25, 2016

bug #18255 [HttpFoundation] Fix support of custom mime types with par…
…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

@GuilhemN GuilhemN deleted the GuilhemN:MIMETYPE branch Mar 25, 2016

@fabpot fabpot referenced this pull request Apr 29, 2016

Merged

Release v2.3.40 #18669

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.