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

[8.x] Allows Stringable objects as middleware. #39439

Merged
merged 3 commits into from
Nov 2, 2021
Merged

[8.x] Allows Stringable objects as middleware. #39439

merged 3 commits into from
Nov 2, 2021

Conversation

DarkGhostHunter
Copy link
Contributor

@DarkGhostHunter DarkGhostHunter commented Nov 1, 2021

What?

You can use Stringable objects into middleware declaration. These will be transformed into a string automatically. Pretty much like a Rule::exists() works.

use Illuminate\Support\Facades\Route;
use App\Http\Middleware\Helpers\Subscribe;

Route::get('library', 'LibraryController@index')
    ->middleware(Subscribed::to('premium')->can('listen')->except('admin'));

Why?

This allows to use helpers for middleware that contain multiple parameters into one fluid sentence. Useful for packages that have middleware with a parameters that should be overridden per-route basis.

For example, the built-in ThrottlesRequest middleware which has 3 parameters. Using a Stringable-based helper, we can transform the declaration into something more understandable forwarding the call to the helper.

// Before
Route::get('library', 'LibraryController@index')
     ->middleware('throttle:120,5,download-book');

// After
Route::get('library', 'LibraryController@index')
     ->middleware(ThrottleRequests::as('download-book')->for(5)->max(120));

Also works on Resource routes:

 $this->router->resource('song', 'SongController::class')
                     ->only('index')
                     ->middleware(['auth', ThrottleRequests::as('download-book')->for(5)->max(120)]);

BC?

  • No, it's adds a checks if the middleware is not a string and casts it to a string.

@taylorotwell
Copy link
Member

taylorotwell commented Nov 2, 2021

Does this work for route groups, etc?

Route::middleware()->group(fn);

@DarkGhostHunter
Copy link
Contributor Author

Does this work for route groups, etc?

Route::middleware()->group(fn);

Yes. I added a new test to be 200% sure.

@DarkGhostHunter
Copy link
Contributor Author

Test stalled (HUGE THANKS WINDOWS). Can someone make a re-run?

@taylorotwell taylorotwell merged commit 8febec7 into laravel:8.x Nov 2, 2021
@taylorotwell
Copy link
Member

I may need to revert this already @DarkGhostHunter

This simple test doesn't even work locally:

image

image

@DarkGhostHunter
Copy link
Contributor Author

DarkGhostHunter commented Nov 2, 2021

What does the stack trace says?

Weird since supposedly the value is casted as a string on ingress. If it's an array, it will do it for each member.

@taylorotwell
Copy link
Member

Try it in your own application so we don't have to debug over GH issues.

@DarkGhostHunter
Copy link
Contributor Author

DarkGhostHunter commented Nov 2, 2021 via email

@DarkGhostHunter
Copy link
Contributor Author

Got it. Forgot to add it to the Route instance. Route::middleware() will work, but Route::get()->middleware() will not as it uses the Route instance itself instead of the registrar.

Adding a PR to fix this...

@DarkGhostHunter
Copy link
Contributor Author

Fixed in #39449

@mingyoung
Copy link

@DarkGhostHunter

if ($key === 'middleware') {
foreach ($value as $index => $middleware) {
$value[$index] = (string) $middleware;
}
}

image

It will throw when I use closure middleware.
image

@DarkGhostHunter
Copy link
Contributor Author

Docblock states only array or string are accepted.

@lupinitylabs
Copy link
Contributor

I ran into the same issue like @mingyoung using closure middleware.
I have no idea where I picked it up from, but it's obviously neither in the documentation nor in the tests... 🤷

@DarkGhostHunter
Copy link
Contributor Author

DarkGhostHunter commented Nov 29, 2021 via email

@lupinitylabs
Copy link
Contributor

I think that is, word by word, exactly what I wrote 😅

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.

None yet

5 participants