Skip to content

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Mar 6, 2023

Continuation of #46219. Framework first version of "HasParameters".

This PR introduces a more "typed" API for all the first party middleware - which gives a nicer DX than the "stringy" API, in my opinion.

Route::get('users', UserController::class) ->middleware([ Authenticate::class, // default. Authenticate::using('web'), // specify a guard. Authenticate::using('web', 'another'), // specify multiple guards. AuthenticateWithBasicAuth::class, // default. AuthenticateWithBasicAuth::using('web'), // specify a guard. AuthenticateWithBasicAuth::using('web', 'field'), // specify field AuthenticateWithBasicAuth::using(field: 'field'), // supports named arguments Authorize::using('access-nova'), // specify an ability. Authorize::using('store', Post::class), // specify an ability with a model. Authorize::using('update', 'post', 'comment'), // specify multiple models. EnsureEmailIsVerified::class, // default. EnsureEmailIsVerified::redirectTo('route.name'), // specify a redirect. RequirePassword::class, // default. RequirePassword::using('route.name'), // specify a redirect. RequirePassword::using('route.name', 100), // specify both. RequirePassword::using(passwordTimeoutSeconds: 100), // supports named arguments SetCacheHeaders::using('max_age=120;no-transform;s_maxage=60'), // using a string. SetCacheHeaders::using([ 'max_age=120', 'no-transform', 's_maxage=60', ]), // using a list of values. SetCacheHeaders::using([ 'max_age' => 120, 'no-transform', 's_maxage' => '60', ]), // using a hash/list of values. ValidateSignature::class, // default (absolute). ValidateSignature::relative(), // relative URL. ValidateSignature::absolute(), // absolute URL. This is the default, but provided a named method for completeness. ThrottleRequests::using('gold-tier'), // named rate limiter ThrottleRequests::with(100, 1, 'foo'), // custom with attempts, decay, and prefix ThrottleRequests::with(prefix: 'foo'), // supports named arguments. ]);

If we made this the documented approach, we could potentially remove the middleware aliases from the kernel.

https://github.com/laravel/laravel/blob/5070934fc5fb8bea7a4c8eca44a6b0bd59571be7/app/Http/Kernel.php#L48-L66

@timacdonald timacdonald force-pushed the middleware-2 branch 2 times, most recently from 13be1eb to 5a870cc Compare April 20, 2023 00:12
@timacdonald timacdonald marked this pull request as ready for review April 20, 2023 01:36

return $this->responseFactory->redirectGuest(
$this->urlGenerator->route($redirectToRoute ?? 'password.confirm')
$this->urlGenerator->route($redirectToRoute ?: 'password.confirm')
Copy link
Member Author

@timacdonald timacdonald Apr 20, 2023

Choose a reason for hiding this comment

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

$redirectToRoute may be null if no arguments, but it may also be an empty string when only passwordTimeoutSeconds is specified.

"password.confirm:,300" // or with this new syntax RequirePassword::using(passwordTimeoutSeconds: 300);
@newtonjob
Copy link

I feel that using potentially works for all cases, and should probably be available for all the middleware classes.

Authorize::using('admin') may read nicer than Authorize::can('admin').

Also, I think middleware aliases have their place, especially for simple middleware with few or no parameters.

@laserhybiz
Copy link
Contributor

I would suggest naming the using() method to withParameters().

I think this functionality should either be implemented in a base middleware class which all middleware should extend or a trait like the original package, this will be helpful when creating for custom middleware with the make:middleware command to not have to write this functionality manually.

@timacdonald
Copy link
Member Author

timacdonald commented Apr 20, 2023

After taking a beat, I think I could get behind using for the authorise middleware. can feels a bit janky. I prefer the other different named methods, but wouldn’t be mad if we just used using everywhere.

I don’t think it makes sense to put this in a base class / trait for what we are trying to achieve. You lose named parameters, which is a nice feature of this, and then you’d have to have a method docblock for each middleware anyway to specify the parameters / types etc.

@timacdonald
Copy link
Member Author

I've renamed Authorize::can(...) to Authorize::using(...).

@laravel laravel deleted a comment from Mastacow Apr 24, 2023
@taylorotwell taylorotwell merged commit 7136338 into laravel:10.x Apr 24, 2023
@timacdonald timacdonald deleted the middleware-2 branch April 25, 2023 22:53
milwad-dev pushed a commit to milwad-dev/framework that referenced this pull request May 12, 2023
* wip * Standardise of `using` for Authorization middleware * Update ValidateSignature.php * Update ThrottleRequests.php * Update ThrottleRequests.php * Update RequirePassword.php --------- Co-authored-by: Taylor Otwell <taylor@laravel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants