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 MissingConstructorArgumentsException returning missing argument one by one#49832

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

Conversation

@ktherage
Copy link
Contributor

@ktheragektherage commentedMar 27, 2023•
edited
Loading

QA
Branch?6.3
Bug fix?yes
New feature?no
Deprecations?no
Tickets-
LicenseMIT
Doc PR-

This PR is an improvment of#49013 andfix#42712.

She aims to fix the problem reported in#49013 indicating that the methodgetMissingConstructorArguments() of\Symfony\Component\Serializer\Exception\MissingConstructorArgumentsException class introduced in#42712 is returning only one missing arguments when there's more than one missing.

@nicolas-grekas
Copy link
Member

/cc@HypeMC FYI

ktherage reacted with thumbs up emoji

@ktherage
Copy link
ContributorAuthor

/cc@BafS also

@HypeMC
Copy link
Member

The original goal of my PR was to add the class as an argument to the exception, this reverts that as well. While I like the idea of having all the arguments at once, I don't see why the part with the class being passed needs to be removed.

@ktherage
Copy link
ContributorAuthor

@HypeMC

The original goal of my PR was to add the class as an argument to the exception, this reverts that as well.

Well, sorry, I missed that part I'll check that.

HypeMC reacted with heart emoji

@ktheragektherageforce-pushed thefix-missing-constructor-arguments-exception branch fromada843a to5dc7445CompareMarch 27, 2023 16:57
@ktherage
Copy link
ContributorAuthor

ktherage commentedMar 27, 2023•
edited
Loading

I have a failing test locally and I think that it's fine as it requires now a constructor parameter that is missing but I'm not sure if I might fix it (ie I actually broke a thing).

It's at src/Symfony/Component/Serializer/Tests/SerializerTest.php:764

details :

1) Symfony\Component\Serializer\Tests\SerializerTest::testUnionTypeDeserializableWithoutAllowedExtraAttributesSymfony\Component\Serializer\Exception\MissingConstructorArgumentsException: Cannot create an instance of "Symfony\Component\Serializer\Tests\DummyCTypeForUnion" from serialized data because its constructor requires the following parameters to be present : "c"./home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php:408/home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php:244/home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php:323/home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Serializer.php:227/home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php:537/home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php:601/home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php:370/home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php:244/home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php:323/home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Serializer.php:227/home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Serializer.php:145/home/ktherage/Documents/forks/symfony/src/Symfony/Component/Serializer/Tests/SerializerTest.php:764

@ktheragektherageforce-pushed thefix-missing-constructor-arguments-exception branch from371daec to8998701CompareMarch 28, 2023 09:46
@ktheragektherageforce-pushed thefix-missing-constructor-arguments-exception branch from1ff6b4a to0f3622aCompareApril 21, 2023 07:32
@nicolas-grekasnicolas-grekasforce-pushed thefix-missing-constructor-arguments-exception branch frome1b891c to43d028dCompareApril 21, 2023 15:29
@nicolas-grekas
Copy link
Member

Thank you@ktherage.

ktherage reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas merged commitb8e918d intosymfony:6.3Apr 21, 2023
@ktheragektherage deleted the fix-missing-constructor-arguments-exception branchApril 26, 2023 07:09
nicolas-grekas added a commit that referenced this pull requestOct 17, 2023
… argument (HypeMC)This PR was squashed before being merged into the 5.4 branch.Discussion----------[Serializer] Fix collecting only first missing constructor argument| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MITAlternative to#51866, sort of followup to#49832Currently on 5.4 only the first exception is added to the `not_normalizable_value_exceptions` array when `COLLECT_DENORMALIZATION_ERRORS` is `true` or only the first argument is mentioned in the `MissingConstructorArgumentsException` when it is `false`.On 6.3 however, the part with the `MissingConstructorArgumentsException` was fix with#49832, but the part with the `not_normalizable_value_exceptions` was overlooked.IMO this is inconsistent behavior as the two cases are actually the same thing with the only difference being that in one case an exception is thrown while in the other the errors are collected.I'm not sure if#51866 really qualifies as a bug or is actually more a feature, but the reason#49832 was merged onto 6.3 was because of the changes originally done in#49013, which itself was a feature.If#51866 does qualify as a bug then it would make sense to backport#49832 to 5.4 for consistency, which is what this PR does.The PR contains two commits:1) backport of#498322) alternative to#51866If#51866 does not qualify as a bug, the first commit can be drooped and the second one can be rebased with 6.4.PS I think it's easier to review the changes commit by commit.Commits-------0f398ce [Serializer] Fix collecting only first missing constructor argument
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@HypeMCHypeMCHypeMC left review comments

@mtarldmtarldmtarld left review comments

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

+1 more reviewer

@WebMambaWebMambaWebMamba left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

6 participants

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