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] 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 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:4.2fromdbu:fix-callback-validation
Apr 8, 2019

Conversation

@dbu
Copy link
Contributor

@dbudbu commentedApr 7, 2019

QA
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 ignoringnull as callback
Deprecations?no
Tests pass?yes
Fixed ticketsRelated to#30888
LicenseMIT
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 tonormalize()

Copy link
ContributorAuthor

@dbudbu 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:

@jewome62jewome62force-pushed thefix-callback-validation branch fromcf6cfee to071b51eCompareApril 7, 2019 12:23
@jewome62jewome62force-pushed thefix-callback-validation branch fromeac3831 to6363306CompareApril 7, 2019 13:18
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 7, 2019
edited
Loading

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

Copy link
Contributor

@maxheliasmaxhelias 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

@dbudbu requested a review fromxabbuh as acode ownerApril 8, 2019 10:07
@dbudbuforce-pushed thefix-callback-validation branch 2 times, most recently from7f78a0e tode5a875CompareApril 8, 2019 10:56
@dbudbuforce-pushed thefix-callback-validation branch fromde5a875 to5d15669CompareApril 8, 2019 11:03
@dbu
Copy link
ContributorAuthor

dbu commentedApr 8, 2019

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

Copy link
Member

@nicolas-grekasnicolas-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)

@dbudbuforce-pushed thefix-callback-validation branch 2 times, most recently from4b8061b to502bf35CompareApril 8, 2019 11:25
@dbudbuforce-pushed thefix-callback-validation branch from502bf35 to3789152CompareApril 8, 2019 13:10
@dbu
Copy link
ContributorAuthor

dbu commentedApr 8, 2019

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

@fabpot
Copy link
Member

Thank you@dbu.

@fabpotfabpot merged commit3789152 intosymfony:4.2Apr 8, 2019
fabpot added a commit that referenced this pull requestApr 8, 2019
…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
@fabpotfabpot mentioned this pull requestApr 16, 2019
fabpot added a commit that referenced this pull requestApr 27, 2019
This PR was squashed before being merged into the 4.3-dev branch (closes#30888).Discussion----------[serializer] extract normalizer tests to traitseufossa| 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 possibleStuff 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhAwaiting requested review from xabbuh

+2 more reviewers

@jewome62jewome62jewome62 approved these changes

@maxheliasmaxheliasmaxhelias approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

[8]ページ先頭

©2009-2025 Movatter.jp