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] Route list urifilter #40815

Closed
wants to merge 5 commits into from
Closed

[10.x] Route list urifilter #40815

wants to merge 5 commits into from

Conversation

Synchro
Copy link
Contributor

@Synchro Synchro commented Feb 4, 2022

This PR adds two new features to the route:list artisan command, to address shortcomings in the existing filtering options available in there.

Firstly, it adds filtering by domain. For example, if you have two routes that are identical, but in different domains, there is no way to separate them in the route list. The domain column is already present in the output, so this simply adds the ability to filter on it, just like the other supported columns do.

The second feature is more advanced: filtering by URI matching on resolved routes. This addresses a major shortcoming of the existing path filter, which only supports simple string matching on the path pattern itself, not URIs that match it. Since it's very common to have routes that do not contain the literal path, such as regex routes, or ones with parameters, a more thorough approach is needed.

If I want to find out which route will handle a URI like /posts/1, I could search using --path=posts, however, this would likely find several false positives, such as /posts. If the route is using model binding, it will be defined as /posts/{post}, which will fail to match a search for the URI in the path param --path=/posts/1.

This extension works like this:

./artisan route:list --uri="https://www.example.com/posts/1"

and you can also use all the other route:list options with it:

./artisan route:list --uri="https://www.example.com/posts/1" --method=PATCH --middleware=auth --columns=uri,method,middleware

The uri param interacts with the domain param, so you can provide the domain and pass a relative path inuri separately, like this:

--domain=www.example.com --uri=/posts/1

and internally it will construct a URL of https://www.example.com/posts/1, and resolve that against the routes.

It's especially useful for debugging, for example the common scenario of inadvertently putting API resource routes before other routes will often mean that a request is sent to the wrong controller; a path query would fail to spot that, but a URI match would identify it immediately and precisely. In practice, I find that filtering by path often tells you what should be happening, but matching the URI tells you what is actually happening.

In short, given a full application URL, it will unambiguously and accurately tell you exactly which route will handle the request; this functionality is not currently available through the existing params.

The implementation makes one internal change which is to pass the route itself to the filterRoute method, allowing future filtering options to do anything they like with the route without having to add yet more options to the method.

I'm submitting this as a PR against framework rather than creating a standalone extension because it is a small extension to existing functionality, and it interacts with the existing route:list params. Since there is no way to extend built-in commands like this, I would either have to clone the entire functionality of the existing command just to add this one feature, or lose all the other existing functionality it integrates with.

@GrahamCampbell GrahamCampbell changed the title Route list urifilter [10.x] Route list urifilter Feb 5, 2022
*
* @param array $route
* @return array|null
* @return array|void
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect phpdoc. Needs to be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove that if you like; I made that change because it was tripping static analysis tools.

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

@taylorotwell
Copy link
Member

I would be fine with the domain part of this PR - I'm not sure I want to take on the route matching stuff, etc.

@Synchro
Copy link
Contributor Author

Synchro commented Feb 5, 2022

Thanks for looking. The route matching is a single call; the rest is boilerplate for managing command options. While it's a small thing, it enables a whole new class of route debugging abilities.

Alternatively, can you think of a more straightforward way of being able to ask "which route handles this URL?", and getting a definitive answer, since path isn't enough?

Failing that I can break out the domain matching part.

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