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

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

Merged
merged 1 commit into from Apr 15, 2022

Conversation

Copy link
Contributor

@fancyweb fancyweb commented Feb 23, 2022

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 Feb 23, 2022
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 23, 2022

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

@fancyweb fancyweb force-pushed the routing/requirement-c branch 2 times, most recently from 98ade84 to 019c4d3 Compare Feb 23, 2022
@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 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 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 Feb 24, 2022
@fabpot
Copy link
Member

@fabpot 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 nicolas-grekas commented Mar 28, 2022

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

@tarlepp
Copy link
Contributor

@tarlepp 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 jmsche commented Apr 1, 2022

What about a slug pattern?

@pbowyer
Copy link
Contributor

@pbowyer pbowyer commented Apr 1, 2022

To complement the slug pattern, there's <<id>>-<<slug for seo only>> and <<slug for seo only>>-<<id>> which are both popular. I use the latter, as does Medium and other big sites. Example:

https://timjwise.medium.com/will-smith-chris-rock-and-what-youre-missing-about-that-slap-3ad83f924c82

Copy link
Member

@GromNaN GromNaN left a comment

The validator component contains some regex that can be useful in URL matching. Is it worth adding them?

public const PATTERN = '/^(?<year>\d{4})-(?<month>\d{2})-(?<day>\d{2})$/';

public const PATTERN = '/^(\d{2}):(\d{2}):(\d{2})$/';

public const VALIDATION_PATTERN = '/[A-Z]{2}[A-Z0-9]{9}[0-9]{1}/';

src/Symfony/Component/Routing/Tests/RequirementTest.php Outdated Show resolved Hide resolved
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Thank you all for the complementary proposals. What about merging as is and iterate by sending PRs?

@fabpot
Copy link
Member

@fabpot fabpot commented Apr 12, 2022

I'd rather wait for more requirements to be implemented. As is, this is of limited value. Announcing a well-thought feature is better IMHO.

@fancyweb
Copy link
Contributor Author

@fancyweb fancyweb commented Apr 12, 2022

I moved the enum to the Requirement namespace since EnumRequirement was merged 😃

Do we envision adding more "simple" requirements there? Like for integers, emails

Not sure adding simple cases like DIGIT = '\d+' bring value?
For emails, we would reuse the 2 patterns from the Validator component?

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

Y-m-d and H:i:s?

I think that there should be also patterns for specific versions of UUID

We can add several regexes but can we fully validate all versions just with a regex? I'm not sure, @nicolas-grekas?

What about a slug pattern?

Slugs can be very different depending on the application IMHO but we can add one very generic case like [a-zA-Z0-9-]+ but does it really bring value?

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Apr 12, 2022

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

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Apr 12, 2022

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 Apr 13, 2022
Copy link
Member

@Nyholm Nyholm left a comment

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 kbond commented Apr 13, 2022

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.

@fabpot
Copy link
Member

@fabpot fabpot commented Apr 14, 2022

Tests looks broken

@fancyweb
Copy link
Contributor Author

@fancyweb fancyweb commented Apr 14, 2022

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 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 Apr 15, 2022
…on constants to use as route parameter requirements
@fabpot
Copy link
Member

@fabpot fabpot commented Apr 15, 2022

Thank you @fancyweb.

@fabpot fabpot merged commit 2a38810 into symfony:6.1 Apr 15, 2022
8 of 9 checks passed
@fancyweb fancyweb deleted the routing/requirement-c branch Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet