revert #6002 #6004
Closed
revert #6002 #6004
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There is reason why the author used current() because the index is not necessarily zero-indexed.
|
I had replaced |
|
The test you added neither covers items[0] nor reset(). So items[0] did not fail. And what does reset() fix? It's not covered either. |
|
Yes it is covered, check Travis reports (symfony/symfony). |
|
Strange, it did not fail on my machine. I just recognised items[0] problem from reading. |
fabpot
added a commit
that referenced
this pull request
Nov 14, 2012
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.
Sign up for free
t
5934
o join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@vicb: There is reason why the author used current() because the index is not necessarily zero-indexed.