[http-foudation] Better accept header parsing #5841
Conversation
|
As dicussed in #5711. |
| public function __construct(array $items) | ||
| { | ||
| $this->sorted = true; | ||
| $this->items = array(); |
There was a problem hiding this comment.
you should move these to the property declarations
|
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. |
|
Sorry please check Seldaek@376dd93 instead, I missed a few tests in the RequestTest class. |
…ry AcceptHeaderItem
|
@fabpot do you think the introduced BC break is acceptable? |
|
@jfsimon Are all getAccept*() method BC? |
|
@fabpot nope, just |
|
@fabpot I think missunderstood... only |
|
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. |
|
@fabpot Okay, I'll update this PR according to your second option. |
|
@fabpot done. As you can see here: https://github.com/symfony/symfony/pull/5841/files#L5L1029 value returned by |
| @@ -14,6 +14,8 @@ | |||
| use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage; | |||
| use Symfony\Component\HttpFoundation\Session\Session; | |||
| use Symfony\Component\HttpFoundation\Request; | |||
| use Symfony\Component\HttpFoundation\AcceptHeader; | |||
| use Symfony\Component\HttpFoundation\AcceptHeaderItem; | |||
| */ | ||
| public function first() | ||
| { | ||
| $this->ensureSorted(); |
|
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). |
|
@fabpot I could add this example here: http://symfony.com/doc/current/components/http_foundation/introduction.html#request after |
|
Yes, looks good to me. |
| /** | ||
| * Constructor. | ||
| * | ||
| * @param array $items |
There was a problem hiding this comment.
should also be AcceptHeaderItem[]
| return $qA > $qB ? -1 : 1; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Shouldn't it set $this->sorted = true; after sorting?!
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Quality:
The special
qitem attribute represents its quality. I had to make some choices:qattribute, it's assigned to quality property, but not to attributes__toString()method only renderqattribute if quality is less than 1BC 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 ofAcceptHeaderItemindexed by its value.