[HttpFoundation] Fix AcceptHeader #6002

Merged
merged 1 commit into from Nov 13, 2012

Conversation

Projects
None yet
2 participants
Contributor

vicb commented Nov 13, 2012

The important lines are:

<?php
-        return !empty($this->items) ? current($this->items) : null;
+        return !empty($this->items) ? $this->items[0] : null;

(and the corresponding test).

The commit has some code re-org to make reading tests easier (providers defined close the the corresponding test). This might be personal preferences only, let me know if it should be reverted.

@fabpot fabpot added a commit that referenced this pull request Nov 13, 2012

@fabpot fabpot merged branch vicb/acceptheader (PR #6002)
This PR was merged into the master branch.

Commits
-------

395c004 [HttpFoundation] Fix AcceptHeader

Discussion
----------

[HttpFoundation] Fix AcceptHeader

The important lines are:

```php
<?php
-        return !empty($this->items) ? current($this->items) : null;
+        return !empty($this->items) ? $this->items[0] : null;
```

(and the corresponding test).

The commit has some code re-org to make reading tests easier (providers defined close the the corresponding test). This might be personal preferences only, let me know if it should be reverted.
7bfe13c

fabpot merged commit 395c004 into symfony:master Nov 13, 2012

1 check was pending

default The Travis build is in progress
Details
Contributor

vicb commented Nov 13, 2012

oops last minute changes are bad... will fix

@vicb vicb added a commit to vicb/symfony that referenced this pull request Nov 13, 2012

@vicb vicb [HttpFoundation] fix #6002 f4b630d

@Tobion Tobion added a commit to Tobion/symfony that referenced this pull request Nov 13, 2012

@Tobion Tobion revert #6002
There is reason why the author used current() because the index is not necessarily zero-indexed.
3757d0a

fabpot referenced this pull request Nov 14, 2012

Merged

[HttpFoundation] fix #6002 #6003

@fabpot fabpot added a commit that referenced this pull request Nov 14, 2012

@fabpot fabpot merged branch vicb/acceptheaderfix (PR #6003)
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?

&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 #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.
225e3e5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment