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

Already on GitHub? Sign in to your account

[POC] Content negotiation implementation. #5711

Closed
wants to merge 33 commits into
from

Conversation

Projects
None yet
Contributor

jfsimon commented Oct 9, 2012

This PR attempts to implement server content negotiation features to Symfony2 components, as described here: http://httpd.apache.org/docs/2.2/en/content-negotiation.html.

How should it work:

  1. Let says I wish to let the framework negotiate the format & locale, I would write my routes like this:

    my_route:
        pattern: /my/route.{_locale}.{_format}
        requirements:
            _locale: en|es|fr
            _format: html|json|xml
        options:
            negotiate: [_locale, _format]
    
  2. I need to have the following headers in my request:

    • Accept which gives the accepted content types (_format)
    • Accept-Language which gives the accepted languages (_locale)
  3. I want to get the best resource variant when requesting /my/route URL.

The following rules should do the trick:

  • If none of a required parameter (ie. en, es or fr for _locale) is present the corresponding Accept header (ie. Accepted-Language for _locale), a 406 exception is thrown.
  • Else the value with the highest quality is selected and passed to route parameter.

Here is an example: https://gist.github.com/3872155

@scor scor and 1 other commented on an outdated diff Oct 11, 2012

src/Symfony/Component/HttpFoundation/Request.php
}
/**
- * Returns true if the request is a XMLHttpRequest.
+ * Gets a list of encodings acceptable by the client browser.
@scor

scor Oct 11, 2012

The client might not be a browser, but could be any machine requesting data from the server (search engine, another server, etc.). best to stick with a generic "client" terminology alone IMO.

@jfsimon

jfsimon Oct 11, 2012

Contributor

You're right that was a concrete example, not a generic one.
client browser should be user agent.

@scor

scor Oct 12, 2012

you missed another "client browser" in src/Symfony/Component/HttpFoundation/Request.php (there were two of them)

@jfsimon

jfsimon Oct 12, 2012

Contributor

yep, thanks!

Contributor

jfsimon commented Oct 12, 2012

@scor @fabpot I thing the POC is ready.
It's quite simple but not really following standard algorithme.
Actually, we dont choose the best resource variant, but the best value for each negotiated variable.
I'd like to work on another more complex POC which allows various strategies to handle negotiation.
What do you think?

@stof stof commented on an outdated diff Oct 12, 2012

src/Symfony/Component/Routing/RequestAcceptance.php
+ *
+ * @return array
+ */
+ public function getValues()
+ {
+ return array_keys($this->qualities);
+ }
+
+ /**
+ * Applies a closure on values.
+ *
+ * @param \Closure $closure
+ *
+ * @return array
+ */
+ public function mapValues(\Closure $closure)
@stof

stof Oct 12, 2012

Member

you should allow any callable, not only closures

Member

stof commented Oct 12, 2012

@jfsimon does it work when using the cached router ?

Contributor

jfsimon commented Oct 12, 2012

@stof you mean the PhpMatcherDumper? not yet, it looks tricky!

Contributor

lsmith77 commented Oct 12, 2012

well there isn't a "standard algorithm" afaik .. since the actual negotiation isnt covered in the spec. so i think its good indeed to make it possible to plugin different algorithms both the change the behavior but also to allow performance considerations.

Contributor

jfsimon commented Oct 13, 2012

@lsmith77 I was thinking to http://httpd.apache.org/docs/2.2/en/content-negotiation.html#methods when talking about a "standard algorithm". I also keep http://pretty-rfc.herokuapp.com/RFC2295 in mind.

In order to allow something more complex, I think I need to create some classes (Variant, NegotiationStrategyInterface, QualifierInterface etc...). Where do you think this code should stand? In HttpKernel?

I also wonder how to manage the RFC2295's features. It seems interesting.

@ghost Unknown and 1 other commented on an outdated diff Oct 13, 2012

src/Symfony/Component/HttpFoundation/AcceptHeader.php
+
+/**
+ * Accept-* HTTP headers utils.
+ *
+ * @author Jean-François Simon <jeanfrancois.simon@sensiolabs.com>
+ */
+class AcceptHeader
+{
+ /**
+ * Splits an Accept-* HTTP header.
+ *
+ * @param string $header Header to split
+ *
+ * @return array Array indexed by the values of the Accept-* header in preferred order
+ */
+ static public function split($header)
@ghost

ghost Oct 13, 2012

I think it's public static function now following PSR-2

@jfsimon

jfsimon Oct 13, 2012

Contributor

fixed

Member

stof commented Oct 13, 2012

@jfsimon The discussion on the ticket requesting the feature was talking about creating a new component for the content negotiation

Contributor

lsmith77 commented Oct 13, 2012

@stof i dont think it necessarily needs to be a new component. but if an existing component is used, why not HttpFoundation?

Contributor

Crell commented Oct 13, 2012

I would encourage making it a stand-alone component for easier reuse. There's no need to bind the negotiation algorithm to HttpFoundation's Request implementation.

Contributor

Crell commented Oct 13, 2012

I'm not quite clear from the code, but the description above says that one would specify a route format like this:

_format: html|json|xml

That is not sufficient. That's just the serialization format. application/atom+xml and application/svg+xml are very different formats, but both "xml parsing". Similarly, application/json does not imply JSON-LD, which has its own mime type, I believe.

Contributor

lsmith77 commented Oct 13, 2012

@Crell: I think it makes sense to offer a simple algorithm just based on the "mapped" formats. But I agree there also needs to be a way to do negotiation based on actual mime types.

Contributor

jfsimon commented Oct 13, 2012

@lsmith77 @Crell do you think that RFC2295 should be fully implemented? There's a lot of interesting features, but also many things involved. Would you like to discuss about it on IRC?

I think the code could take place in HttpFoundation and HttpKernel.
Basically, content negotiation involves to:

  • read headers from the request
  • mutate the routing (each variant must be accessible via a GET)
  • decide which variant is the best
  • build a response with it + headers Vary, TCN, Alternatives
Contributor

Crell commented Oct 13, 2012

Yes, let's talk in #symfony-dev. I'm available now.

Contributor

lsmith77 commented Oct 13, 2012

@jfsimon who has actually implemented RFC 2295? I am not an expert in the entire world of RFCs, but from all I know that RFC requires client support.

Owner

fabpot commented Oct 13, 2012

Keep in mind that this is a POC. So, no support for the PHP dumper for now, and the same goes for the standalone component discussion. But the POC can probably already tell us that content negociation is tied to both the request and the routing system, so a standalone component is probably not possible.

Contributor

lsmith77 commented Oct 13, 2012

yeah .. indeed we should postpone the discussion about where the code will end up util we have a working implementation doing what we want. at that point we can make a much easier decision on where the code should go.

Contributor

jfsimon commented Oct 13, 2012

@lsmith77 your're right for the RFC, but it gives a good vision of the negotiation process. I'll try to keep things simple.

@stof stof and 2 others commented on an outdated diff Oct 13, 2012

src/Symfony/Component/Routing/Generator/UrlGenerator.php
@@ -120,6 +120,10 @@ public function generate($name, $parameters = array(), $absolute = false)
throw new RouteNotFoundException(sprintf('Route "%s" does not exist.', $name));
}
+ foreach ($this->context->getOptionsHandlers(array_keys($route->getOptions())) as $handler) {
@stof

stof Oct 13, 2012

Member

@jfsimon should the handlers really be registered on the RequestContext ? It seems wrong to me as they are able to change the way the route are compiled, which means that the cached router would need to depend of what is set in the RequestContext you set into it.
IMO, you should register the handlers in the matcher and generator themselves.

@Crell

Crell Oct 13, 2012

Contributor

Note that Drupal cannot use RequestContext because it doesn't have enough data, so putting the useful information on RequestContext would not be useful for us.

@jfsimon

jfsimon Oct 13, 2012

Contributor

@stof right. I'll move this in the matcher / generator.

@jfsimon

jfsimon Oct 13, 2012

Contributor

done

@stof stof and 1 other commented on an outdated diff Oct 13, 2012

src/Symfony/Component/Routing/RequestAcceptance.php
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing;
+
+use Symfony\Component\HttpFoundation\AcceptHeader;
+
+/**
+ * Holds information about an Accept-* header of hte current request.
+ *
+ * @author Jean-François Simon <jeanfrancois.simon@sensiolabs.com>
+ */
+class RequestAcceptance
@stof

stof Oct 13, 2012

Member

This class seems unused

@jfsimon

jfsimon Oct 13, 2012

Contributor

Yep!

@jfsimon

jfsimon Oct 13, 2012

Contributor

removed

@stof stof and 1 other commented on an outdated diff Oct 13, 2012

...y/Component/HttpFoundation/Negotiation/Negotiator.php
+ private $frozen;
+
+ /**
+ * @var bool
+ */
+ private $sorted;
+
+ /**
+ * Constructor.
+ */
+ public function __construct()
+ {
+ $this->qualifiers = array();
+ $this->contents = array();
+ $this->frozen = false;
+ $this->sorted = true;
@stof

stof Oct 13, 2012

Member

you should set the default values on the property declarations instead

@jfsimon

jfsimon Oct 13, 2012

Contributor

done

@stof stof and 1 other commented on an outdated diff Oct 13, 2012

...onent/HttpFoundation/Negotiation/ContentInterface.php
+
+namespace Symfony\Component\HttpFoundation\Negotiation;
+
+/**
+ * Represents a negotiable content.
+ *
+ * @author Jean-François Simon <contact@jfsimon.fr>
+ */
+interface ContentInterface
+{
+ /**
+ * Returns content type.
+ *
+ * @return string
+ */
+ function getType();
@stof

stof Oct 13, 2012

Member

missing public keyword on all methods

@jfsimon

jfsimon Oct 13, 2012

Contributor

should we really add visibility in interfaces? everything is public here

@stof

stof Oct 14, 2012

Member

this was the only point where the Sf2 CS were not matching the PSR-2 rules so @fabpot switched the Sf2 rule a few months ago.

@stof stof and 1 other commented on an outdated diff Oct 13, 2012

...onent/HttpFoundation/Negotiation/ContentInterface.php
+ */
+ function getLanguage();
+
+ /**
+ * Returns content charset.
+ *
+ * @return string
+ */
+ function getCharset();
+
+ /**
+ * Set the content quality.
+ *
+ * @param $quality
+ *
+ * @return mixed
@stof

stof Oct 13, 2012

Member

mixed ? Isn't it a float ?

@jfsimon

jfsimon Oct 13, 2012

Contributor

changed

@stof stof and 1 other commented on an outdated diff Oct 13, 2012

...ent/HttpFoundation/Negotiation/QualifierInterface.php
+ */
+interface QualifierInterface
+{
+ /**
+ * Qualifies a content.
+ *
+ * @param ContentInterface $content
+ *
+ * @return float A quality, between 0 and 1
+ */
+ function qualify(ContentInterface $content);
+
+ /**
+ * Returns varying headers.
+ *
+ * @return mixed
@stof

stof Oct 13, 2012

Member

this must be an array, otherwise you get an error when using it in array_merge

@jfsimon

jfsimon Oct 13, 2012

Contributor

changed

@stof stof and 1 other commented on an outdated diff Oct 13, 2012

src/Symfony/Component/HttpFoundation/Request.php
*/
- public function splitHttpAcceptHeader($header)
@stof

stof Oct 13, 2012

Member

you need to keep the splitHttpAcceptHeader method for BC

@jfsimon

jfsimon Oct 13, 2012

Contributor

thanks :)

@jfsimon

jfsimon Oct 13, 2012

Contributor

done

@stof stof and 1 other commented on an outdated diff Oct 13, 2012

...tpKernel/EventListener/ContentNegotiationListener.php
+{
+ public function onKernelRequest(GetResponseEvent $event)
+ {
+ $attributes = $event->getRequest()->attributes;
+
+ if ($attributes->has('_negotiation')) {
+ $vary = implode(', ', $attributes->get('_negotiation'));
+ $headers = $event->getResponse()->headers;
+
+ $headers->set('Vary', $headers->has('Vary') ? $headers->get('Vary').', '.$vary : $vary);
+ }
+ }
+
+ public static function getSubscribedEvents()
+ {
+ return array(KernelEvents::REQUEST => 'onKernelRequest');
@stof

stof Oct 13, 2012

Member

onKernelRequest to set a response header ? this is wrong. It has to be done on kernel.response to be able to access the response.

@jfsimon

jfsimon Oct 13, 2012

Contributor

ooops

@jfsimon

jfsimon Oct 14, 2012

Contributor

changed

Contributor

jfsimon commented Oct 14, 2012

I re-implemented the first basic negotiation system in a better way.
I'm ready to implement more complexe possibility.
@lsmith77 @Crell could you explain me what do you need.

@stof stof and 1 other commented on an outdated diff Oct 14, 2012

src/Symfony/Component/HttpFoundation/Request.php
- return $values;
+ /**
+ * Returns true if the request is a XMLHttpRequest.
+ *
+ * It works if your JavaScript library set an X-Requested-With HTTP header.
+ * It is known to work with Prototype, Mootools, jQuery.
+ *
+ * @return Boolean true if the request is an XMLHttpRequest, false otherwise
+ *
+ * @api
+ */
+ public function isXmlHttpRequest()
+ {
+ return 'XMLHttpRequest' == $this->headers->get('X-Requested-With');
@stof

stof Oct 14, 2012

Member

you should keep this method before splitHttpAcceptHeader to avoid unnecessary diff in the PR. This will avoid breaking the git history for this method once the commits are squashed.

@jfsimon

jfsimon Oct 14, 2012

Contributor

okay, moved

@stof stof and 1 other commented on an outdated diff Oct 14, 2012

...tpKernel/EventListener/ContentNegotiationListener.php
+/**
+ * Set response header for content negotiation.
+ *
+ * @author Jean-François Simon <jeanfrancois.simon@sensiolabs.com>
+ */
+class ContentNegotiationListener implements EventSubscriberInterface
+{
+ /**
+ * @var Router
+ */
+ private $router;
+
+ /**
+ * @param Router $router
+ */
+ public function __construct(Router $router)
@stof

stof Oct 14, 2012

Member

you should not typehint the class IMO as it means it is broken when someone replaces the router by composition (BeSimpleI18nRoutingBundle does it)
Thus, it makes it impossible to use this listener in Silex (which has a matcher and a generator but no router)

@jfsimon

jfsimon Oct 14, 2012

Contributor

This listener has been removed.

@stof stof and 1 other commented on an outdated diff Oct 14, 2012

...tpKernel/EventListener/ContentNegotiationListener.php
+ private $router;
+
+ /**
+ * @param Router $router
+ */
+ public function __construct(Router $router)
+ {
+ $this->router = $router;
+ }
+
+ /**
+ * @param GetResponseEvent $event
+ */
+ public function onKernelRequest(GetResponseEvent $event)
+ {
+ $this->router->addRouteHandler(new NegotiatedRouteHandler($event->getRequest()->headers));
@stof

stof Oct 14, 2012

Member

if you are setting handlers according to the request, and that these handlers can change the compilation of routes, it breaks things:

  • the compiled routes cannot be cached anymore (you need to have a cache for each possible set of headers)
  • the router needs to be moved to the request scope (as you may have different headers for a subrequest), which would break everything
@stof

stof Oct 14, 2012

Member

and btw, you are setting them in the router after it is used, due to the priority of the listeners.
And the initialization of the router should go in the RouterListener

@jfsimon

jfsimon Oct 14, 2012

Contributor

I finally removed all the RouteHandler crap. I don't like it. I'll find a better way.

@stof stof and 1 other commented on an outdated diff Oct 14, 2012

...mfony/Component/HttpKernel/NegotiatedRouteHandler.php
@@ -0,0 +1,123 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel;
@stof

stof Oct 14, 2012

Member

This class should go in a subnamespace IMO. It is not one of the main classes of the component (Kernel and HttpKernel are at the top level)

@jfsimon

jfsimon Oct 14, 2012

Contributor

moved

@stof stof and 1 other commented on an outdated diff Oct 14, 2012

...mfony/Component/HttpKernel/NegotiatedRouteHandler.php
+ $route->setDefault($variable, current($values));
+ } else {
+ $message = sprintf('None of the accepted values "%s" match route requirement "%s".', implode(', ', $values), $requirement);
+
+ throw new NotAcceptableException($variables, $message);
+ }
+ }
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function updateMatchedParameters(Route $route, array $parameters)
+ {
+ $parameters['_negotiated_headers'] = $this->headers;;
@stof

stof Oct 14, 2012

Member

duplicated semicolon

@jfsimon

jfsimon Oct 14, 2012

Contributor

thx

@stof stof and 1 other commented on an outdated diff Oct 14, 2012

...Component/Routing/Generator/UrlGeneratorInterface.php
@@ -21,7 +22,7 @@
*
* @api
*/
-interface UrlGeneratorInterface extends RequestContextAwareInterface
+interface UrlGeneratorInterface extends RequestContextAwareInterface, RouteHandlerAwareInterface
@stof

stof Oct 14, 2012

Member

I don't think it should extend the new interface. This would be a BC break as it would force to implement the new interface on all existing generators (thus breaking things if someone implements the interface on its own). Simply add the new interface and implement them in the core implementations, and do an instanceof check in the RouterListener.

@jfsimon

jfsimon Oct 14, 2012

Contributor

okay

@stof stof and 1 other commented on an outdated diff Oct 14, 2012

src/Symfony/Component/Routing/Router.php
@@ -32,6 +32,7 @@ class Router implements RouterInterface
protected $resource;
protected $options;
protected $logger;
+ protected $routeHandlers;
@stof

stof Oct 14, 2012

Member

you don't need this variable as you don't use it

@stof

stof Oct 14, 2012

Member

hmm, actually, depending of the way you do it, it could make sense to keep this variable, and to add them in the matcher and the generator lazily: addRouteHandler would add the element in the array, and proxy the call only if the matcher and the generator are already initialized. Otherwise, it would be set only when initializing them.

@jfsimon

jfsimon Oct 14, 2012

Contributor

good point

Contributor

jfsimon commented Oct 14, 2012

I removed changes to the routing and added a simple negotiation process. To negotiate parameters for a route, you must add an option negotiate which contains the list of negotiated parameters.

my_route:
    pattern: /my/route.{_locale}.{_format}
    options:
        negotiate: [_locale, _format] // all negotiated parameters should obviously be present in the pattern
    requirements:
        _locale: en|de|fr // requirements are still optional

This avoid the previously introduced cache problem, a route is always negotiated for the same parameters regardless to the negotiation process. If a parameter cant be negotiated properly, a NegotiationFailureException is thrown and turned into a NotAcceptableHttpException by the RouteListener.

Contributor

jfsimon commented Oct 14, 2012

I re-implemented (again) the basic content negotiator upon new routing feature.
I'm waiting for comments to write more code.
@lsmith77 @Crell @stof please send feedback!

Contributor

jfsimon commented Oct 14, 2012

One more thing: in the basic negotiation implemented above, we assume that parameters are totaly independant from each others.

It may not be true, a good example is the language and the charset. In this case, we have to use a different process:

  • build every content variants
  • negotiate headers against these contents

This is why I added Symfony\Component\HttpFoudation\Negotiation classes.

Maybe it would be great to be able to mix these 2 content negotiation processes.

@stof stof and 1 other commented on an outdated diff Oct 14, 2012

...nel/Negotiation/MatcherNegotiatorBuilderInterface.php
+
+use Symfony\Component\Routing\Matcher\Negotiator;
+
+/**
+ * MatcherNegotiatorBuilderInterface.
+ *
+ * @author Jean-François Simon <contact@jfsimon.fr>
+ */
+interface MatcherNegotiatorBuilderInterface
+{
+ /**
+ * Builds the negotiator.
+ *
+ * @param Negotiator $negotiator
+ */
+ public function buildNegotiator(Negotiator $negotiator);
@stof

stof Oct 14, 2012

Member

The typehint should be an interface

@jfsimon

jfsimon Oct 14, 2012

Contributor

done

@stof stof and 1 other commented on an outdated diff Oct 14, 2012

...ponent/Routing/Matcher/NegotiatorMatcherInterface.php
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Matcher;
+
+use Symfony\Component\Routing\RequestContextAwareInterface;
+use Symfony\Component\Routing\Exception\ResourceNotFoundException;
+use Symfony\Component\Routing\Exception\MethodNotAllowedException;
+
+/**
+ * NegotiatorMatcherInterface.
+ *
+ * @author Jean-François Simon <contact@jfsimon.fr>
+ */
+interface NegotiatorMatcherInterface extends RequestContextAwareInterface
@stof

stof Oct 14, 2012

Member

does it really need to extend RequestContextAwareInterface ? I think they should be able to be implemented separately

@jfsimon

jfsimon Oct 14, 2012

Contributor

indeed, I think it's a copy/paste issue :-/

@jfsimon

jfsimon Oct 14, 2012

Contributor

fixed

@stof stof commented on the diff Oct 14, 2012

src/Symfony/Component/Routing/Matcher/UrlMatcher.php
{
const REQUIREMENT_MATCH = 0;
const REQUIREMENT_MISMATCH = 1;
const ROUTE_MATCH = 2;
+ /**
+ * @var RequestContext
+ */
@stof

stof Oct 14, 2012

Member

we don't use @var on non-public properties

@jfsimon

jfsimon Oct 14, 2012

Contributor

oO are you sure? properties are almost never public, there are dozens of @var to remove!

@Crell

Crell Oct 14, 2012

Contributor

I'm not sure what the official Symfony standard is, but I always treat any object property that is lacking a @var as a bug. So does Drupal's coding standards. I would keep them. Never assume someone reading your code has any clue what you were thinking. :-)

@stof

stof Oct 14, 2012

Member

@Crell when the typehing of the constructor argument 10 lines below shows it is a RequestContext, is it really an issue ? even IDEs are able to use the constructor now.

@Crell

Crell Oct 14, 2012

Contributor

Yes, I think it is. Going to track down the type of a variable is a distraction from whatever it was I was doing before, and now I'm reading unrelated assignment code. I've not seen my IDE figure it out from the constructor yet, so I don't know how common that is. Every issue that causes me to think about something other than the problem I'm trying to solve is one more distraction that interrupts my flow. Plus, it's easier to make a blanket statement of "document everything" than to sort out "do we really need it this time?"

@ghost

ghost Oct 17, 2012

@stof - typehinting properties is really important.

@stof stof and 1 other commented on an outdated diff Oct 14, 2012

src/Symfony/Component/Routing/Matcher/UrlMatcher.php
@@ -47,6 +58,7 @@ public function __construct(RouteCollection $routes, RequestContext $context)
{
$this->routes = $routes;
$this->context = $context;
+ $this->negotiator = new Negotiator();
@stof

stof Oct 14, 2012

Member

you shoould allow injecting it, to allow using a different implementation

@jfsimon

jfsimon Oct 14, 2012

Contributor

done

@stof stof and 1 other commented on an outdated diff Oct 14, 2012

...Component/HttpKernel/EventListener/RouterListener.php
}
}
+ public function onKernelResponse(GetResponseEvent $event)
+ {
+ $headers = $event->getResponse()->headers;
+ $vary = implode(', ', $this->getMatcherNegotiatorVaryingHeaders());
@stof

stof Oct 14, 2012

Member

this looks wrong for subrequests. Your nagotiator builders are tied to the request (at least the one added here) but the same negotiator is reused for subrequests.

If you want to be safe, the negotiator should be a stateless object, and the state should be kept elsewhere (probably stored in a request attribute as it is tied to the request)

@jfsimon

jfsimon Oct 14, 2012

Contributor

good pont, again!
what could I do without your code reviews? not sure I want to know...

@jfsimon

jfsimon Oct 14, 2012

Contributor

done

@pborreli pborreli commented on an outdated diff Oct 14, 2012

...fony/Component/HttpFoundation/Negotiation/Content.php
+ {
+ return $this->charset;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setQuality($quality)
+ {
+ $this->quality = $quality;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ function getQuality()
@pborreli

pborreli Oct 14, 2012

Contributor

need scope public

@pborreli pborreli and 1 other commented on an outdated diff Oct 14, 2012

...nt/HttpFoundation/Negotiation/NegotiatorInterface.php
+ *
+ * @author Jean-François Simon <contact@jfsimon.fr>
+ */
+interface NegotiatorInterface
+{
+ /**
+ * Adds a qualifier for the negotiation.
+ *
+ * @param QualifierInterface $qualifier
+ */
+ public function addQualifier(QualifierInterface $qualifier);
+
+ /**
+ * Adds a document to qualify.
+ *
+ * @param ContentInterface $variant
@pborreli

pborreli Oct 14, 2012

Contributor

this must be $content

@jfsimon

jfsimon Oct 17, 2012

Contributor

done

@pborreli pborreli and 1 other commented on an outdated diff Oct 14, 2012

...nt/HttpFoundation/Negotiation/NegotiatorInterface.php
+ */
+interface NegotiatorInterface
+{
+ /**
+ * Adds a qualifier for the negotiation.
+ *
+ * @param QualifierInterface $qualifier
+ */
+ public function addQualifier(QualifierInterface $qualifier);
+
+ /**
+ * Adds a document to qualify.
+ *
+ * @param ContentInterface $variant
+ */
+ public function addContent(ContentInterface $content);
@pborreli

pborreli Oct 14, 2012

Contributor

or this must be $variant

@jfsimon

jfsimon Oct 17, 2012

Contributor

it's $content :)

@scor scor and 1 other commented on an outdated diff Oct 16, 2012

src/Symfony/Component/HttpFoundation/AcceptHeader.php
+ return array();
+ }
+
+ $values = array();
+ $groups = array();
+ foreach (array_filter(explode(',', $header)) as $value) {
+ // Cut off any q-value that might come after a semi-colon
+ if (preg_match('/;\s*(q=.*$)/', $value, $match)) {
+ $q = substr(trim($match[1]), 2);
+ $value = trim(substr($value, 0, -strlen($match[0])));
+ } else {
+ $q = 1;
+ }
+
+ $groups[$q][] = $value;
+ }
@scor

scor Oct 16, 2012

Looking at the split() method and the test cases, it looks like non-q parameters are ignored, only q is extracted. To give you a concrete example from the JSON-LD in Drupal use case [1], it looks like we will need to support Accept headers such as

application/ld+json; profile="http://drupal.org/project/deploy/content-staging"

This feature might be out of scope for the first POC conneg, but thought you should be aware of it, as we'll likely need it in Drupal.

[1] http://drupal.org/node/1797210#comment-6609212

@jfsimon

jfsimon Oct 17, 2012

Contributor

Indeed. I dont think it's out of the scope of the current POC. I'll try to implement that without any BC break soon.
Another example I saw: text/html; level=1.

Contributor

jfsimon commented Oct 17, 2012

@Crell I improved the accept-* header parser. If you think I should add more feature, let me know!

@stof stof commented on the diff Oct 17, 2012

src/Symfony/Component/HttpFoundation/AcceptHeader.php
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpFoundation;
+
+/**
+ * Accept-* HTTP header parser.
+ *
+ * @author Jean-François Simon <jeanfrancois.simon@sensiolabs.com>
+ */
+class AcceptHeader
@stof

stof Oct 17, 2012

Member

I would renamed it to AcceptHeaderParser

@ghost

ghost Oct 30, 2012

I would agree.

@pborreli pborreli commented on the diff Oct 17, 2012

src/Symfony/Component/HttpFoundation/Request.php
@@ -1264,45 +1287,15 @@ public function isXmlHttpRequest()
}
/**
- * Splits an Accept-* HTTP header.
+ * Splitts an Accept-* header.
@pborreli

pborreli Oct 17, 2012

Contributor

typo (Splitts)

@lsmith77 lsmith77 referenced this pull request in symfony-cmf/routing Oct 22, 2012

Merged

refactored _locale handling #27

@lanthaler lanthaler commented on the diff Oct 22, 2012

src/Symfony/Component/HttpFoundation/AcceptHeader.php
+ * @param string $header
+ */
+ public function __construct($header)
+ {
+ $this->values = array();
+ foreach (explode(',', $header) as $value) {
+ $properties = explode(';', $value);
+ if (!$name = trim(array_shift($properties))) {
+ continue;
+ }
+ $this->values[$name] = array();
+ foreach ($properties as $property) {
+ $bits = explode('=', $property);
+ $this->values[$name][trim($bits[0])] = isset($bits[1]) ? trim($bits[1]) : null;
+ }
+ }
@lanthaler

lanthaler Oct 22, 2012

Contributor

If I'm not completely off, this parsing is too optimistic. It will break as soon as one of the characters used in explode() appears in the value of a parameter. Consider e.g. the following Accept header:

text/html;q=0.9,text/plain; charset=utf-8; param="this;should,not,matter"; footnotes=true

The current code interprets not and matter as parameters whereas they really should be part of the value of param.

I'm not sure if the adding complexity for these corner cases is worth it, but at least this limitations should be documented properly.

@schmittjoh

schmittjoh Oct 22, 2012

Contributor

It should be quite easy to implement something more sophisticated using preg_split.

@jfsimon

jfsimon Oct 24, 2012

Contributor

@lanthaler @schmittjoh you're absolutely right. I'll propose a PR soon with this only feature (the accept-* headers parsing) and I'll use preg_split which is awesome.

@ghost

ghost Oct 30, 2012

If it's of any use, we used a little trick to process the q part here

@stof

stof Oct 30, 2012

Member

@Drak your code suffers from exactly the same bug than the one described in the comment on which you replied. Look at #5841 for a working implementation

scor commented Oct 22, 2012

Is the example at https://gist.github.com/3872155 still the right way to test this? I get a fatal error:
Uncaught exception 'LogicException' with message 'Route "negotiate" option must be an array of negotiated parameters.' in src/Symfony/Component/Routing/Route.php:412

Contributor

jfsimon commented Oct 22, 2012

@scor I fixed your exemple above, the value of negotiate option changed recently from boolean to an array of negotiated parameters.

scor commented Oct 23, 2012

@jfsimon ok, new error now. If this example working ok for you? I get this: PHP Catchable fatal error: Argument 1 passed to Symfony\Component\Routing\Exception\NegotiationFailureException::__construct() must be an array, object given, called in src/Symfony/Component/Routing/Matcher/Negotiator.php on line 64 and defined in src/Symfony/Component/Routing/Exception/NegotiationFailureException.php on line 34

Member

Tobion commented Oct 23, 2012

@jfsimon you need to add the defaults for _locale and _format to the example route in the introduction. Otherwise /my/route would not even match this route.

Contributor

jfsimon commented Oct 24, 2012

@Tobion this should not be necessary, and it's not. Take a look at the UrlMatcher, these variables defaults are set to null before compilation, so they are optional.

@scor I didn't test it (I know it's bad), and I'm not really sure this approach is the best one. What do you think? I'll take a moment this afternoon to fix this.

Thanks guys for your feedback.

Owner

fabpot commented Nov 5, 2012

@jfsimon Can you rebase this PR now that #5841 has been merged?

@jfsimon Just in case you didn't see it, I made a PR against your content-negotiation branch to make the content negotiation functional and provide an example that works. It probably needs some cleanup (especially in light of recent merges), but it seemed to work at the time. @scor

scor commented Dec 4, 2012

turns out the maintainer of the mimeparse library (@ramsey) is also a contributor to Symfony. @ramsey, is there anything from your lib that could be contributed in this PR? Could you maybe review the code here and the sub-PR @pdrakeweb created at jfsimon#1 ? any kind of feedback to make progress here would be great! :) thanks

Contributor

jfsimon commented Dec 4, 2012

@pdrakeweb sorry for the late... I'll try to merge your PR after rebasing master. I'll surely face many conflicts :-/

I'm not sure this POC should be merged to Symfony, I think it's a well playground to test ideas, but real changes should be done in separate PR (as it was done for header parsing), @scor what do you think?

IMHO, the difficult part of this is the framework integration, (not the variant negotiation).

Contributor

jfsimon commented Feb 23, 2013

@lsmith77 Thanks for your links!
Weird CS but interesting implementation.
Their header parsing/modelisation is indeed more sophisticated than the one added in #5841.
I'll try to open a PR based upon it this week.

Contributor

kor3k commented Mar 18, 2013

speaking about locale and formats, you should also consider this:

most browsers send accept-language implicitly with every request and it's value equals OS's locale. but this is not always the visitor's desired locale (i have eng windows, but wanna see website in czech).
so, the visitor should have an option to switch the locale "once and for all" - i use session for this. then, the accept-header is less significant than session value.

but the situation changes with other formats like json or xml. these formats are usually not requested by a browser, but by a 3rd party app (eg curl) or a xhr. and these "requestors" do not send accept-language headers implicitly, but only if their user sets them explicitly. therefore in this case, accept-language is more significant than session value.

when requesting a html format, the priority for determining request's locale should be like this:

  1. the _locale request / query attribute -
    • this also stores the selected locale to session
  2. session
  3. accept-language header
  4. default value

but with other formats (xml, json):

  1. the _locale request / query attribute -
    • this also stores the selected locale to session
  2. accept-language header
  3. session
  4. default value

also would be nice if the usage of session for locale can be enabled/disabled via framework configuration

Member

stof commented Mar 19, 2013

The usage of session has been removed in 2.1. If you want to store the locale in the session, you should do it in your own listener.

@ondrejmirtes ondrejmirtes pushed a commit to ondrejmirtes/symfony that referenced this pull request Nov 25, 2013

@fabpot fabpot merged branch jfsimon/accept-header-parsing (PR #5841)
This PR was squashed before being merged into the master branch (closes #5841).

Commits
-------

6b601bd [http-foudation] Better accept header parsing

Discussion
----------

[http-foudation] Better accept header parsing

Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes

**Quality:**
The special `q` item attribute represents its quality. I had to make some choices:
*  if I set `q` attribute, it's assigned to quality property, but not to attributes
*  the `__toString()` method only render `q` attribute if quality is less than 1

**BC break:**
The return of `Request::splitHttpAcceptHeader()` has changed. It's result was an array of qualities indexed by an accept value, it now returns an array of `AcceptHeaderItem` indexed by its value.

---------------------------------------------------------------------------

by jfsimon at 2012-10-26T08:35:55Z

As dicussed in symfony#5711.

---------------------------------------------------------------------------

by Seldaek at 2012-10-27T10:35:49Z

Maybe you can pull Seldaek@5e8a526 into your branch (for some reason I can't send a PR to your repo, it doesn't show up in github's repo selector.. looks like they don't like projects with too many forks). It allows you to use usort() which hopefully is faster than your merge sort, though I did not bench it. I also added tests to confirm the functionality.

---------------------------------------------------------------------------

by Seldaek at 2012-10-27T10:40:27Z

Sorry please check Seldaek@376dd93 instead, I missed a few tests in the RequestTest class.

---------------------------------------------------------------------------

by jfsimon at 2012-10-29T16:26:03Z

@fabpot do you think the introduced BC break is acceptable?

---------------------------------------------------------------------------

by fabpot at 2012-10-29T16:37:06Z

@jfsimon Are all getAccept*() method BC?

---------------------------------------------------------------------------

by jfsimon at 2012-10-29T16:39:26Z

@fabpot nope, just `Request::splitHttpAcceptHeader()`

---------------------------------------------------------------------------

by jfsimon at 2012-10-29T16:43:18Z

@fabpot I think missunderstood... only `Request::splitHttpAcceptHeader()` breaks BC.

---------------------------------------------------------------------------

by fabpot at 2012-10-29T16:53:22Z

So, a BC break on just splitHttpAcceptHeader is possible... but should be documented properly. Another option would be to deprecate the current method (and keep it as is), and just use the new version everywhere. Sounds better as it won"t introduce any BC breaks.

---------------------------------------------------------------------------

by jfsimon at 2012-10-29T16:55:57Z

@fabpot Okay, I'll update this PR according to your second option.

---------------------------------------------------------------------------

by jfsimon at 2012-10-29T20:14:46Z

@fabpot done.

As you can see here: https://github.com/symfony/symfony/pull/5841/files#L5L1029 value returned by `Request::splitHttpAcceptHeader()` is not **exactly** the same as before because all attributes are present (not only those before the `q` one).

---------------------------------------------------------------------------

by fabpot at 2012-10-30T06:16:23Z

The last thing missing before I can merge is a PR to update the documentation (should probably be just a note somewhere with the example you have in the UPGRADE file).

---------------------------------------------------------------------------

by jfsimon at 2012-10-30T07:07:08Z

@fabpot I could add this example here: http://symfony.com/doc/current/components/http_foundation/introduction.html#request after `Accessing the session`, what do you think?

---------------------------------------------------------------------------

by fabpot at 2012-10-30T07:14:10Z

Yes, looks good to me.
53fad04

@fabpot fabpot referenced this pull request Mar 26, 2014

Closed

Content negotiation #10538

Owner

fabpot commented Mar 26, 2014

Closing (#10538)

@fabpot fabpot closed this Mar 26, 2014

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