[POC] Content negotiation implementation. #5711

jfsimon · 2012-10-09T09:43:58Z

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:

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]

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)
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 · 2012-10-11T19:57:34Z

src/Symfony/Component/HttpFoundation/Request.php

    }

    /**
-     * Returns true if the request is a XMLHttpRequest.
+     * Gets a list of encodings acceptable by the client browser.


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.

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

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

yep, thanks!

jfsimon · 2012-10-12T16:32:48Z

@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 · 2012-10-12T17:35:04Z

src/Symfony/Component/Routing/RequestAcceptance.php

+     *
+     * @return array
+     */
+    public function mapValues(\Closure $closure)


you should allow any callable, not only closures

stof · 2012-10-12T17:37:01Z

@jfsimon does it work when using the cached router ?

jfsimon · 2012-10-12T18:55:28Z

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

lsmith77 · 2012-10-12T21:52:57Z

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.

jfsimon · 2012-10-13T05:42:40Z

@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 · 2012-10-13T06:05:30Z

src/Symfony/Component/HttpFoundation/AcceptHeader.php

+     *
+     * @return array Array indexed by the values of the Accept-* header in preferred order
+     */
+    static public function split($header)


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

stof · 2012-10-13T14:44:41Z

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

lsmith77 · 2012-10-13T14:47:35Z

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

Crell · 2012-10-13T17:31:39Z

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.

Crell · 2012-10-13T17:35:46Z

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.

lsmith77 · 2012-10-13T17:37:31Z

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

jfsimon · 2012-10-13T19:29:32Z

@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

Crell · 2012-10-13T19:39:18Z

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

lsmith77 · 2012-10-13T19:39:54Z

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

jfsimon · 2012-10-17T22:04:10Z

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

stof · 2012-10-17T22:09:38Z

src/Symfony/Component/HttpFoundation/AcceptHeader.php

+ *
+ * @author Jean-François Simon <jeanfrancois.simon@sensiolabs.com>
+ */
+class AcceptHeader


I would renamed it to AcceptHeaderParser

I would agree.

lanthaler · 2012-10-22T20:51:12Z

src/Symfony/Component/HttpFoundation/AcceptHeader.php

+                $bits = explode('=', $property);
+                $this->values[$name][trim($bits[0])] = isset($bits[1]) ? trim($bits[1]) : null;
+            }
+        }


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.

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

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

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

@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 · 2012-10-22T22:04:25Z

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

jfsimon · 2012-10-22T22:15:07Z

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

scor · 2012-10-23T15:52:42Z

@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

Tobion · 2012-10-23T20:33:56Z

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

jfsimon · 2012-10-24T05:30:08Z

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

fabpot · 2012-11-05T11:29:05Z

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

pdrakeweb · 2012-11-09T16:43:15Z

@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 · 2012-12-04T14:54:21Z

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

jfsimon · 2012-12-04T15:32:12Z

@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).

lsmith77 · 2013-02-23T13:24:07Z

havent studied the code in detail yet but it seems like zf2 got a decently sophisticated implementation:
https://github.com/zendframework/zf2/blob/master/library/Zend/Http/Header/AbstractAccept.php
https://github.com/zendframework/zf2/blob/master/tests/ZendTest/Http/Header/AcceptTest.php

jfsimon · 2013-02-23T15:15:44Z

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

kor3k · 2013-03-18T11:31:47Z

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:

the _locale request / query attribute -
- this also stores the selected locale to session
session
accept-language header
default value

but with other formats (xml, json):

the _locale request / query attribute -
- this also stores the selected locale to session
accept-language header
session
default value

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

stof · 2013-03-19T21:09:11Z

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.

lunetics · 2013-04-24T12:15:24Z

@kor3k maybe you should look into https://github.com/lunetics/LocaleBundle

@fabpot

This PR was squashed before being merged into the master branch (closes symfony#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.

fabpot · 2014-03-26T16:15:22Z

Closing (#10538)

jfsimon added 8 commits October 9, 2012 11:22

[HttpFoundation] Added content negociation headers support to request.

efcbab1

[HttpFoundation] Added tests.

5fb2601

[routing] Added acceptances to request context.

020914b

[http-foundation] Moved accept headers parsing to dedicated class.

8c02e8a

[routing] Added negotiation to UrlMatcher.

532720a

[http-foundation] Fixed error.

b01c9d5

[http-foundation] Repaired broken tests.

88c5761

[routing] Fixed content negotiation basic implementation.

aeaeb02

scor reviewed Oct 11, 2012
View reviewed changes

jfsimon added 6 commits October 12, 2012 06:35

[http-kernel] Added 406 http exception handler.

cf6ff20

[routing] Added '_negotiation' parameter.

94e2df2

[http-foundation] Fixed docblock.

215dcbe

Cleaned code.

f7651ef

[http-kernel] Added content negotiation listener.

c98fb8f

[framework-bundle] Registered content negotiation listener as service.

88e1d39

stof reviewed Oct 12, 2012
View reviewed changes

ghost reviewed Oct 13, 2012
View reviewed changes

Fixed CS.

4071d7e

stof reviewed Oct 17, 2012
View reviewed changes

lsmith77 mentioned this pull request Oct 22, 2012

refactored _locale handling symfony-cmf/Routing#27

Merged

lanthaler reviewed Oct 22, 2012
View reviewed changes

lsmith77 mentioned this pull request Oct 24, 2012

[WIP] added support for a API versioning FriendsOfSymfony/FOSRestBundle#136

Closed

jfsimon mentioned this pull request Oct 26, 2012

[http-foudation] Better accept header parsing #5841

Closed

celmaun mentioned this pull request Dec 15, 2012

[HttpFoundation] Default request _format should be aware of HTTP Accept header #6276

Closed

lsmith77 mentioned this pull request Dec 19, 2012

implement mod_negotiation algorithm FriendsOfSymfony/FOSRest#1

Closed

lsmith77 mentioned this pull request Jan 11, 2013

Using FOSRest for part of an application with content negotiation FriendsOfSymfony/FOSRestBundle#335

Closed

stof mentioned this pull request Feb 15, 2013

XMLHttpRequest doesn't set the _format option in the request #7089

Closed

fabpot mentioned this pull request Mar 26, 2014

Content negotiation #10538

Closed

fabpot closed this Mar 26, 2014

Uh oh!

[POC] Content negotiation implementation. #5711

[POC] Content negotiation implementation. #5711

Uh oh!

Conversation

jfsimon commented Oct 9, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfsimon commented Oct 12, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Oct 12, 2012

Uh oh!

jfsimon commented Oct 12, 2012

Uh oh!

lsmith77 commented Oct 12, 2012

Uh oh!

jfsimon commented Oct 13, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Oct 13, 2012

Uh oh!

lsmith77 commented Oct 13, 2012

Uh oh!

Crell commented Oct 13, 2012

Uh oh!

Crell commented Oct 13, 2012

Uh oh!

lsmith77 commented Oct 13, 2012

Uh oh!

jfsimon commented Oct 13, 2012

Uh oh!

Crell commented Oct 13, 2012

Uh oh!

lsmith77 commented Oct 13, 2012

Uh oh!

jfsimon commented Oct 17, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scor commented Oct 22, 2012

Uh oh!

jfsimon commented Oct 22, 2012

Uh oh!

scor commented Oct 23, 2012

Uh oh!

Tobion commented Oct 23, 2012

Uh oh!

jfsimon commented Oct 24, 2012

Uh oh!

fabpot commented Nov 5, 2012

Uh oh!

pdrakeweb commented Nov 9, 2012

Uh oh!

scor commented Dec 4, 2012

Uh oh!

jfsimon commented Dec 4, 2012