BoundMethod::call() sends parameters in wrong order

[tssge]

• Laravel Version: 5.5
• PHP Version: 7.1
• Database Driver & Version: MySQL 5.7 / PDO

Description:

BoundMethod::call() will send parameters to the function in wrong order in some cases. If a function parameter has a default value set and one precedes it with no default value, the default value from the second parameter will be applied to the first parameter.

Steps To Reproduce:

//... some class ...

public function list(string $url, string $type = 'all'): void
{
    var_dump($url);
    var_dump($type);
}

//... some container ...
$this->app->call("SomeClass@list", ['testparam']);

This will result in output:

string 'all' (length=3)
string 'testparam' (length=24)

So the default value for $type was sent to $url parameter instead.

Explanation:

This is because in BoundMethod on line 119

      return array_merge($dependencies, $parameters);

the results from depedency resolution are merged with no order at all. The dependencies are resolved first and after that the parameters so no order is taken into account.

How to fix:

In function addDepdencyForCallParameter take given parameters into account and fill dependecies in such a way that the order is preserved.

[tssge]

I have created a patch and can create pull request if needed. Just waiting for some input.

[sisve]

Uhm. Let there be input! And there was input!

What do you want input on? Submit the PR with the tests, and see if your solutions is approved.

[tssge]

No, it does not pass the tests. I am currently trying to figure out why this test

    public function testCallWithCallableArray()
    {
        $container = new Container;
        $stub = new ContainerTestCallStub;
        $result = $container->call([$stub, 'work'], ['foo', 'bar']);
        $this->assertEquals(['foo', 'bar'], $result);
    }

expects that this function

    public function work()
    {
        return func_get_args();
    }

returns 2 arguments even though it accepts none. To me this seems like really strange behavior: a function that accepts 0 arguments is passed 2 arguments and is expected to return correct values for those 2 arguments it never accepts.

Can you shine some light on why such a test exists and why a function that doesn't accept arguments should be tricked to accept arguments by Laravel framework?

[sisve]

Php allows you to send an infinite amount of parameters to a function. (Or at least more than those that are declared.) The code can then use func_get_args to retrieve all arguments sent.

Perhaps it's easier understood as a variadic function; public function work(...$params) { return $params; }.

The test verifies that 1) a callable array [$stub, 'work'] can be used and, 2) the parameters you passed to $container->call is forwarded to the method.

[tssge]

I was about to use addDependencyForCallParameter function to handle the parameters and dependencies, as this function already handles parameter default values. But this particular function checks the argument count instead of blindly merging dependencies with parameters and then passing this array to the function as arguments.

This is why the test doesn't pass, because the arguments given don't match the count of arguments taken.

Should such check be removed, or should we never check the argument count at all instead?

By the way, should we throw BindingResolutionException if there are parameters that cannot be resolved? Currently BoundMethod does not do so and instead let's PHP do whatever error it does when mandatory arguments are missing. This behvaior is inconsistent with the behavior of Container, which throws BindingResolutionException if an argument is mandatory and cannot be resolved.

[sisve]

I cannot answer you about specifics on the BoundMethod class, I do not know enough about it. Try #internals on Slack.