[5.4] Breaking change in ControllerDispatcher::resolveMethodDependencies

[sisve]

• Laravel Version: 5.4.13
• PHP Version: 7.1.1

Description:

The commit 544faac changed ControllerDispatcher::resolveMethodDependencies in a breaking way. It previously kept the array keys, and replaced values in-place for method dependencies. It was changed so that it will now build a new array, without keeping the previous keys.

I am using the previous behavior (and the keys) to reorder the array in Controller::callAction to allow the methods to have another order of parameters than the order of the route segments, provided they have the same name.

Test To Reproduce:

use Illuminate\Routing\ControllerDispatcher;

class ControllerDispatcherTest extends \Illuminate\Foundation\Testing\TestCase {

    use CreatesApplication;

    public function testResolveMethodDependencies() {
        $parameters = array(
            'locale' => 'tlh'
        );
        $target = new ReflectionMethod(self::class, 'testResolveMethodDependencies_Target');

        $controllerDispatcher = new ControllerDispatcher($this->app);
        $actual = $controllerDispatcher->resolveMethodDependencies($parameters, $target);

        $expected = array(
            'locale' => 'tlh'
        );

        $this->assertEquals($expected, $actual);
    }

    protected function testResolveMethodDependencies_Target($locale) {
    }

}

5.4.13 result:

1) ControllerDispatcherTest::testResolveMethodDependencies
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'locale' => 'tlh'
+    0 => 'tlh'
 )

[themsaid]

Fixed in #18604

[sisve]

Another test that still fails, a route like /{tool}/{locale}, and an action that doesn't use all the segments. It would probably fail if I were to change the order of the parameters too.

use Illuminate\Routing\ControllerDispatcher;

class ControllerDispatcherTest extends \Illuminate\Foundation\Testing\TestCase {

    use CreatesApplication;

    public function testResolveMethodDependencies() {
        $parameters = array(
            'tool' => 'Grabthar\'s hammer',
            'locale' => 'tlh'
        );
        $target = new ReflectionMethod(self::class, 'resolveMethodDependencies_Target');

        $controllerDispatcher = new ControllerDispatcher($this->app);
        $actual = $controllerDispatcher->resolveMethodDependencies($parameters, $target);

        $expected = array(
            'tool' => 'Grabthar\'s hammer',
            'locale' => 'tlh'
        );

        $this->assertEquals($expected, $actual);
    }

    protected function resolveMethodDependencies_Target($locale) {
    }

}

This works on 5.4.12 but fails on 5.4.13

1) ControllerDispatcherTest::testResolveMethodDependencies
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'tool' => 'Grabthar's hammer'
-    'locale' => 'tlh'
+    'locale' => 'Grabthar's hammer'
)

[themsaid]

Reopening to look more into the other failing test.

[sisve]

Here's another thing that broke; parameters without default values.

use Illuminate\Routing\ControllerDispatcher;

class ControllerDispatcherTest extends \Illuminate\Foundation\Testing\TestCase {

    use CreatesApplication;

    public function testResolveMethodDependencies() {
        $parameters = array();
        $target = new ReflectionMethod(self::class, 'resolveMethodDependencies_Target');

        $controllerDispatcher = new ControllerDispatcher($this->app);
        $actual = $controllerDispatcher->resolveMethodDependencies($parameters, $target);

        $expected = array();

        $this->assertEquals($expected, $actual);
    }

    protected function resolveMethodDependencies_Target($test) {
    }

}

1) ControllerDispatcherTest::testResolveMethodDependencies
ReflectionException: Internal error: Failed to retrieve the default value

~/vendor/laravel/framework/src/Illuminate/Routing/RouteDependencyResolverTrait.php:57

[sisve]

I've updated by previous test. The parameters did not have to be type-hinted, it was enough for them to not have a default value.

[sisve]

I'm closing this issue since Laravel 5.4.18 has been released with the changes needed.