Closed sgehrig closed 8 years ago
Hi @sgehrig, that's definitely not cool, and I am sorry for that.
There are only two occurences of the array_map()
function: https://github.com/willdurand/Negotiation/blob/5154ac7346ed2821dd55e2b70cb865d54f7cbca7/src/Negotiation/AbstractNegotiator.php#L27-L28. Without any test data, it seems complicated to come up with a fix but I am going to investigate which data might trigger such a warning using the test suite.
@sgehrig: which PHP version do you have in prod?
Currently it's still an Ubuntu 14.04LTS with PHP 5.5.somewhat
.
Oh and thanks for getting into that so quickly :-)
Thank you. That looks tricky because this "bug" should be covered by a test case (here).
Which negotiator are you using in your code?
FriendsOfSymfony/FOSRestBundle uses a FOS\RestBundle\Negotiation\FormatNegotiator
which extends Negotiation\Negotiator
. The getBest()
method is overwritten but calls parent::getBest()
. So the object being returned from Negotiation\AbstractNegotiator::acceptFactory()
seems to be a Negotiation\Accept
.
Unfortunately FOS\RestBundle\Negotiation\FormatNegotiator:: getBest()
does some more things before calling parent::getBest()
but essentially this boils down to
Accept
header from a Symfony\Component\HttpFoundation\Request
But still, in the end some values are fed into Negotiation\AbstractNegotiator::getBest()
and then the error occurs.
It is a bug in Negotiation, not in FOSRestBundle IMO
@willdurand: Updated my comment - sometimes I should think to the end before writing a comment ;-) You're right, logically it's more likely that the bug is in Negotiation.
PHP version is PHP 5.5.9-1ubuntu4.19
@sgehrig actually, they do have a array_map
call: https://github.com/FriendsOfSymfony/FOSRestBundle/blob/c1b87d933dfc9b59fc603f8e44f8064e4530a2b1/Negotiation/FormatNegotiator.php#L116-L118. I cannot reproduce the bug to be honest, I even tried with PHP 5.4.0...
That's the stack trace I'm getting from New Relic.
Maybe the second argument of getBest()
is not an array of strings. This would make the explode()
function to throw a warning inside one of the array_map
calls.
Could it be that PHPUnit interferes here in the sense that the PHPUnit error handler somehow mitigates the problem with PHP bug #55416?
I don't know. PHPUnit keeps track of the E_WARNING
normally.
If I disable these in the phpunit.xml
:
convertErrorsToExceptions="false"
convertNoticesToExceptions="false"
convertWarningsToExceptions="false"
I'm getting
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.
............................................................... 63 / 118 ( 53%)
..............PHP Warning: array_map(): An error occurred while invoking the map callback in /Users/stefan/Documents/Negotiation/src/Negotiation/AbstractNegotiator.php on line 27
.........................................
Time: 63 ms, Memory: 5.75MB
OK (118 tests, 203 assertions)
In fact it's the convertErrorsToExceptions="true"
setting that causes PHPUnit to somehow circumvent the problem with array_map()
.
That is interesting 😋
Great. I fixed the issue I guess.
I hate it when I don't know why things are getting weird, so I dug around PHPUnit a little bit. I assume that's the real issue behind this test succeeding even though it should fail:
Alright, so I pushed a fix for the warning in #82. It does solve the E_WARNING
problem, but you will now get an exception because the accept header or the priorities are not well-formatted. I believe you have correct priorities, hence I guess the issue is only related to the Accept
header sent by the clients, e.g., Android devices (as you suggested).
That being said, question is: should Negotiation handle/accept incorrect Accept
headers?
IMO, it should enforce correct headers, otherwise it would not make much sense. I believe that the decision must be left to the caller of the lib, e.g., FOSRestBundle. Their negotiator should catch the exceptions, and deal with them. For example, if the exception thrown is of type InvalidMediaType
, then it could default to the configured fallback.
WDYT?
That's good question though. The purpose of the getBest()
method is to return the best AcceptHeader
based on the input from a request header and a list of prioritized options. The request header is coming in from the client and there's nothing we can do to enforce their correctness. On the other hand I assume the list of prioritized options is coming from the system using the Negotiation library and therefore its content is controlled by that system.
So in case the header coming in from a client contains incorrect values I personally would assume that getBest()
would just ignore these invalid values and continue until it finds a best match or not. Such a malformed option cannot be the best match naturally but I think there's no need to catch such an exception and deal with the invalid value outside the negotiator because there isn't a way to stop clients from sending such malformed headers.
The case is different for the list of prioritized options. Here I'd expect an exception to be thrown if there's a malformed value because then the system's negotiation base is wrong.
But I think, changing this will lead to a BC break - but at least it'll resolve the main issue very quickly ;-)
The request header is coming in from the client and there's nothing we can do to enforce their correctness
Agreed.
What you said here makes perfectly sense. I am wondering how to fix the whole thing, because it might not be a BC break actually, but a bug fix.
Would you want to work on a fix based on my PR? I think it only needs a try/catch in the loop of the header string.
Sure. Will take a look on Tuesday.
Added pull request #84 - used your changes from pull request #82
Just released Negotiation 2.0.3
Thanks for your quick help!
After deploying the negotiation library as part of FriendsOfSymfony/FOSRestBundle version 2.0.0 we started to get
from
Negotiation\AbstractNegotiator::getBest()
(27 or 28). I assume this happens becauseNegotiation\Accept::__construct()
may throw an exception when constructed with an invalid value. This in turn seems to trigger PHP bug #55416 causing a warning to be emitted.I don't know which exact value seems to cause this problem as the issue is not reproducible and only happens sparsely on our live servers - most often with some Android phones.
Can this part be rewritten somehow to circumvent PHP bug #55416? Or can the logic be implemented a little bit more tolerant to weird values coming in from web clients?