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.6] Allow callable array syntax in route definition #24385

Merged
merged 3 commits into from
May 31, 2018
Merged

[5.6] Allow callable array syntax in route definition #24385

merged 3 commits into from
May 31, 2018

Conversation

hulkur
Copy link
Contributor

@hulkur hulkur commented May 30, 2018

This PR will allow defining route actions as arrays

Route::get('smth', [SomeController::class, 'methodName']);

This has no direct benefit code-wise, but enables better navigation in IDE-s.

is_callable validates that specified class and method exist and only is_array check is needed.

@browner12
Copy link
Contributor

what is the better navigation, that you can click through to the controller?

@hulkur
Copy link
Contributor Author

hulkur commented May 30, 2018

yes, navigating to correct controller from string is bothersome

@RikSomers
Copy link

RikSomers commented May 31, 2018

Can't you just do that already manually, with about the same number of characters?

Route::get('smth', SomeController::class . '@methodName');

@hulkur
Copy link
Contributor Author

hulkur commented May 31, 2018

i guess you can, but in php array is one of the callable syntaxes so I added that

@taylorotwell taylorotwell merged commit 06c77a1 into laravel:5.6 May 31, 2018
@devcircus
Copy link
Contributor

Nice addition. I've hacked this in for years.

@rentalhost
Copy link
Contributor

It should works with code like \Route::get('x', [ 'uses' => [ A::class, 'b' ] ])?

@hulkur
Copy link
Contributor Author

hulkur commented Jun 12, 2018

It does not currently.

@rentalhost
Copy link
Contributor

Can you implement that? Seems that it could be done on line 47. It will make me very happy, once that I commonly use with uses. 😄

@hagabaka
Copy link

hagabaka commented Nov 18, 2018

I think there are two problems with this implementation:

  1. It doesn't honor the Router's group namespace. By default (set in RouteServiceProvider), routes will look up controllers in the App\Http\Controllers namespace, so using the traditional syntax the controller name does not need to have that namespace prepended, including things like Route::resource('users', UserController::class). But the array syntax requires the full class name.
  2. It doesn't work with magic controller methods using __call(), Whereas using Controller@method works when the Controller does not have that method but handles it through __call.

I think these problems as well as lack of support of ['uses' => [Controller::class, 'method']] can be fixed by changing Router::actionReferencesController and Router::convertToControllerAction, which normalizes the route action into the ['uses' => ...] form and applies the group namespace.

@williamdes
Copy link
Contributor

williamdes commented Oct 9, 2019

I would just add that you can not do :

Route::get('/hello-world/{userId}', [
    'as'   => 'call_hello_world_please',
    'uses' => [ Hello::class, 'world' ],
]);

So you need to fallback to the string syntax ... 💀

Symfony\Component\Debug\Exception\FatalThrowableError: ReflectionFunction::__construct() expects parameter 1 to be string, array given in file vendor/laravel/framework/src/Illuminate/Routing/RouteSignatureParameters.php on line 22
Stack trace:
  1. Symfony\Component\Debug\Exception\FatalThrowableError->() vendor/laravel/framework/src/Illuminate/Routing/RouteSignatureParameters.php:22
  2. ReflectionFunction->__construct() vendor/laravel/framework/src/Illuminate/Routing/RouteSignatureParameters.php:22
  3. Illuminate\Routing\RouteSignatureParameters->fromAction() vendor/laravel/framework/src/Illuminate/Routing/Route.php:471
  4. Illuminate\Routing\Route->signatureParameters() vendor/laravel/framework/src/Illuminate/Routing/ImplicitRouteBinding.php:24
  5. Illuminate\Routing\ImplicitRouteBinding->resolveForRoute() vendor/laravel/framework/src/Illuminate/Routing/Router.php:787
  6. Illuminate\Routing\Router->substituteImplicitBindings() vendor/laravel/framework/src/Illuminate/Routing/Middleware/SubstituteBindings.php:39
  7. Illuminate\Routing\Middleware\SubstituteBindings->handle() vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:163
  8. Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}() vendor/laravel/framework/src/Illuminate/Routing/Pipeline.php:53
  9. Illuminate\Routing\Pipeline->Illuminate\Routing\{closure}() vendor/laravel/framework/src/Illuminate/Auth/Middleware/Authenticate.php:43
 10. Illuminate\Auth\Middleware\Authenticate->handle() vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:163
 11. Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}() vendor/laravel/framework/src/Illuminate/Routing/Pipeline.php:53
 12. Illuminate\Routing\Pipeline->Illuminate\Routing\{closure}() vendor/laravel/framework/src/Illuminate/Routing/Middleware/ThrottleRequests.php:58
 13. Illuminate\Routing\Middleware\ThrottleRequests->handle() vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:163
 14. Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}() vendor/laravel/framework/src/Illuminate/Routing/Pipeline.php:53
 15. Illuminate\Routing\Pipeline->Illuminate\Routing\{closure}() vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:104

@hulkur
Copy link
Contributor Author

hulkur commented Oct 9, 2019

This was rejected in #24589

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants