[HttpFoundation] fix #6002 #6003
Conversation
|
Can you add a test? |
|
hehe see #6004 there is also a test |
|
There is a test in the original PR, no need for 6004. |
|
Which is why is was failing btw |
|
The test in 6002 did not fail for me without your patch. |
|
Merge. Go go go :) ----- Reply message ----- — Reply to this email directly or view it on GitHub. |
|
@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. |
|
@Tobion php.net states: The I have no clue what the "element that's currently being pointed to by the internal pointer" in this method so Validated ? |
|
I agree |
|
As explained in #6004, there is already a test from my first PR that made Travis go red. |
This PR was merged into the master branch. Commits ------- f4b630d [HttpFoundation] fix #6002 Discussion ---------- [HttpFoundation] fix #6002 --------------------------------------------------------------------------- by fabpot at 2012-11-13T17:02:45Z Can you add a test? --------------------------------------------------------------------------- by Tobion at 2012-11-13T17:04:23Z hehe see #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 #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 #6002 (#6003) Date : mer., nov. 14, 2012 13:47 @Tobion @vicb What do we do? Just revert #6002 or merge this PR? — 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 #6004 --------------------------------------------------------------------------- by vicb at 2012-11-14T16:20:57Z As explained in #6004, there is already a test from my first PR that made Travis go red.
No description provided.