NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Routing] Add EnumRequirement to help generate route requirements from a \BackedEnum#45803

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:6.1fromfancyweb:routing/backed-enum-requirement

Apr 12, 2022

Merged

[Routing] Add EnumRequirement to help generate route requirements from a \BackedEnum#45803

fabpot merged 1 commit intosymfony:6.1fromfancyweb:routing/backed-enum-requirement

Apr 12, 2022

Conversation

Copy link

Contributor

fancyweb commentedMar 21, 2022•
edited by nicolas-grekas
Loading

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

Ref#44831

I'd like to limit a route parameter allowed values to the backed values of an enum to use it in conjunction with the new\BackedEnum argument resolver (ie fail from the start).
Also, sometimes, I'd like to limit it only to a subset of the backed values.
I couldn't find a way to do that because enums can't implement__toString() and accessing->value is not considered a constant operation.
We can leverage the fact that route requirements can be a\Stringable.

Before (no enum):

#[Route(path:'/foo/{bar}', requirements: ['bar' => FooEnum::AAA.'|'.FooEnum::BBB])]

Allow all enum cases:

#[Route(path:'/foo/{bar}', requirements: ['bar' =>newEnumRequirement(Foo::class)])]

Allow a subset:

#[Route(path:'/foo/{bar}', requirements: ['bar' =>newEnumRequirement(Foo::class, Foo::Aaa, Foo::Bbb)])]

Probably not the best solution but I hope we can find something for that use case for 6.1 😄

cc@ogizanagi

carsonbot added Status: Needs Review Feature Routing labels

Mar 21, 2022

carsonbot added this to the6.1 milestone

Mar 21, 2022

Copy link

carsonbot commentedMar 22, 2022

Hey!

I think@usu has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Copy link

Contributor

ogizanagi commentedMar 22, 2022

Note: the alternative mentioned on Slack was to use a PHP 8 attribute to configure the\BackedEnum argument resolver instead, but perhaps not the best idea, since it'll be slightly less performant and wouldn't allow multiple routes with different requirements on contrary of this suggestion.

Copy link

Member

nicolas-grekas commentedMar 22, 2022•
edited
Loading

If we really want to support subsets, we should give the subset of "what".
Wouldn't this be a cleaner signature?
BackedEnumRequirement(class-string<T> $enum, ...BackedEnum<T> $cases)

Honestly, I find this quite ugly... but 🤷

Copy link

Member

nicolas-grekas commentedMar 28, 2022

What about renaming BackedEnumRequirement to EnumRequirement? I feel like adding "Backed" doesn't provide anything.

Copy link

ContributorAuthor

fancyweb commentedMar 31, 2022

\UnitEnum enums are not supported.EnumRequirement could imply the class supports both\BackedEnum and\UnitEnum.

I'll change the signature toBackedEnumRequirement(class-string<T> $enum, ...BackedEnum<T> $cases) and throw if a $case is not part of $enum.

Copy link

Member

nicolas-grekas commentedMar 31, 2022

EnumRequirement could imply the class supports both \BackedEnum and \UnitEnum

It cannot since it will throw once ppl give it a try - and it's a logical error to expect that a pure enum could work as a part of matching a URL, so ppl that could have this expectation are wrong anyway. "BackedEnum" just adds to the ugliness of using enums to me...

fancyweb force-pushed therouting/backed-enum-requirement branch 2 times, most recently fromcb68e74 to275d111Compare

March 31, 2022 13:04

[Routing] Add EnumRequirement to help generate route requirements fro…

ce87606

…m a \BackedEnum

fancyweb force-pushed therouting/backed-enum-requirement branch from275d111 toce87606Compare

March 31, 2022 13:04

fancyweb changed the title~~[Routing] Add BackedEnumRequirement to help generate route requirements from a \BackedEnum~~[Routing] Add EnumRequirement to help generate route requirements from a \BackedEnum

Mar 31, 2022