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

[10.x] Allow route:list to expand middleware groups in 'VeryVerbose' mode #48703

Merged
merged 7 commits into from
Oct 11, 2023

Conversation

NickSdot
Copy link
Contributor

@NickSdot NickSdot commented Oct 11, 2023

Together with @timacdonald I've been exploring the idea of expanding the middleware groups when using route:list. We both feel that being able to see the actual middleware being used instead of group names like 'web' adds value to the command. This PR proposes this as optional functionality.

Benefits

Developers see what really is applied, instead of group names. No more jumping to e.g. the Kernel, to see what is in the group. Most importantly, the middleware are listed in the order defined by $middlewarePriority.

When merged

route:list (no change)

image

route:list -v (no change)

image

route:list -vv (expands middleware groups)

The group name web is no longer listed. Instead, the actual middleware from that group are listed. The same behaviour applies when using the --json flag.

image

Consideration

The -vv flag until now has not been used in the command. If someone were to use it in an automated way for whatever reason, this change could be considered a BC break.

However, I wanted to get the maintainer's opinion on this first.
If you consider it a BC break, there are two options:

A) Target Laravel 11
B) Add a new flag --expand-groups / -e.

Input welcome.

@NickSdot NickSdot changed the title [10.x] Allow route:list to expand middleware groups in "VeryVerbose' mode [10.x] Allow route:list to expand middleware groups in 'VeryVerbose' mode Oct 11, 2023
NickSdot added a commit to NickSdot/laravel-docs that referenced this pull request Oct 11, 2023
@NickSdot
Copy link
Contributor Author

Also prepared a docs addition, but will wait with the PR until we have a result here.

laravel/docs@10.x...NickSdot:laravel-docs:10.x#diff-1970690c078dba0956a24f726ce3ec51a6e04396faaf18f595f2cef0d2056aec

@taylorotwell
Copy link
Member

Do you know if they are sorted properly into the actual order they will be executed in? For example the SortMiddleware class will rearrange your middleware to make sure certain things run before others.

@NickSdot
Copy link
Contributor Author

NickSdot commented Oct 11, 2023

Do you know if they are sorted properly into the actual order they will be executed in? For example the SortMiddleware class will rearrange your middleware to make sure certain things run before others.

Yes, @taylorotwell, they are. I just added this to the 'Benefits' section of the description 2 minutes before you commented:

Most importantly, the middleware are listed in the order defined by $middlewarePriority.

Command calls Router::gatherRouteMiddleware() -> Router::resolveMiddleware() -> Router::sortMiddleware()

See:

return collect($this->router->gatherRouteMiddleware($route))->map(function ($middleware) {
and
public function gatherRouteMiddleware(Route $route)

Edit:

Example

image

image

image

image

@NickSdot
Copy link
Contributor Author

NickSdot commented Oct 11, 2023

Do you know if they are sorted properly into the actual order they will be executed in? For example the SortMiddleware class will rearrange your middleware to make sure certain things run before others.

Added test testMiddlewareGroupsExpandCorrectlySortedIfVeryVerbose() for this.

…ddlewarePriority() works as well, and that middleware is not displayed in non very verbose cases
@taylorotwell taylorotwell merged commit 5e444ac into laravel:10.x Oct 11, 2023
@taylorotwell
Copy link
Member

Thanks 👍

@NickSdot NickSdot deleted the route-list-expandable-groups branch October 11, 2023 16:17
@timacdonald
Copy link
Member

Lovely addition, @NickSdot. Thanks for making this happen!

@NickSdot
Copy link
Contributor Author

Thanks for ping-ponging the idea with me @timacdonald 🙏

taylorotwell added a commit to laravel/docs that referenced this pull request Oct 24, 2023
* Add docs for expandable middleware group via `php artisan route:list -vv` (laravel/framework#48703)

* Improve wording

* Formatting

* Wording

* Update routing.md

---------

Co-authored-by: Taylor Otwell <[email protected]>
timacdonald pushed a commit to timacdonald/framework that referenced this pull request Oct 24, 2023
…mode (laravel#48703)

* Allow middleware groups to be expanded with -vv

* Keep things simple and do not use group nesting

* Moved test to correct folder

* Removed useless tmp vars

* Style Changes

* Added test to ensure expanded middleware are correctly sorted

* Improved tests once again to ensure ordering via $kernel->prependToMiddlewarePriority() works as well, and that middleware is not displayed in non very verbose cases
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.

3 participants