Skip to content

[Routing] Add Requirement, a collection of universal regular-expressions constants to use as route parameter requirements #45528

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 15, 2022
Merged

[Routing] Add Requirement, a collection of universal regular-expressions constants to use as route parameter requirements #45528

merged 1 commit into from
Apr 15, 2022

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Feb 23, 2022 •
edited
Loading

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

Ref #44665.

We need a way to easily reference some universal complicated regexes in routes parameters requirements, for example:

#[Route(path: '/users/{id}', requirements: ['id' => Requirement::UUID])]

#[Route(path: '/users/{id<'.Requirement::UID_BASE58.'>}')]

What about having a collection in the routing component itself? I'm sure there are other common cases we would add here.

@carsonbot carsonbot added this to the 6.1 milestone Feb 23, 2022
@fancyweb fancyweb force-pushed the routing/requirement-c branch 2 times, most recently from 5a0001f to 299ac1c Compare February 23, 2022 15:48
@nicolas-grekas
Copy link
Member

This is an alternative to #26529 and it fixes #26524
With attributes being mainstream, this is a very workable solution!

@nicolas-grekas nicolas-grekas mentioned this pull request Feb 23, 2022
Closed
@fancyweb fancyweb force-pushed the routing/requirement-c branch from 299ac1c to 4cfdec7 Compare February 23, 2022 15:54
@fancyweb fancyweb force-pushed the routing/requirement-c branch 2 times, most recently from 98ade84 to 019c4d3 Compare February 23, 2022 16:00
@fancyweb fancyweb changed the title [Routing] Add Requirement, a collection of universal regular expressions to use as routes parameters requirements [Routing] Add Requirement, a collection of universal regular expressions to use as route parameter requirements Feb 23, 2022
@fancyweb fancyweb force-pushed the routing/requirement-c branch from 019c4d3 to d22a7bb Compare February 23, 2022 16:04
@fancyweb fancyweb changed the title [Routing] Add Requirement, a collection of universal regular expressions to use as route parameter requirements [Routing] Add Requirement, a collection of universal regular-expressions constants to use as route parameter requirements Feb 23, 2022
@ro0NL
Copy link
Contributor

ro0NL commented Feb 23, 2022

Now i'm wondering ... when by default (any format allowed) can we me make the canonical format also the canonical URL at HTTP level? https://developers.google.com/search/docs/advanced/crawling/consolidate-duplicate-urls

@fancyweb fancyweb force-pushed the routing/requirement-c branch 2 times, most recently from 2d7d85d to 1530a42 Compare February 24, 2022 09:27
@fancyweb fancyweb force-pushed the routing/requirement-c branch from 1530a42 to 41bb856 Compare February 24, 2022 15:27
@fancyweb fancyweb force-pushed the routing/requirement-c branch from 41bb856 to 3d16f29 Compare March 28, 2022 08:56
@fabpot
Copy link
Member

fabpot commented Mar 28, 2022

I like the idea.

Do we envision adding more "simple" requirements there? Like for integers, emails, ... My question is about what we want to have in this Requirement class.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 28, 2022 •
edited
Loading

Also some date formats would be nice as they're common in URLs.

@tarlepp
Copy link
Contributor

tarlepp commented Apr 1, 2022

I think that

#[Route(path: '/users/{id}', requirements: ['id' => Requirement::UUID])]

is not enough, I think that there should be also patterns for specific versions of UUID and not just generic one.

@jmsche
Copy link
Contributor

jmsche commented Apr 1, 2022

What about a slug pattern?

@nicolas-grekas
Copy link
Member

It should be possible to validate the version of a UUID using a regexp yes.

@nicolas-grekas
Copy link
Member

See https://stackoverflow.com/questions/136505/searching-for-uuids-in-text-with-regex which gives a stricter regexp even for generic UUIDs.

@fancyweb fancyweb force-pushed the routing/requirement-c branch 2 times, most recently from a612206 to 5571b8c Compare April 13, 2022 12:41
Nyholm
Nyholm approved these changes Apr 13, 2022
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Wow, what a great PR.

If you add \d+ then it would cover all needs that I have in my application.

At some few scenarios, I've used .* if I wanted to catch everything including slashes (/). Ie "redirect to legacy app controller".

EDIT: Use .+ would make more sense.
.+ - Catch everything
.* - Catch everything including empty string

@kbond
Copy link
Member

kbond commented Apr 13, 2022 •
edited
Loading

At some few scenarios, I've used .* if I wanted to catch everything including slashes (/). Ie "redirect to legacy app controller".

Agree, I use this quite a lot but as .+ - not sure if .* is the better choice. Having this as a Requirement would remove that dilemma :)

Lots of questions on stackoverflow about this.

@fancyweb fancyweb force-pushed the routing/requirement-c branch from 5571b8c to 7a230c6 Compare April 13, 2022 13:20
@fabpot
Copy link
Member

fabpot commented Apr 14, 2022

Tests looks broken

@fancyweb
Copy link
Contributor Author

fancyweb commented Apr 14, 2022 •
edited
Loading

Tests looks broken

I'll add some tests for every regex, do you think we have reached the first iteration of the set that will be merged?

@fabpot
Copy link
Member

fabpot commented Apr 14, 2022

Looks like a good first set.

@fancyweb fancyweb force-pushed the routing/requirement-c branch 2 times, most recently from 269192f to 73af741 Compare April 15, 2022 09:07
@fancyweb
[Routing] Add Requirement, a collection of universal regular-expressi…
f49cb6a 
…on constants to use as route parameter requirements
@fancyweb fancyweb force-pushed the routing/requirement-c branch from 73af741 to f49cb6a Compare April 15, 2022 09:09
@fabpot
Copy link
Member

fabpot commented Apr 15, 2022

Thank you @fancyweb.

@fabpot fabpot mentioned this pull request Apr 15, 2022
Closed
@fabpot fabpot merged commit 2a38810 into symfony:6.1 Apr 15, 2022
@fancyweb fancyweb deleted the routing/requirement-c branch April 15, 2022 10:39
@fabpot fabpot mentioned this pull request Apr 27, 2022
Merged
@HeahDude HeahDude mentioned this pull request May 21, 2022
Merged
nicolas-grekas added a commit that referenced this pull request May 27, 2022
@nicolas-grekas
feature #46430 [Routing] Add Requirement::POSITIVE_INT for common i…
a10071b 
…ds and pagination (HeahDude)

This PR was merged into the 6.2 branch.

Discussion
----------

[Routing] Add `Requirement::POSITIVE_INT` for common ids and pagination

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #45528
| License       | MIT
| Doc PR        | ~

I targeted 6.1 because it's not yet released, should I rebase onto 6.2?

ping `@fancyweb` :)

Commits
-------

bea54e6 [Routing] Add `Requirement::POSITIVE_INT` for common ids and pagination
@jokaorgua
Copy link

jokaorgua commented May 30, 2022 •
edited
Loading

what about adding an Email regexp? I understand that not all emails can be covered by regexp but some basic ones could be created I think.

https://stackoverflow.com/a/201378/11815081 - pretty good explanation about validating an email by regexp

@nicwortel nicwortel mentioned this pull request Jun 29, 2022
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jul 4, 2022
@javiereguiluz
minor #16926 [Routing] Document the Requirement constants in routing.…
2baf371 
…rst (nicwortel)

This PR was merged into the 6.1 branch.

Discussion
----------

[Routing] Document the Requirement constants in routing.rst

Fixes #16718

symfony/symfony#45528 by `@fancyweb` introduced the `Symfony\Component\Routing\Requirement\Requirement` enum in Symfony 6.1, as a solution for symfony/symfony#26524.

It was blogged about in https://symfony.com/blog/new-in-symfony-6-1-improved-routing-requirements-and-utf-8-parameters, but I couldn't find it anywhere in the Routing documentation.

This PR adds a small hint about the use of the `Requirement` enum for common route requirements:

![The Requirement enum contains a collection of commonly used regular-expression constants such as digits, dates and UUIDs which can be used as route parameter requirements.](https://user-images.githubusercontent.com/1055691/176497123-24b9bf6e-1b79-4385-baee-0eb132ae8944.png)

I'm open for suggestions to further improve this. I decided to use a small info block instead without code examples, because it doesn't seem to be a big feature and I'm not sure how well it is supported across all configuration formats. Let me know if you would like to see some code examples.

Commits
-------

571647d Document the Requirement constants in routing.rst
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz left review comments

@kbond kbond kbond left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@GromNaN GromNaN GromNaN left review comments

@OskarStark OskarStark OskarStark left review comments

@fabpot fabpot fabpot approved these changes

@Nyholm Nyholm Nyholm approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Feature Routing Status: Reviewed
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.