8000 [OptionsResolver] Changed order of validation by wouterj · Pull Request #11501 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[OptionsResolver] Changed order of validation #11501

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
Aug 31, 2014
Merged

[OptionsResolver] Changed order of validation #11501

merged 1 commit into from
Aug 31, 2014

Conversation

wouterj
Copy link
Member
@wouterj wouterj commented Jul 28, 2014
Q A
Bug fix? yes
New feature? yes
BC breaks? no (I don't think it causes breaks)
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR -

It's both a new feature and bug fix actually... I let @fabpot decide on this one.

@sstok can you please confirm if this fixes #4500 ? I couldn't fully follow
that ticket and then I discovered this error. If not, can you please add more
information to your ticket about the problems?

@stof
Copy link
Member
stof commented Aug 14, 2014

👍

@Tobion
Copy link
Contributor
Tobion commented Aug 14, 2014

@sstok is talking about "filtering". Not sure what he means. Filtering is not done. Either an exception is thrown or all options are returned. So I don't see the impact of this change here.

@sstok
Copy link
Contributor
sstok commented Aug 14, 2014

When I set which value-types are allowed they are validated after the normalizing process.
So the normalizer is receiving possible invalid values.

Second, the value returned by the normalizer is validated for correctness. Meaning that when I want to return something special the validation needs to be configured to accept the returned value.

So technically the normalizer should be run after the validating, instead of running it as first.
Or we can add an after-normalizer/transformer that is run after the validating and merging.

@Tobion
Copy link
Contributor
Tobion commented Aug 14, 2014
8000

So this change here has nothing to do with #4500
But we can still merge it because I think it makes sense to validate types before values.

@sstok
Copy link
Contributor
sstok commented Aug 14, 2014

Indeed. I have updated the original issue so it will get lost when merging this PR.

@wouterj
Copy link
Member Author
wouterj commented Aug 14, 2014

Ah, now your issue is clear to me, @sstok. thanks! I've removed it from the PR description.

@fabpot
Copy link
Member
fabpot commented Aug 28, 2014

👍 Looks like a bug fix to me.

@fabpot
Copy link
Member
fabpot commented Aug 31, 2014

To be on the safe side, I'm going to merge this one on master.

@fabpot
Copy link
Member
fabpot commented Aug 31, 2014

Thank you @wouterj.

@fabpot fabpot merged commit a4f208b into symfony:master Aug 31, 2014
fabpot added a commit that referenced this pull request Aug 31, 2014
@fabpot
feature #11501 [OptionsResolver] Changed order of validation (WouterJ)
0f59bca 
This PR was merged into the 2.6-dev branch.

Discussion
----------

[OptionsResolver] Changed order of validation

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no (I don't think it causes breaks)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | -

It's both a new feature and bug fix actually... I let @fabpot decide on this one.

<s>@sstok can you please confirm if this fixes #4500 ? I couldn't fully follow
that ticket and then I discovered this error. If not, can you please add more
information to your ticket about the problems?</s>

Commits
-------

a4f208b Changed order of validation
@xabbuh xabbuh mentioned this pull request Aug 31, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
OptionsResolver
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[OptionsResolver] performing validation before filters
6 participants
@wouterj @stof @Tobion @sstok @fabpot @jakzal
0