Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.4] Bring back an old behaviour in resolving controller method dependencies #18646

Merged
merged 9 commits into from
Apr 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions src/Illuminate/Routing/RouteDependencyResolverTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ protected function resolveClassMethodDependencies(array $parameters, $instance,
*/
public function resolveMethodDependencies(array $parameters, ReflectionFunctionAbstract $reflector)
{
$results = [];

$instanceCount = 0;

$values = array_values($parameters);
Expand All @@ -51,14 +49,14 @@ public function resolveMethodDependencies(array $parameters, ReflectionFunctionA
if (! is_null($instance)) {
$instanceCount++;

$results[$parameter->getName()] = $instance;
} else {
$results[$parameter->getName()] = isset($values[$key - $instanceCount])
? $values[$key - $instanceCount] : $parameter->getDefaultValue();
$this->spliceIntoParameters($parameters, $key, $instance);
} elseif (! isset($values[$key - $instanceCount]) &&
$parameter->isDefaultValueAvailable()) {
$this->spliceIntoParameters($parameters, $key, $parameter->getDefaultValue());
}
}

return $results;
return $parameters;
}

/**
Expand Down Expand Up @@ -93,4 +91,19 @@ protected function alreadyInParameters($class, array $parameters)
return $value instanceof $class;
}));
}

/**
* Splice the given value into the parameter list.
*
* @param array $parameters
* @param string $offset
* @param mixed $value
* @return void
*/
protected function spliceIntoParameters(array &$parameters, $offset, $value)
{
array_splice(
$parameters, $offset, 0, [$value]
);
}
}
121 changes: 103 additions & 18 deletions tests/Routing/RoutingRouteTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -309,24 +309,6 @@ public function testClassesCanBeInjectedIntoRoutes()
unset($_SERVER['__test.route_inject']);
}

public function testClassesAndVariablesCanBeInjectedIntoRoutes()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this test removed? Makes me a little nervous that perhaps we are breaking something?

Copy link
Member Author

@themsaid themsaid Apr 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To transform a route method dependency we first check if the class already exists in the parameters passed to the route if so then we assume it's a Model so we pass it as is.

The given test contains two type-hinted stdClass arguments, checking for the second stdClass we'll find that it's already in parameters when the first stdClass was resolved, as a result we won't resolve it and we pass the default value instead.

That test used to work before because we were building a brand new array of results instead of mutating the $parameters array, and thus when we check for the second stdClass in $parameters it won't exist since the first resolved one was stored in the $results array not the $parameters array.

For example before this PR you could have function show(Request $request, $id, Request $sameRequest) and you get two arguments with a Request instance, which isn't considered a breaking change because it actually doesn't make sense to resolve the same method dependency twice.

The fix that removed test was supposed to check is now tested here: https://github.com/laravel/framework/pull/18646/files#diff-4480aa925f3c087f5bd9c0603d1eb0fcR439

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that test was the only test introduced #17973 that started my issue. What test is now covering the changes in that PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A majority of those tests are just rewritten tests from my PR. They ensure the 5.4.12 behavior, they do not verify the new functionality in 5.4.13. Is it the invocation of withModels that is the new behavior?

{
unset($_SERVER['__test.route_inject']);
$router = $this->getRouter();
$router->get('foo/{var}/{bar?}/{baz?}', function (stdClass $foo, $var, $bar = 'test', stdClass $baz = null) {
$_SERVER['__test.route_inject'] = func_get_args();

return 'hello';
});
$this->assertEquals('hello', $router->dispatch(Request::create('foo/bar', 'GET'))->getContent());
$this->assertInstanceOf('stdClass', $_SERVER['__test.route_inject'][0]);
$this->assertEquals('bar', $_SERVER['__test.route_inject'][1]);
$this->assertEquals('test', $_SERVER['__test.route_inject'][2]);
$this->assertInstanceOf('stdClass', $_SERVER['__test.route_inject'][3]);
$this->assertArrayHasKey(3, $_SERVER['__test.route_inject']);
unset($_SERVER['__test.route_inject']);
}

public function testOptionsResponsesAreGeneratedByDefault()
{
$router = $this->getRouter();
Expand Down Expand Up @@ -420,6 +402,56 @@ public function testRouteParametersDefaultValue()
$this->assertEquals('foo', $router->dispatch(Request::create('foo', 'GET'))->getContent());
}

public function testControllerCallActionMethodParameters()
{
$router = $this->getRouter();

// Has one argument but receives two
unset($_SERVER['__test.controller_callAction_parameters']);
$router->get(($str = str_random()).'/{one}/{two}', 'Illuminate\Tests\Routing\RouteTestAnotherControllerWithParameterStub@oneArgument');
$router->dispatch(Request::create($str.'/one/two', 'GET'));
$this->assertEquals(['one' => 'one', 'two' => 'two'], $_SERVER['__test.controller_callAction_parameters']);

// Has two arguments and receives two
unset($_SERVER['__test.controller_callAction_parameters']);
$router->get(($str = str_random()).'/{one}/{two}', 'Illuminate\Tests\Routing\RouteTestAnotherControllerWithParameterStub@twoArguments');
$router->dispatch(Request::create($str.'/one/two', 'GET'));
$this->assertEquals(['one' => 'one', 'two' => 'two'], $_SERVER['__test.controller_callAction_parameters']);

// Has two arguments but with different names from the ones passed from the route
unset($_SERVER['__test.controller_callAction_parameters']);
$router->get(($str = str_random()).'/{one}/{two}', 'Illuminate\Tests\Routing\RouteTestAnotherControllerWithParameterStub@differentArgumentNames');
$router->dispatch(Request::create($str.'/one/two', 'GET'));
$this->assertEquals(['one' => 'one', 'two' => 'two'], $_SERVER['__test.controller_callAction_parameters']);

// Has two arguments with same name but argument order is reversed
unset($_SERVER['__test.controller_callAction_parameters']);
$router->get(($str = str_random()).'/{one}/{two}', 'Illuminate\Tests\Routing\RouteTestAnotherControllerWithParameterStub@reversedArguments');
$router->dispatch(Request::create($str.'/one/two', 'GET'));
$this->assertEquals(['one' => 'one', 'two' => 'two'], $_SERVER['__test.controller_callAction_parameters']);

// No route parameters while method has parameters
unset($_SERVER['__test.controller_callAction_parameters']);
$router->get(($str = str_random()).'', 'Illuminate\Tests\Routing\RouteTestAnotherControllerWithParameterStub@oneArgument');
$router->dispatch(Request::create($str, 'GET'));
$this->assertEquals([], $_SERVER['__test.controller_callAction_parameters']);

// With model bindings
unset($_SERVER['__test.controller_callAction_parameters']);
$router->get(($str = str_random()).'/{user}/{defaultNull?}/{team?}', [
'middleware' => SubstituteBindings::class,
'uses' => 'Illuminate\Tests\Routing\RouteTestAnotherControllerWithParameterStub@withModels',
]);
$router->dispatch(Request::create($str.'/1', 'GET'));

$values = array_values($_SERVER['__test.controller_callAction_parameters']);

$this->assertInstanceOf('Illuminate\Http\Request', $values[0]);
$this->assertEquals(1, $values[1]->value);
$this->assertNull($values[2]);
$this->assertInstanceOf('Illuminate\Tests\Routing\RoutingTestTeamModel', $values[3]);
}

/**
* @expectedException \Symfony\Component\HttpKernel\Exception\NotFoundHttpException
*/
Expand Down Expand Up @@ -1293,6 +1325,34 @@ public function returnParameter($bar = '')
}
}

class RouteTestAnotherControllerWithParameterStub extends Controller
{
public function callAction($method, $parameters)
{
$_SERVER['__test.controller_callAction_parameters'] = $parameters;
}

public function oneArgument($one)
{
}

public function twoArguments($one, $two)
{
}

public function differentArgumentNames($bar, $baz)
{
}

public function reversedArguments($two, $one)
{
}

public function withModels(Request $request, RoutingTestUserModel $user, $defaultNull = null, RoutingTestTeamModel $team = null)
{
}
}

class RouteTestResourceControllerWithModelParameter extends Controller
{
public function show(RoutingTestUserModel $fooBar)
Expand Down Expand Up @@ -1471,6 +1531,31 @@ public function firstOrFail()
}
}

class RoutingTestTeamModel extends Model
{
public function getRouteKeyName()
{
return 'id';
}

public function where($key, $value)
{
$this->value = $value;

return $this;
}

public function first()
{
return $this;
}

public function firstOrFail()
{
return $this;
}
}

class RoutingTestExtendedUserModel extends RoutingTestUserModel
{
}
Expand Down