Skip to content

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

Merged
fabpot merged 1 commit intosymfony:6.1from
fancyweb: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.1from
fancyweb:routing/backed-enum-requirement

Conversation

@fancyweb
Copy link
Copy Markdown
Contributor

@fancyweb fancyweb commented Mar 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' => new EnumRequirement(Foo::class)])]

Allow a subset:

#[Route(path: '/foo/{bar}', requirements: ['bar' => new EnumRequirement(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 carsonbot added this to the 6.1 milestone Mar 21, 2022
@carsonbot
Copy link
Copy Markdown

carsonbot commented Mar 22, 2022

Hey!

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

Cheers!

Carsonbot

@ogizanagi
Copy link
Copy Markdown
Contributor

ogizanagi commented Mar 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.

@nicolas-grekas
Copy link
Copy Markdown
Member

nicolas-grekas commented Mar 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 🤷

@nicolas-grekas
Copy link
Copy Markdown
Member

nicolas-grekas commented Mar 28, 2022

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

@fancyweb
Copy link
Copy Markdown
Contributor Author

fancyweb commented Mar 31, 2022

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

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

@nicolas-grekas
Copy link
Copy Markdown
Member

nicolas-grekas commented Mar 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 fancyweb force-pushed the routing/backed-enum-requirement branch 2 times, most recently from cb68e74 to 275d111 Compare March 31, 2022 13:04
@fancyweb fancyweb force-pushed the routing/backed-enum-requirement branch from 275d111 to ce87606 Compare March 31, 2022 13:04
@fancyweb 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
@fabpot
Copy link
Copy Markdown
Member

fabpot commented Apr 12, 2022

Thank you @fancyweb.

@fabpot fabpot merged commit 9cbc853 into symfony:6.1 Apr 12, 2022
@fabpot fabpot mentioned this pull request Apr 12, 2022
Closed
@fancyweb fancyweb deleted the routing/backed-enum-requirement branch April 12, 2022 07:49
@fabpot fabpot mentioned this pull request Apr 15, 2022
Merged
@tarlepp
Copy link
Copy Markdown
Contributor

tarlepp commented Apr 19, 2022

Similar solution for #[IsGranted(SomeEnum::FOO)] would also be great.

@nicolas-grekas
Copy link
Copy Markdown
Member

nicolas-grekas commented Jun 21, 2022 •
edited
Loading

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

Similar solution for #[IsGranted(SomeEnum::FOO)] would also be great.

Both ways could be supported natively if php/php-src#8825 were available, reducing the boilerplate needed for common simple cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot approved these changes

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

@stof stof stof 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.

7 participants

@fancyweb @carsonbot @ogizanagi @nicolas-grekas @fabpot @tarlepp @stof