8000 [OptionsResolver] performing validation before filters · Issue #4500 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[OptionsResolver] performing validation before filters #4500

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
Closed
sstok opened this issue Jun 5, 2012 · 9 comments · Fixed by #11501
Closed

[OptionsResolver] performing validation before filters #4500

sstok opened this issue Jun 5, 2012 · 9 comments · Fixed by #11501
Labels
OptionsResolver

Comments

@sstok
Copy link
Contributor
sstok commented Jun 5, 2012

Currently the OptionsResolver performs filtering before validating.
Which is kind of weird.

When I set an value that can only be an string or null the filtering is performed before the actual type validating.
So I still need to check in my filtering function whether the input value is accepted and throw an exception (with not to much information as that is not available), second when my filtering function returns something else like an object I get an error because type does not match.

Should it be better to perform type validation before performing filtering or add an different method for adding validations that will be performed bore filtering?
@webmozart
Copy link
Contributor

Hm very good point. I will look into changing this.

@sstok
Copy link
Contributor Author
sstok commented Jan 14, 2013

@bschussek ping

@wouterj wouterj mentioned this issue Jul 28, 2014
Merged
@jakzal jakzal added the hasPR label Jul 29, 2014
@sstok
Copy link
Contributor Author
sstok commented Aug 14, 2014

A better description for this issue.

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.

@wouterj
Copy link
Member
wouterj commented Aug 14, 2014

@jakzal the hasPR label should be removed from this issue, my PR was not related.

@jakzal jakzal removed the hasPR label Aug 14, 2014
@jakzal
Copy link
Contributor
jakzal commented Aug 14, 2014

@wouterj done, boss 👍

@stof stof mentioned this issue Aug 19, 2014
Closed
@kix kix mentioned this issue Aug 20, 2014
Closed
fabpot added a commit that referenced this issue 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
Copy link
Member
xabbuh commented Aug 31, 2014

@fabpot I think that this has to reopened. GitHub cannot recognise @wouterj's correction in #11501.

@fabpot fabpot reopened this Aug 31, 2014
@webmozart
Copy link
Contributor

I'll do this once #11716 is merged.

@webmozart webmozart mentioned this issue Oct 6, 2014
Merged
4 tasks
@webmozart
Copy link
Contributor

See #12156 for a PR.

webmozart added a commit that referenced this issue Oct 22, 2014
@webmozart
bug #12156 [OptionsResolver] Merged Options class into OptionsResolve…
8838143 
…r (webmozart)

This PR was merged into the 2.6-dev branch.

Discussion
----------

[OptionsResolver] Merged Options class into OptionsResolver

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #4500, #9174, #10585, #10202, #11020, makes #7979+#10616 obsolete
| License       | MIT
| Doc PR        | symfony/symfony-docs#4159

#11716 was reverted as preparation of this PR (453882c).

The Options class was merged into OptionsResolver. This made it possible to fix several tickets, as indicated above.

**Usage**

See [the updated documentation](https://github.com/webmozart/symfony-docs/blob/issue11705/components/options_resolver.rst).

**Bug Fixes**

Previously, the options weren't validated before the normalizers were called. As a result, the normalizers had to perform validation again:

```php
$resolver->setAllowedTypes(array(
    'choices' => 'array',
));
$resolver->setNormalizers(array(
    'choices' => function (Options $options, $value) {
         array_merge($options['choices'], ...);
    },
));

// fatal error
$resolver->resolve(array('choices' => 'foobar'));
```

This is fixed now.

**BC Breaks**

The `array` type hint was removed from `setRequired()`, `setAllowedValues()`, `addAllowedValues()`, `setAllowedTypes()` and `addAllowedTypes()`. If anybody implemented `OptionsResolverInterface`, they must adapt their code.

The Options class was turned into an interface extending ArrayAccess and Countable. Anybody instantiating Options directly should instantiate OptionsResolver instead. Anybody using any of the methods available in Options (`get()`, `has()`) should use the ArrayAccess interface instead.

Normalizers are not called anymore for undefined options (#9174). People need to set a default value if they want a normalizer to be executed independent of the options passed to `resolve()`.

**Deprecations**

OptionsResolverInterface was deprecated and will be removed in 3.0. OptionsResolver instances are not supposed to be shared between classes, hence an interf
81CE
ace does not make sense.

Several other methods were deprecated. See the CHANGELOG and UPGRADE-2.6 files for information.

**Todo**

- [x] Fix PHPDoc
- [x] Adapt CHANGELOG/UPGRADE
- [x] Adapt documentation
- [x] Deprecate OptionsResolver[Interface]

Commits
-------

642c119 [OptionsResolver] Merged Options class into OptionsResolver
@webmozart
Copy link
Contributor

Fixed in #12156.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
OptionsResolver
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[OptionsResolver] Changed order of validation wouterj/symfony
6 participants
@fabpot @webmozart @jakzal @wouterj @sstok @xabbuh
0