8000 [Config] Allow using an enum FQCN with `EnumNode` by alexandre-daubois · Pull Request #57686 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Config] Allow using an enum FQCN with EnumNode #57686

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 a 8000 gree 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
Feb 26, 2025
Merged

[Config] Allow using an enum FQCN with EnumNode #57686

merged 1 commit into from
Feb 26, 2025

Conversation

alexandre-daubois
Copy link
Member
@alexandre-daubois alexandre-daubois commented Jul 9, 2024 •
edited by OskarStark
Loading
Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #57659
License MIT

This PR allows doing the following with backed enums:

$rootNode
    ->children()
        ->enumNode('fqcn_enum_node')->enumClass(BackedTestEnum::class)->end()

Then, you can pass enum cases as well as plain strings to the config, which will be automatically converted to an enum case with BackedEnum::tryFrom().

@carsonbot carsonbot added this to the 7.2 milestone Jul 9, 2024
@alexandre-daubois alexandre-daubois force-pushed the config-enum-node-class branch from a84a01b to 1b7d1e9 Compare July 9, 2024 14:34
@alexandre-daubois
Copy link
Member Author
alexandre-daubois commented Jul 9, 2024 •
edited
Loading

Thanks for the review! I didn't answer to each message individually to avoid spamming your notifications but I addressed everything

@alexandre-daubois alexandre-daubois force-pushed the config-enum-node-class branch 3 times, most recently from 6d25b9b to 7808738 Compare July 9, 2024 14:56
@Seldaek
Copy link
Member
Seldaek commented Jul 10, 2024

Great stuff overall, thank 8000 s for picking this up! :)

@alexandre-daubois alexandre-daubois force-pushed the config-enum-node-class branch 2 times, most recently from f1daab5 to ba522e8 Compare July 10, 2024 14:27
Seldaek
Seldaek approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member
@Seldaek Seldaek left a comment

Choose a reason for hiding this comment

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

LGTM now from an end user perspective, but the core team might have more feedback 😅

@Seldaek Seldaek mentioned this pull request Jul 10, 2024
Open
@Seldaek
Copy link
Member
Seldaek commented Jul 10, 2024

Ah an interesting problem came up here https://github.com/symfony/symfony/pull/57653/files#r1671760696 - what if the enum class is not defined? Should it make an empty value set then? Or error loudly but in a nicer way than just calling ::cases() would?

@alexandre-daubois
Copy link
Member Author
alexandre-daubois commented Jul 10, 2024 •
edited
Loading

In the constructor, I check the existence of the enum. If it doesn't exist, it will treat it as a simple string. I think it is enough, otherwise I'm affraid it will complexify the whole thing a lot.

Additionally, EnumNodeDefinition (roughly, when using ->enumNode(...)->enumFqcn(...)) will throw if the enum doesn't exist. Seems safe to me 🤔

I also think that people will use it with the ::class syntax rather than a raw string, which will make PHP complain if the enum doesn't exist as well

@Seldaek
Copy link
Member
Seldaek commented Jul 10, 2024

Yes it seems safe, but it doesn't solve the problem in the other PR :)

I also think that people will use it with the ::class syntax rather than a raw string, which will make PHP complain if the enum doesn't exist as well

That is not true unfortunately https://3v4l.org/ibsvJ

@alexandre-daubois
Copy link
Member Author
alexandre-daubois commented Jul 10, 2024 •
edited
Loading

Ah yes indeed... In fact, making an assertion that a string looks like an FQCN seems a bit brittle to me.

Another solution that could be considered is that, if only one value is passed to $values and it's a string, then we consider that it must always be the FQCN of an enum. Why not, although I feel like it's a bit of a hack when I write it.

@Seldaek
Copy link
Member
Seldaek commented Jul 10, 2024

Sorry you lost me there. You already have ->enumFqcn() so it is clear that what is passed in there must be a valid enum class name, and enum_exists will ensure that passing RandomNonExistantEnum::class will fail at runtime at least. I think let's leave this PR as is for now.

@alexandre-daubois
Copy link
Member Author

Ah yes I forgot a bit of context, sorry. Actually I was talking about the standalone use of EnumNode, without EnumNodeDefinition::enumFqcn().

But anyway, let's see what others think of the current state yes! 👍

@alexandre-daubois alexandre-daubois force-pushed the config-enum-node-class branch 2 times, most recently from 315c6b3 to 1f036a0 Compare September 3, 2024 19:55
@javiereguiluz javiereguiluz added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 3, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
derrabus
derrabus reviewed Jan 18, 2025
View reviewed changes
Copy link
Member
@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Let's get this over the finish line, shall we?

nicolas-grekas
nicolas-grekas reviewed Feb 11, 2025
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Minor things on my side

@alexandre-daubois alexandre-daubois force-pushed the config-enum-node-class branch 2 times, most recently from 95b3c59 to 6529b5a Compare February 11, 2025 12:25
@alexandre-daubois
Copy link
Member Author

Thank you for the review. I updated to have as many early return/throw as possible as well as improved the exception messages.

@fabpot
Copy link
Member
fabpot commented Feb 26, 2025

Thank you @alexandre-daubois.

@fabpot fabpot merged commit 6b02c77 into symfony:7.3 Feb 26, 2025
10 of 11 checks passed
@fabpot fabpot mentioned this pull request Feb 26, 2025
Closed
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus requested changes

@stof stof stof requested changes

@fabpot fabpot fabpot approved these changes

@Seldaek Seldaek Seldaek approved these changes

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

Assignees
No one assigned
Labels
Config Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

[Config] Support passing the value of backed enums in enumNode
8 participants
@alexandre-daubois @Seldaek @fabpot @nicolas-grekas @stof @derrabus @javiereguiluz @carsonbot
0