-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] Also validate callbacks when given in the normalizer context #30950
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8a67f0c
to
cf6cfee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for bringing back the is_array validation that i accidentally dropped.
i am confused by this thing:
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
Outdated
Show resolved
Hide resolved
cf6cfee
to
071b51e
Compare
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php
Outdated
Show resolved
Hide resolved
eac3831
to
6363306
Compare
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
Outdated
Show resolved
Hide resolved
Sorry I started commenting using regular comments, here is one more to thank you for this PR :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With @nicolas-grekas's comments
7f78a0e
to
de5a875
Compare
de5a875
to
5d15669
Compare
thanks. i improved the exception messages and cleaned up the max_depth_handler to only validate when we need, and to refuse |
src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(once the typo is fixed)
4b8061b
to
502bf35
Compare
…ler are actually callable
502bf35
to
3789152
Compare
typo fixed. i rebased on 4.2 because there where (unrelated) failing tests. |
Thank you @dbu. |
…malizer context (dbu) This PR was merged into the 4.2 branch. Discussion ---------- [Serializer] Also validate callbacks when given in the normalizer context | Q | A | ------------- | --- | Branch? | 4.2 (callbacks are handled differently in 3.4) | Bug fix? | yes | New feature? | no | BC breaks? | no (unless somebody relied on this bug ignoring `null` as callback | Deprecations? | no | Tests pass? | yes | Fixed tickets | Related to #30888 | License | MIT | Doc PR | - callbacks configuration for the normalizer is validated to be valid callbacks when using setCallbacks or using the callbacks field in the default options. however, it was not validated when using the callbacks field in a context passed to `normalize()` Commits ------- 3789152 [serializer] validate that the specified callbacks and max_depth_handler are actually callable
This PR was squashed before being merged into the 4.3-dev branch (closes #30888). Discussion ---------- [serializer] extract normalizer tests to traits eufossa | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | Relates to #30818 | License | MIT | Doc PR | - As discussed with @joelwurtz, extract normalizer functionality tests into traits to ensure consistent behaviour of all normalizers. * [x] Rebase when #30977, #30950 and #30907 are merged to master **blocker** * [x] Clean up order of trait inclusion and methods in the tests * [x] Clean up fixture classes of the traits. I started having one class named the same as the trait, where possible Stuff that we should do eventually, but can also do in separate pull requests, after this one has been merged: * [ ] Extract all features that we can (the existing normalizer tests should more or less only have the legacy tests in them, all functionality should be in trait) * [ ] Run test coverage and increase coverage so that we cover all important features and all relevant error cases. Commits ------- 2b6ebea [serializer] extract normalizer tests to traits
null
as callbackcallbacks configuration for the normalizer is validated to be valid callbacks when using setCallbacks or using the callbacks field in the default options. however, it was not validated when using the callbacks field in a context passed to
normalize()