Skip to content

[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.

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
Apr 8, 2019
Merged

[Serializer] Also validate callbacks when given in the normalizer context #30950

merged 1 commit into from
Apr 8, 2019

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Apr 7, 2019

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()

@xabbuh xabbuh added this to the 4.2 milestone Apr 7, 2019
@dbu dbu mentioned this pull request Apr 7, 2019
Merged
5 tasks
@jewome62 jewome62 force-pushed the fix-callback-validation branch 2 times, most recently from 8a67f0c to cf6cfee Compare April 7, 2019 11:39
dbu
dbu commented Apr 7, 2019
View reviewed changes
Copy link
Contributor Author

@dbu dbu left a 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:

@jewome62 jewome62 force-pushed the fix-callback-validation branch from cf6cfee to 071b51e Compare April 7, 2019 12:23
@jewome62 jewome62 force-pushed the fix-callback-validation branch from eac3831 to 6363306 Compare April 7, 2019 13:18
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 7, 2019 •
edited
Loading

Sorry I started commenting using regular comments, here is one more to thank you for this PR :)

maxhelias
maxhelias approved these changes Apr 7, 2019
View reviewed changes
Copy link
Contributor

@maxhelias maxhelias left a 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

@dbu dbu requested a review from xabbuh as a code owner April 8, 2019 10:07
@dbu dbu force-pushed the fix-callback-validation branch 2 times, most recently from 7f78a0e to de5a875 Compare April 8, 2019 10:56
@dbu dbu force-pushed the fix-callback-validation branch from de5a875 to 5d15669 Compare April 8, 2019 11:03
@dbu
Copy link
Contributor Author

dbu commented Apr 8, 2019

thanks. i improved the exception messages and cleaned up the max_depth_handler to only validate when we need, and to refuse null

nicolas-grekas
nicolas-grekas approved these changes Apr 8, 2019
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.

(once the typo is fixed)

@dbu dbu force-pushed the fix-callback-validation branch 2 times, most recently from 4b8061b to 502bf35 Compare April 8, 2019 11:25
@dbu dbu force-pushed the fix-callback-validation branch from 502bf35 to 3789152 Compare April 8, 2019 13:10
@dbu
Copy link
Contributor Author

dbu commented Apr 8, 2019

typo fixed. i rebased on 4.2 because there where (unrelated) failing tests.

@fabpot
Copy link
Member

fabpot commented Apr 8, 2019

Thank you @dbu.

@fabpot fabpot merged commit 3789152 into symfony:4.2 Apr 8, 2019
fabpot added a commit that referenced this pull request Apr 8, 2019
@fabpot
bug #30950 [Serializer] Also validate callbacks when given in the nor…
08f24b0 
…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
@fabpot fabpot mentioned this pull request Apr 16, 2019
Merged
fabpot added a commit that referenced this pull request Apr 27, 2019
@fabpot
minor #30888 [serializer] extract normalizer tests to traits (dbu)
016425e 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jewome62 jewome62 jewome62 approved these changes

@fabpot fabpot fabpot approved these changes

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

@maxhelias maxhelias maxhelias approved these changes

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Bug Serializer Status: Reviewed
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
7 participants
@dbu @nicolas-grekas @fabpot @jewome62 @maxhelias @xabbuh @carsonbot