Skip to content

[10.x] Named static methods for middleware #46362

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

Merged
merged 6 commits into from
Apr 24, 2023

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
@@ -63,7 +77,7 @@ public function handle($request, Closure $next, $redirectToRoute = null, $passwo
}

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants