Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
AllValidator should not throw when not traversable is passed#26477
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Destroy666x commentedMar 12, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
What's the use case of this change? The reason of returning when value is empty is that there's EDIT: oh, the use case is in the issue, I see your point. But I don't agree with your approach of loose variables that store simple stuff like E-mails as you've shown in the example. Maybe a better example that's not easily achievable with type hinting would change my mind. Also, regardless of that it requires a change to a lot of validators for consistency as you mentioned. And it's definitely not a bug. |
ogizanagi commentedMar 12, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I understand everyone's points here, but I really think requiring to use group sequences to workaround this is really bad DX for such a simple case (so I'd be in favor of removing this exception). So instead, shouldn't we just add a violation? Anyway, thanks@zerkms for opening this issue & PR. I had this in my todo list since few months but haven't taken the time to report it yet. |
zerkms commentedMar 12, 2018
@Destroy666x that's trivial: implement an API that accepts a JSON object with a user account:
None of 1-4 items can be validated reliably (without the using the groups in a weird way) |
zerkms commentedMar 12, 2018
@ogizanagi Currently it would also pass validation if you pass an empty value (or don't pass it at all) unless you have a In either case - I personally think the validator should never throw: literally any arbitrary input should either return 0 or N violation errors. |
ogizanagi commentedMar 12, 2018
True. I'm just saying this implementation might be dangerous for existing applications not having proper |
zerkms commentedMar 12, 2018
In that case I'm glad I've raised an awareness :-) Do we close this? |
ogizanagi commentedMar 12, 2018
Let's wait for more feedback. No need to close anyway if you're still up to provide a solution regarding the original issue, even if the implementation may differ. |
Destroy666x commentedMar 13, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@zerkms why not? You just deserialize it properly into a class and pass corresponding properties to proper validators. I've validated a lot of such JSONs with no problem. I surely agree with |
zerkms commentedMar 13, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
for the very same reason I reported an issue: if a type mismatches - an exception is thrown. So you don't have a viloations list, just an exception. Try to create the validator that validates those 4 fields and validate this: |
ostrolucky commentedApr 1, 2018
Another possible solution is to deprecate this constraint in favor of new Each constraint, as was planned long time ago#9888 This new constraint can be made to not throw exception |
zerkms commentedApr 2, 2018
@ostrolucky the problem is much deeper: a lot of validators throw. It must be fixed everywhere. |
…ing (ogizanagi)This PR was squashed before being merged into the 4.1-dev branch (closes #26945).Discussion----------[Messenger] Support configuring messages when dispatching| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | see CI checks <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | todoFor now, this is mainly a RFC to get your opinion on such a feature (so it misses proper tests).Supporting wrapped messages out-of-the-box would mainly allow to easily create middlewares that should act only or be configured differently for specific messages (by using wrappers instead of making messages implements specific interfaces and without requiring dedicated buses). For instance, I might want to track specific messages and expose metrics about it.The Symfony Serializer transport (relates to #26900) and Validator middleware serve as guinea pigs for sample purpose here. But despite message validation usually is simple as it matches one use-case and won't require specific validation groups in most cases, I still think it can be useful sometimes. Also there is the case of the [`AllValidator` which currently requires using a `GroupSequence`](symfony/symfony#26477) as a workaround, but that could also be specified directly in message metadata instead.## Latest updatesPR updated to use a flat version of configurations instead of wrappers, using an `Envelope` wrapper and a list of envelope items.Usage:```php$message = Envelope::wrap(new DummyMessage('Hello')) ->with(new SerializerConfiguration(array(ObjectNormalizer::GROUPS => array('foo')))) ->with(new ValidationConfiguration(array('foo', 'bar')));```~~By default, Messenger protagonists receive the original message. But each of them can opt-in to receive the envelope instead, by implementing `EnvelopeReaderInterface`.Then, they can read configurations from it and forward the original message or the envelope to another party.~~Senders, encoders/decoders & receivers always get an `Envelope`.Middlewares & handlers always get a message. But middlewares can opt-in to receive the envelope instead, by implementing `EnvelopeReaderInterface`.Commits-------749054a1e8 [Messenger] Support configuring messages when dispatching
…ing (ogizanagi)This PR was squashed before being merged into the 4.1-dev branch (closes#26945).Discussion----------[Messenger] Support configuring messages when dispatching| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | see CI checks <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | todoFor now, this is mainly a RFC to get your opinion on such a feature (so it misses proper tests).Supporting wrapped messages out-of-the-box would mainly allow to easily create middlewares that should act only or be configured differently for specific messages (by using wrappers instead of making messages implements specific interfaces and without requiring dedicated buses). For instance, I might want to track specific messages and expose metrics about it.The Symfony Serializer transport (relates to#26900) and Validator middleware serve as guinea pigs for sample purpose here. But despite message validation usually is simple as it matches one use-case and won't require specific validation groups in most cases, I still think it can be useful sometimes. Also there is the case of the [`AllValidator` which currently requires using a `GroupSequence`](#26477) as a workaround, but that could also be specified directly in message metadata instead.## Latest updatesPR updated to use a flat version of configurations instead of wrappers, using an `Envelope` wrapper and a list of envelope items.Usage:```php$message = Envelope::wrap(new DummyMessage('Hello')) ->with(new SerializerConfiguration(array(ObjectNormalizer::GROUPS => array('foo')))) ->with(new ValidationConfiguration(array('foo', 'bar')));```~~By default, Messenger protagonists receive the original message. But each of them can opt-in to receive the envelope instead, by implementing `EnvelopeReaderInterface`.Then, they can read configurations from it and forward the original message or the envelope to another party.~~Senders, encoders/decoders & receivers always get an `Envelope`.Middlewares & handlers always get a message. But middlewares can opt-in to receive the envelope instead, by implementing `EnvelopeReaderInterface`.Commits-------749054a [Messenger] Support configuring messages when dispatching
ogizanagi commentedOct 24, 2018
…ing (ogizanagi)This PR was squashed before being merged into the 4.1-dev branch (closes #26945).Discussion----------[Messenger] Support configuring messages when dispatching| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | see CI checks <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | todoFor now, this is mainly a RFC to get your opinion on such a feature (so it misses proper tests).Supporting wrapped messages out-of-the-box would mainly allow to easily create middlewares that should act only or be configured differently for specific messages (by using wrappers instead of making messages implements specific interfaces and without requiring dedicated buses). For instance, I might want to track specific messages and expose metrics about it.The Symfony Serializer transport (relates to #26900) and Validator middleware serve as guinea pigs for sample purpose here. But despite message validation usually is simple as it matches one use-case and won't require specific validation groups in most cases, I still think it can be useful sometimes. Also there is the case of the [`AllValidator` which currently requires using a `GroupSequence`](symfony/symfony#26477) as a workaround, but that could also be specified directly in message metadata instead.## Latest updatesPR updated to use a flat version of configurations instead of wrappers, using an `Envelope` wrapper and a list of envelope items.Usage:```php$message = Envelope::wrap(new DummyMessage('Hello')) ->with(new SerializerConfiguration(array(ObjectNormalizer::GROUPS => array('foo')))) ->with(new ValidationConfiguration(array('foo', 'bar')));```~~By default, Messenger protagonists receive the original message. But each of them can opt-in to receive the envelope instead, by implementing `EnvelopeReaderInterface`.Then, they can read configurations from it and forward the original message or the envelope to another party.~~Senders, encoders/decoders & receivers always get an `Envelope`.Middlewares & handlers always get a message. But middlewares can opt-in to receive the envelope instead, by implementing `EnvelopeReaderInterface`.Commits-------749054a1e8 [Messenger] Support configuring messages when dispatching
Uh oh!
There was an error while loading.Please reload this page.
The same way
AllValidatordoes not throw for empty values, and the same way every other validator does not throw but skips or returns a violation instead - theAllValidatorshould returnnullin case if not an array or a traversable passed.