Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Serializer] Fix usingDateIntervalNormalizer with union types#51992

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

Merged
fabpot merged 1 commit intosymfony:5.4fromJeroeny:fixdateinterval
Oct 25, 2023

Conversation

@Jeroeny
Copy link
Contributor

@JeroenyJeroeny commentedOct 11, 2023•
edited
Loading

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
LicenseMIT

The union logic ofAbstractObjectNormalizer tries to denormalize each union type and catchesNotNormalizableValueException (among some other exceptions). If a non-catching exception is thrown, denormalization fails on that first type, while a later type might have succeeded.

If I try to denormalize aDateTimeInterface value into aDateInterval|DateTimeInterface type, it will fail because theDateIntervalNormalizer throwsUnexpectedValueException instead ofNotNormalizableValueException.
Denormalizing aDateInterval intoDateTimeInterface|DateInterval does work, becauseDateTimeNormalizer throwsNotNormalizableValueException. I also checked some other Object-specific normalizers likeUid,Problem,DateTimeZone,DataUri,BackedEnum, they are usingNotNormalizableValueException.

See reproducer:https://github.com/Jeroeny/reproduce/tree/union/src

@carsonbotcarsonbot added this to the5.4 milestoneOct 11, 2023
@OskarStarkOskarStark changed the title[Serializer] Fix using DateIntervalNormalizer with union types[Serializer] Fix usingDateIntervalNormalizer with union typesOct 11, 2023
@nicolas-grekas
Copy link
Member

Could it be possible to add a test case that covers the situation you describe, ie that uses a union type?

@Jeroeny
Copy link
ContributorAuthor

Jeroeny commentedOct 12, 2023•
edited
Loading

@nicolas-grekas I think this tests the union type with a failure first:https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/Serializer/Tests/SerializerTest.php#L740

But it won't testevery normalizer for throwing the correct exception.

And secondly theDateIntervalNormalizerTest now does assert the exception is thrown for this normalizer.

@nicolas-grekas
Copy link
Member

/cc@HypeMC maybe for a review? 🙏

@HypeMC
Copy link
Member

@nicolas-grekas Sure, I'll take a look sometime today.

Copy link
Member

@HypeMCHypeMC left a comment

Choose a reason for hiding this comment

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

Looks good, tried it out and it works as expected.

@fabpot
Copy link
Member

Thank you@Jeroeny.

@fabpotfabpot merged commit3f92492 intosymfony:5.4Oct 25, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@HypeMCHypeMCHypeMC approved these changes

@mtarldmtarldmtarld approved these changes

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

7 participants

@Jeroeny@nicolas-grekas@HypeMC@fabpot@stof@mtarld@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp