Skip to content

[OptionsResolver] array of instance allowed type #15524

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

Closed
soullivaneuh opened this issue Aug 12, 2015 · 11 comments
Closed

[OptionsResolver] array of instance allowed type #15524

soullivaneuh opened this issue Aug 12, 2015 · 11 comments

Comments

@soullivaneuh
Copy link
Contributor

It could be nice to have possibility to make this kind of requirement:

->setAllowedTypes('attachments', '\Swift_Attachment[]')

Basically, for this case, an array of Swift_Attachment objects.

It could be a good beginning waiting #4833.

@stof
Copy link
Member

stof commented Aug 12, 2015

@soullivaneuh the issue with that is that it cannot be implemented easily (it requires some parsing of the string, and looping over the array rther than just checking the value).

There is a workaround though:

$resolver->setAllowedTypes('attachments', 'array')
    ->setAllowedValues('attachments', function (array $attachments) {
        // we already know it is an array as types are validated first

        foreach ($attachments as $attachment) {
            if (!$attachment instanceof \Swift_Attachment) {
                return false;
            }
        }

        return true;
    });

@soullivaneuh
Copy link
Contributor Author

Thanks for the workaround. I already did something similar but with a normalizer.

I let this issue open if somebody got some courage to handle it. ;-)

EVODelavega pushed a commit to EVODelavega/symfony that referenced this issue Sep 2, 2015
@EVODelavega
Copy link
Contributor

@stof I've sent a PR that would enable typed array support, I've added a couple of simple test cases ('\DateTime[]' and int[][]), both of which seem to work just fine

@soullivaneuh
Copy link
Contributor Author

@EVODelavega Maybe you can propose a PR. Not sure if it should be on 2.7 or 2.8.

@EVODelavega
Copy link
Contributor

@soullivaneuh I've taken 2.8 as a starting point, but I should imagine I can patch 2.7 quite easily anyway, so if the maintainers want, I can create a PR, or mail a patch for 2.7 all the same

@stof
Copy link
Member

stof commented Sep 2, 2015

@soullivaneuh new features cannot be added in maintenance branches. So there is no discussion about 2.7 or 2.8: it cannot be 2.7 (you could discuss it between 2.8 and 3.0, but we already have the answer for this: if it is BC it goes in 2.x)

@Tobion
Copy link
Contributor

Tobion commented Sep 2, 2015

Maybe this problem would also be solved with #4833, i.e. if you consider an array of something as an suboption with any key and it's corresponding type.

@stof
Copy link
Member

stof commented Sep 2, 2015

@Tobion I don't think it can. Suboptions work fine when you know the expected children and can configure validation for them. This case is for a list of similar nodes.
If you look at these concepts in the Config Definition component, this is the difference between an array node and a prototyped array node.

@Tobion
Copy link
Contributor

Tobion commented Sep 2, 2015

I know, but the same thing already is going on when not using suboptions. If you want to allow any option, you have to read the passed options and make them known to the resolver. The same would apply for suboptions. The question is only if we need some special handling considering these options would have an integer key.

@EVODelavega
Copy link
Contributor

@Tobion I thought about ways to implement what you're hinting at, but decided not to do that because that, IMO, is beyond the scope of allowed types. Pre-defining a specific structure, including keys existing or not, and the types of the values they hold is something best left to a custom validator, or in a normalizer.

On the other hand, you could create a wrapper class that defines the given structure in full, and just write $resolver->setAllowedTypes('foo', 'YourClass'); or, if my PR gets merged $resolver->setAllowedTypes('foo', 'YourClass[]');

@stof
Copy link
Member

stof commented Sep 2, 2015

@Tobion associative arrays don't mean it is a set of keys defined by the form type. The option could be expecting an arbitrary map.
And this cannot be determined automatically because there is not enough info in the value to know how it is expected to be used (and even less info when you know that the info is here to be validated, and may not even be from the expected format)

EVODelavega pushed a commit to EVODelavega/symfony that referenced this issue Dec 18, 2015
EVODelavega pushed a commit to EVODelavega/symfony that referenced this issue Dec 21, 2015
HeahDude added a commit to HeahDude/symfony that referenced this issue Mar 12, 2016
closes symfony#15524.

Set allowed types for many options or for a set of nested options.
HeahDude added a commit to HeahDude/symfony that referenced this issue Mar 12, 2016
closes symfony#15524.

Set allowed types for many options or for a set of nested options.
HeahDude added a commit to HeahDude/symfony that referenced this issue Mar 12, 2016
closes symfony#15524.

Set allowed types for many options or for a set of nested options.
HeahDude added a commit to HeahDude/symfony that referenced this issue Mar 12, 2016
closes symfony#15524.

Set allowed types for many options or for a set of nested options.
HeahDude added a commit to HeahDude/symfony that referenced this issue Mar 13, 2016
closes symfony#15524.

Set allowed types for many options or for a set of nested options.
HeahDude added a commit to HeahDude/symfony that referenced this issue Mar 13, 2016
closes symfony#15524.

Set allowed types for many options or for a set of nested options.
HeahDude added a commit to HeahDude/symfony that referenced this issue Mar 13, 2016
closes symfony#15524.

Set allowed types for many options or for a set of nested options.
HeahDude added a commit to HeahDude/symfony that referenced this issue Mar 14, 2016
closes symfony#15524.

Set allowed types for many options or for a set of nested
or extra options.

  * add `Options::NONE`
  * add `Options::ALL`
  * add `Options::DEFINED`
  * add `Options::NESTED`
  * add `Options::EXTRA`
  * add `OptionsResolver::allowedValuesForAll`
  * add `OptionsResolver::allowedTypesForAll`
  * add `OptionsResolver::resolveUndefined`
  * add `OptionsResolver::addAllowedValuesForAll()`
  * add `OptionsResolver::addAllowedTypesForAll()`
  * add `OptionsResolver::setPrototype()`
HeahDude added a commit to HeahDude/symfony that referenced this issue Mar 14, 2016
closes symfony#15524.

Set allowed types for many options or for a set of nested
or extra options.

  * add `Options::NONE`
  * add `Options::ALL`
  * add `Options::DEFINED`
  * add `Options::NESTED`
  * add `Options::EXTRA`
  * add `OptionsResolver::allowedValuesForAll`
  * add `OptionsResolver::allowedTypesForAll`
  * add `OptionsResolver::resolveUndefined`
  * add `OptionsResolver::addAllowedValuesForAll()`
  * add `OptionsResolver::addAllowedTypesForAll()`
  * add `OptionsResolver::setPrototype()`
fabpot added a commit that referenced this issue Oct 12, 2017
…pe (pierredup)

This PR was merged into the 3.4 branch.

Discussion
----------

[OptionsResolver] Support array of types in allowed type

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #17032, #15524
| License       | MIT
| Doc PR        | TBD

This replaces #17032 with a simpler approach to allow an array of types in the allowed types for the options resolver

Note: This implementation doesn't support nested values (I.E `int[][]`), but if there is a strong need for it, I'll add it in another PR

Commits
-------

d066a23 Support array of types in allowed type
@fabpot fabpot closed this as completed Oct 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants