8000 [OptionsResolver] Support union of types by VincentLanglet · Pull Request #59354 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[OptionsResolver] Support union of types #59354

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
Jan 6, 2025
Merged

[OptionsResolver] Support union of types #59354

merged 1 commit into from
Jan 6, 2025

Conversation

VincentLanglet
Copy link
Contributor
@VincentLanglet VincentLanglet commented Jan 3, 2025 •
edited
Loading
Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #...
License MIT

Add support for | syntax when describing allowedType in OptionResolver.

It's not really useful for int|string since we can pass an array ['string', 'int'].
But it's useful for array values since (int|string)[] was not possible so far and ['string[]', 'int[]'] was not the same thing.

@carsonbot carsonbot added this to the 7.3 milestone Jan 3, 2025
@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.3" but it seems your PR description refers to branch "7.3 for features".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

yceruto
yceruto requested changes Jan 3, 2025
View reviewed changes
Copy link
Member
@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

Good addition 👍 thanks!

@yceruto
Copy link
Member
yceruto commented Jan 3, 2025

Also, we could find a regex that matches all possible variants instead of checking them one by one.

@VincentLanglet
Copy link
Contributor Author
VincentLanglet commented Jan 3, 2025 •
edited
Loading

Also, we could find a regex that matches all possible variants instead of checking them one by one.

Didn't find an easy one, but I'm opened to suggestion.

I preferred to add the logic in the existing recursive implementation

@VincentLanglet VincentLanglet requested a review from yceruto January 3, 2025 16:19
@VincentLanglet
Copy link
Contributor Author

I found edge case where my previous implementation remove badly parenthesis, so I now introduce a private method isEnclosedWithParenthesis @yceruto

@yceruto
Copy link
Member
yceruto commented Jan 3, 2025

Could you confirm that class Union is also supported with this new syntax? i.e. (App\FooInterface|App\BarInterface)[]

@VincentLanglet
Copy link
Contributor Author

Could you confirm that class Union is also supported with this new syntax? i.e. (App\FooInterface|App\BarInterface)[]

I added a test for \DateTime::class.'|'.\DateTimeImmutable::class. Is it enough ?

@VincentLanglet VincentLanglet requested a review from yceruto January 6, 2025 10:42
@fabpot
Copy link
Member
fabpot commented Jan 6, 2025

Thank you @VincentLanglet.

@fabpot fabpot merged commit cd24b4b into symfony:7.3 Jan 6, 2025
9 of 11 checks passed
@fabpot fabpot mentioned this pull request Jan 6, 2025
Closed
@alamirault alamirault mentioned this pull request Jan 12, 2025
Merged
fabpot added a commit that referenced this pull request Jan 12, 2025
@fabpot
minor #59483 [OptionsResolver] Add missing support of union type in c…
0d0758e 
…hangelog (alamirault)

This PR was merged into the 7.3 branch.

Discussion
----------

[OptionsResolver] Add missing support of union type in changelog

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT

I found changelog is missing while documenting symfony/symfony-docs#20531

Code PR: #59354

Commits
-------

946278f [OptionsResolver] Add missing support of union type
@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

@fabpot fabpot fabpot approved these changes

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

@yceruto yceruto 57BB yceruto approved these changes

Assignees
No one assigned
Labels
Feature OptionsResolver Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
5 participants
@VincentLanglet @carsonbot @yceruto @fabpot @nicolas-grekas
0