merged branch vicb/acceptheaderfix (PR symfony#6003)

fabpot · fabpot · commit 225e3e53d4d5 · 2012-11-14T17:48:15.000+01:00
This PR was merged into the master branch. Commits ------- f4b630d [HttpFoundation] fix symfony#6002 Discussion ---------- [HttpFoundation] fix symfony#6002 --------------------------------------------------------------------------- by fabpot at 2012-11-13T17:02:45Z Can you add a test? --------------------------------------------------------------------------- by Tobion at 2012-11-13T17:04:23Z hehe see symfony#6004 there is also a test --------------------------------------------------------------------------- by vicb at 2012-11-13T17:05:06Z There is a test in the original PR, no need for 6004. --------------------------------------------------------------------------- by vicb at 2012-11-13T17:05:20Z Which is why is was failing btw --------------------------------------------------------------------------- by Tobion at 2012-11-13T17:06:36Z The test in 6002 did not fail for me without your patch. --------------------------------------------------------------------------- by fabpot at 2012-11-14T12:47:46Z @Tobion @vicb What do we do? Just revert symfony#6002 or merge this PR? --------------------------------------------------------------------------- by vicb at 2012-11-14T13:25:51Z Merge. Go go go :) ----- Reply message ----- De : "Fabien Potencier" <notifications@github.com> Pour : "symfony/symfony" <symfony@noreply.github.com> Cc : "Victor Berchet" <victor@suumit.com> Objet : [symfony] [HttpFoundation] fix symfony#6002 (symfony#6003) Date : mer., nov. 14, 2012 13:47 @Tobion @vicb What do we do? Just revert symfony#6002 or merge this PR? &mdash; Reply to this email directly or view it on GitHub. --------------------------------------------------------------------------- by Tobion at 2012-11-14T13:31:22Z @vicb can you explain what it fixes? As I said, your test does not cover something that would fail without the patch. So I don't see the bug. --------------------------------------------------------------------------- by vicb at 2012-11-14T15:30:55Z @Tobion php.net states: The `current()` function simply returns the value of the array element that's currently being pointed to by the internal pointer and `reset()` returns the value of the first array element. I have no clue what the "element that's currently being pointed to by the internal pointer" in this method so `reset()` is probably what you want. Validated ? --------------------------------------------------------------------------- by Tobion at 2012-11-14T16:12:03Z I agree `reset()` is more explicit here. But `current()` should work just as well in this case because the array pointer can only be at the first item when calling the method. Anyway, this is good to merge. I just hoped there was a unit test that ensures this on my machine. This is why I added the test in my patch. Maybe Fabien can just merge the second commit on symfony#6004 --------------------------------------------------------------------------- by vicb at 2012-11-14T16:20:57Z As explained in symfony#6004, there is already a test from my first PR that made Travis go red.
diff --git a/src/Symfony/Component/HttpFoundation/AcceptHeader.php b/src/Symfony/Component/HttpFoundation/AcceptHeader.php
@@ -146,7 +146,7 @@ public function first()
     {
         $this->sort();
 
-        return !empty($this->items) ? $this->items[0] : null;
+        return !empty($this->items) ? reset($this->items) : null;
     }
 
     /**

Original file line number	Diff line number	Diff line change
`@@ -146,7 +146,7 @@ public function first()`
`146`	`146`	`{`
`147`	`147`	`$this->sort();`
`148`	`148`
`149`		`- return !empty($this->items) ? $this->items[0] : null;`
	`149`	`+ return !empty($this->items) ? reset($this->items) : null;`
`150`	`150`	`}`
`151`	`151`
`152`	`152`	`/**`