Skip to content

[OptionsResolver] Allow giving a callback as an allowedValue to OptionsResolver#8375

Merged
fabpot merged 1 commit intosymfony:masterfrom
marekkalnik:allow-callback-to-validate-option
Jan 7, 2014
Merged

[OptionsResolver] Allow giving a callback as an allowedValue to OptionsResolver#8375
fabpot merged 1 commit intosymfony:masterfrom
marekkalnik:allow-callback-to-validate-option

Conversation

@marekkalnik
Copy link
Copy Markdown
Contributor

I recently had to use an option which was an array and could contain some one or multiple values from a list. As it could contain all possible combinations, it was not possible to validate it with a list of allowed values.

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets --
License MIT
Doc PR symfony/symfony-docs#3437

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are not checking if $options[$option] is set

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and you can use if instead of elseif a the previous if throws an exception

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @stof, it's fixed. I should stop making PR late at night... :/

@shouze
Copy link
Copy Markdown

shouze commented Aug 2, 2013

good idea, 👍

@tyx
Copy link
Copy Markdown
Contributor

tyx commented Aug 2, 2013

need ! 👍

@Remy-Luciani
Copy link
Copy Markdown

👍 nice. :)

@stloyd
Copy link
Copy Markdown
Contributor

stloyd commented Aug 8, 2013

👍

1 similar comment
@fabriceb
Copy link
Copy Markdown

fabriceb commented Aug 8, 2013

👍

@shouze
Copy link
Copy Markdown

shouze commented Aug 8, 2013

Perhaps it would be useful to also support regular expressions. Your opinion?
Can be done in another PR.

@marekkalnik
Copy link
Copy Markdown
Contributor Author

@shouze - actually I start to think that it would be nice to use validation constraints with it. This would couple the two components though. As for regexp - you can use a callback, I don't see much of a gain in direct support.

@lfalorni
Copy link
Copy Markdown

lfalorni commented Aug 8, 2013

👍

1 similar comment
@popofr13
Copy link
Copy Markdown

popofr13 commented Aug 8, 2013

👍

@shouze
Copy link
Copy Markdown

shouze commented Aug 8, 2013

@marekkalnik you're right.

In fact we've seen your PR few days ago while we were looking for an elegant solution to validate query string params for a REST api (and avoid some cache poisoning).

We know that we could use Form validation but it looked a bit over bloated for a such simple thing.
We finally adopted a solution based on the treeBuilder because OptionResolver does not support nested options #4833

BTW OptionsResolver must remain simple.

@CharlyP
Copy link
Copy Markdown

CharlyP commented Aug 8, 2013

Great feature ! 👍

@sstok
Copy link
Copy Markdown
Contributor

sstok commented Aug 9, 2013

💯 👍

Regex and constraints should be possible with the closure, so adding that would over-complicate things.

@jakzal
Copy link
Copy Markdown
Contributor

jakzal commented Dec 22, 2013

👍

@cordoval
Copy link
Copy Markdown
Contributor

the feature has gotten some feedback so i think the docs would be nice 👍

👶

@marekkalnik
Copy link
Copy Markdown
Contributor Author

Ok, I'll write some by the end of the week.

@marekkalnik
Copy link
Copy Markdown
Contributor Author

I added the doc, sory that it's late, it wasn't easy with all the family holidays :(

But it made me think - it won't work with the addAllowedValue, so maybe I should write a separate method for specyfying a callback? What do you think?

@fabpot
Copy link
Copy Markdown
Member

fabpot commented Jan 7, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have said: The option "%s" has the value "%s", which it is not valid

@marekkalnik
Copy link
Copy Markdown
Contributor Author

@fabpot I fixed the CS error and changed the message

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong as you have 2 %s but only one var

@marekkalnik
Copy link
Copy Markdown
Contributor Author

@fabpot it's better now

@fabpot
Copy link
Copy Markdown
Member

fabpot commented Jan 7, 2014

Thank you @marekkalnik.

fabpot added a commit that referenced this pull request Jan 7, 2014
…Value to OptionsResolver (marekkalnik)

This PR was merged into the 2.5-dev branch.

Discussion
----------

[OptionsResolver] Allow giving a callback as an allowedValue to OptionsResolver

I recently had to use an option which was an array and could contain some one or multiple values from a list. As it could contain all possible combinations, it was not possible to validate it with a list of allowed values.

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | --
| License       | MIT
| Doc PR        | symfony/symfony-docs#3437

Commits
-------

07d1d30 Allow giving a callback as an allowedValue to OptionsResolver
@fabpot fabpot closed this Jan 7, 2014
@fabpot fabpot merged commit 07d1d30 into symfony:master Jan 7, 2014
weaverryan added a commit to symfony/symfony-docs that referenced this pull request Jan 21, 2014
This PR was merged into the master branch.

Discussion
----------

Add info about callback in options resolver

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes (symfony/symfony#8375)
| Applies to    | 2.5+
| Fixed tickets |

The documentation for the PR (pending) about validation callbacks in OptionsResolver.

Commits
-------

8231230 Fix according to PR comments
94fe8dc Add info about callback in options resolver
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.