-
-
Notifications
You must be signed in to change notification settings - Fork 426
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
Make universal routes work for controller middleware #1151
Conversation
@stancl Could you take a look at this PR. For my projects this would be the last puzzle piece to make Tenancy v3 work with Livewire v3 |
Curious how you're using controller middleware with Livewire. Are you specifying it in the component in some way? And is there a reason you can't use route middleware here? |
Livewire v3 itself now uses controller middleware for the File Upload feature by using the getMiddleware() function. Now that this Livewire PR is merged, we can define With the way this feature is coded now, there is no other way to apply middleware to this route. A PR was opened to allow custom Route definitions, but that was closed due to issues with the implementation. |
@chillbram Does your PR handle: this route as well? https://github.com/livewire/livewire/blob/1ec54cdf57c6f1bf734bb14f9732904473b91c3f/src/Features/SupportFileUploads/SupportFileUploads.php#L38 And wouldn't your [
[
"middleware" => "tenancy",
"options" => [],
],
[
"middleware" => "universal",
"options" => [],
],
] Which I think should be: [
[
"middleware" => ["tenancy", "universal"],
"options" => [],
],
] |
No it does not handle the preview file. I don't know why it doesn't implement middleware in the same way, but I've never used that feature and have no idea how it works. You're right that the array_map forms the array in that way, but that's how Laravel wants it. I guess they want to allow options to be set for each individual middleware. I tested passing an array to the middleware array key, but Laravel does not support that. |
I see, so the usage of Route::group([
'prefix' => 'foo',
'middleware' => [
['middleware' => FooMiddleware::class, 'options' => ['options for foo middleware']],
['middleware' => BarMiddleware::class, 'options' => ['options for bar middleware']],
],
], function () {
//
}); |
I don't think your code example works when used like that on the |
@stancl Do you agree with this implementation and its test coverage? |
We're looking into this at the moment. The solution used here is causing some issues with how we do things in v4, so we're looking at other ways to approach the problem. |
@chillbram could you try if changing the Livewire code locally like this fixes the issue for you? |
It works for files uploaded on a tenant, but not on the central domain. Universal routes don't seem to work with this implementation yet |
I submitted a PR to Livewire with a similar implementation to the one I linked. Once it's merged, if you could check whether the changes submitted here work with the new version of Livewire, I can merge this. Our priority is Livewire 3 support in v4, but since this is essentially a one line PR and doesn't appear to break any tests, I can merge this into v3. |
Just checked and this change still works with Livewire v3.4.1, so I think this is safe to merge into Tenancy v3 |
I added a fallback to |
This PR allows support for controller middleware.
$route->computedMiddleware
is equal toRouter::uniqueMiddleware(array_merge($route->middleware(), $route->controllerMiddleware()));
. So basically the same result as before, but with the controllerMiddleware added. I have added a test, which should cover this use case.