[5.5] Preserve parameter order in BoundMethod::call()

[tssge]

This pull request fixes #22687

We will first resolve the dependencies normal way, but in this commit we
will keep track on the order of the parameters.

Then we will loop through given parameters if all were not consumed and consume these if
we have some unfilled parameters left in our function call.

This way we prevent the default value of some parameter being applied to
the wrong position.

We will need to loop the parameters outside of
addDependencyForCallParameter, because calls to this function check for
parameter count of the function and send the ReflectionParameter as
argument. Because of according to test
ContainerTest::testCallWithCallableArary we must give parameters to
function even if the function parameter count mismatches, we cannot
perform this inside of addDependencyCallParameter.

[taylorotwell]

With your change I still do not get the proper results exactly:

[tssge]

The results are proper, as I decided to keep the old behavior. As seen in test

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

the arguments that go over the function argument count should be passed to the function nevertheless according to this test.

As in your example, the function is given 3 arguments: 2 chosen by the user and 1 default. These 3 are passed as was done previously by Laravel.

I necessarily do not agree with this behavior myself, but I decided to preserve it due to backwards compatibility.

If you are saying that it is wrong to pass the default value and then the given one, this was done previously as well by BoundMethod::getMethodDependencies:

    protected static function getMethodDependencies($container, $callback, array $parameters = [])
    {
        $dependencies = [];

        foreach (static::getCallReflector($callback)->getParameters() as $parameter) {
            static::addDependencyForCallParameter($container, $parameter, $parameters, $dependencies);
        }

        return array_merge($dependencies, $parameters);
    }

after resolving the dependencies (this will resolve default values as well) it will simply merge the resolved dependencies and given parameters having the same end result as in your test.

Running your test without my patch results in the following result

array(3) {
  [0] =>
  string(7) "default"
  [1] =>
  string(3) "foo"
  [2] =>
  string(7) "default"
}

which is the same, but the argument order is wrong. And that is what my patch fixes.

[taylorotwell]

I guess I don't see the point of passing 3 arguments to a method that only expects 2?

[tssge]

Me neither and I'd rather check the argument count and only send the correct amount of arguments. But I did not want to change the existing test, because someone might depend on this behavior, as it has been previously outlined in a test that arguments are sent to the function regardless of how many arguments it actually accepts.

Also, I think we should throw BindingResolutionException if we are unable to resolve a dependency. Currently BoundMethod just blindly calls the function even if it were unable to resolve all dependencies. I would see that throwing in this case would be consistent with the behavior of Container.

How do you think I should proceed?

[taylorotwell]

Looking at the existing code I don't think it was ever intended to be called in the way you are describing. It is meant to be called like this:

[tssge]

I believe you are mistaken. Please see test testCallWithCallableArray in Container.

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

https://github.com/laravel/framework/blob/5.5/tests/Container/ContainerTest.php#L525

This test in particular enforces that this is intended usage.

Though if it weren't intended usage, this would mean that there would be bug on line https://github.com/laravel/framework/blob/5.5/src/Illuminate/Container/BoundMethod.php#L119 which sends unnamed parameters to the function when they weren't supposed to be sent.

I can create a patch either way, just tell me how it's supposed to work and I'll fix the bug in question.