willdurand / Negotiation

Content Negotiation tools for PHP.
https://williamdurand.fr/Negotiation/
MIT License
1.41k stars 62 forks source link

getBest() not working correctly? #66

Closed milesj closed 4 years ago

milesj commented 9 years ago

So I have this code based on the JSON API spec.

$negotiator = new Negotiator();
$priority = 'application/vnd.api+json';

if ($contentTypeHeader = $request->headers->get('Content-Type')) {
    $contentType = $negotiator->getBest($contentTypeHeader, [$priority]);

    if ($contentType && $contentType->getValue() === $priority && count($contentType->getParameters())) {
        throw new ApiException(415, ErrorCode::UNSUPPORTED_MEDIA_TYPE);
    }
}

Which checks the Content-Type header in the request and throws and error if any parameters exist.

When testing, I set Content-Type to application/vnd.api+json;version=1, text/html, which seems legitimate, but it never matches as the "best". The $contentType variable is always null.

When trying to debug, it seems like AbstractNegotiator::match() is never called, but findMatches() is called. Not sure where the disconnect is, or if I'm doing this incorrectly.

akomm commented 9 years ago

Having header "Accept: a" is enough to match positive against priority "application/json" I see no reason, why this should result in a positive. Maybe its acceptable if it would result in a positive if you have the proper generic type like application - even here I'm not sure - but sure not "a".

milesj commented 9 years ago

Maybe I'm not understanding how this matching system works, but how would Accept: a match application/json, but not what I've done? Or am I misunderstanding what you said.

neural-wetware commented 8 years ago

Are you getting mixed up between Content-Type and Accept headers?

application/vnd.api+json; version=1 will not match application/vnd.api+json because of the version parameter. With the parameter, the acceptable media type is too specific and the server cannot meet the requirement. I believe this conforms to the RFC.

nickshanks commented 8 years ago

@neural-wetware That depends which way round the matching is done. If the client requests only foo/bar and the server has foo/bar;baz then that's a match. If the client requests foo/bar;baz and the server only knows it's serving foo/bar then it's not a match.

mkorkmaz commented 5 years ago

Quick fix is unsetting version parameter after these lines in BaseAccept class.

https://github.com/willdurand/Negotiation/blob/d8c86d51f01de65c1ca41dfbb842105d42906229/src/Negotiation/BaseAccept.php#L39-L42

if (isset($parameters['version'])) {
    unset($parameters['version']);
}

Since using version phrase in Accept header becomes common, it would be great to add version parameter as a property of BaseAccept class.

I came across this problem when I wanted use zend-problem-details library in a Zend Expressive application, so I want to ping @weierophinney to inform about this situation.

weierophinney commented 4 years ago

@mkorkmaz When versioning in Apigility, we allow either path-based versioning (e.g., /api/v2/such-and-such), or as part of the media-type name itself (e.g., application/vnd.such-and-such.v2+json) - never via a parameter. We'll do the same in Expressive, most likely.