E_WARNING: array_map(): An error occurred while invoking the map callback

[sgehrig]

After deploying the negotiation library as part of FriendsOfSymfony/FOSRestBundle version 2.0.0 we started to get

E_WARNING: array_map(): An error occurred while invoking the map callback

from Negotiation\AbstractNegotiator::getBest() (27 or 28). I assume this happens because Negotiation\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?

[willdurand]

Hi @sgehrig, that's definitely not cool, and I am sorry for that.

There are only two occurences of the array_map() function:

Negotiation/src/Negotiation/AbstractNegotiator.php

Lines 27 to 28 in 5154ac7

	$headers = array_map(array($this, 'acceptFactory'), $headers);
	$priorities = array_map(array($this, 'acceptFactory'), $priorities);

. 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.

[willdurand]

@sgehrig: which PHP version do you have in prod?

[sgehrig]

Currently it's still an Ubuntu 14.04LTS with PHP 5.5.somewhat.

Oh and thanks for getting into that so quickly :-)

[willdurand]

Thank you. That looks tricky because this "bug" should be covered by a test case (here).

Which negotiator are you using in your code?

[sgehrig]

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

• get Accept header from a Symfony\Component\HttpFoundation\Request
• doing some magic with priorities

But still, in the end some values are fed into Negotiation\AbstractNegotiator::getBest() and then the error occurs.

[willdurand]

It is a bug in Negotiation, not in FOSRestBundle IMO

[sgehrig]

@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.

[sgehrig]

PHP version is PHP 5.5.9-1ubuntu4.19

[willdurand]

@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...

[sgehrig]

That's the stack trace I'm getting from New Relic.

[willdurand]

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.

[sgehrig]

Could it be that PHPUnit interferes here in the sense that the PHPUnit error handler somehow mitigates the problem with PHP bug #55416?

[willdurand]

I don't know. PHPUnit keeps track of the E_WARNING normally.

[sgehrig]

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().

[willdurand]

That is interesting 😋

[willdurand]

Great. I fixed the issue I guess.

[sgehrig]

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:

sebastianbergmann/phpunit#1505

[willdurand]

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?

[sgehrig]

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.

[sgehrig]

But I think, changing this will lead to a BC break - but at least it'll resolve the main issue very quickly ;-)

[willdurand]

The request header is coming in from the client and there's nothing we can do to enforce their correctness

Agreed.

[willdurand]

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.

[sgehrig]

Sure. Will take a look on Tuesday.

[sgehrig]

Added pull request #84 - used your changes from pull request #82

[willdurand]

Just released Negotiation 2.0.3

[sgehrig]

Thanks for your quick help!