[Validator] Docblock about throwing a validator Exception #29832

gmponos · 2019-01-10T08:49:01Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT

Most of the validators if I remember correctly can throw a ValidatorException. This makes the docblock in line with that.

xabbuh · 2019-01-10T08:59:34Z

Exceptions should only be thrown by the validators if the developer made some mistake in their configuration and those exceptions should not be caught. Thus I would not document them here.

ro0NL · 2019-01-10T09:06:32Z

src/Symfony/Component/Validator/ConstraintValidatorInterface.php

@@ -30,6 +31,8 @@ public function initialize(ExecutionContextInterface $context);
     *
     * @param mixed      $value      The value that should be validated
     * @param Constraint $constraint The constraint for the validation
+     *
+     * @throws ValidatorException


yet some implementations are tied to the constraint or metadata factory even..

NoSuchMetadataException

MissingOptionsException

I'd prefer the expected ones here:

UnexpectedTypeException

UnexpectedValueException

ConstraintDefinitionException

What happens in cases that I have a custom validator that throws a custom exception extending the ValidatorException.

Also I don't get what is the issue with NoSuchMetadataException or MissingOptionsException since they both extend ValidatorException

constraint validators never throw such an exception, at least not in core.

We cant just add ANY exception the user might throw, only the expected ones (which the high level validator deals with)

We cant just add ANY exception

What I have added here is not ANY exception.. It is a specific exception that ANY exception implemented by a developer it must extend it...

ok put different :) i think ConstraintValidatorInterface::validate() is designed to throw specific exceptions, for specific cases. Not just any ValidatorException.. esp. since the high level validator checks specific ones, e.g. UnexpectedValueException

Throwing ValidatorException instead of UnexpectedValueException would be a wrong implementation. As such i think we should document the specific cases.

Now the argument is understandable.. although I disagree :)

ro0NL · 2019-01-10T09:15:54Z

Exceptions should only be thrown by the validators if the developer made some mistake in their configuration

To comply with the API of a constraint validator, implementors should at least know about the Unexpected... exceptions.

gmponos · 2019-01-10T09:32:48Z

Exceptions should only be thrown by the validators if the developer made some mistake in their configuration and those exceptions should not be caught.

Why not.. ?? I might have a controller that initializes my custom validator and I need to customize the message shown to the consumer of the controller by catching the exception...

ro0NL · 2019-01-10T17:22:32Z

I suggest something like

@throws UnexpectedTypeException If the constraint is incompatible
@throws UnexpectedTypeException If the value is incompatible (causes an unrecoverable error)
@throws UnexpectedValueException If the value is incompatible (causes a recoverable violation)
@throws ConstraintDefintionException If a constraint option is incompatible during runtime
@throws ValidatorException If any other validation related error occured

im :+-0: on adding ValidatorException specifically; i've no real proof it was designed like that. It seems reasonable, though in practice it could be a plain \RuntimeException as well. Which is why im skeptical about adding it now.

I might have a controller that initializes my custom validator and I need to customize the message shown to the consumer of the controller by catching the exception...

In this case, i think you could throw a custom exception as well, and document it on your custom constraint validator.

gmponos · 2019-01-10T17:51:08Z

@ro0NL I saw that you added above also this

@throws ValidatorException If any other validation related error occured

I can agree with that then.. I thought that in your review comment you you wanted to remove the generic one and add only the specific ones...

gmponos · 2019-01-14T21:02:13Z

I made some changes according to @ro0NL but... I haven't added the UnexpectedValueException. This exception exists on master and I am targeting 3.4

nicolas-grekas

Not extra convinced, but no objections either :)

…lizer (gmponos) This PR was squashed before being merged into the 3.4 branch (closes #29889). Discussion ---------- [Serializer] Docblock about throwing exceptions on serializer | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT Most of the serializers can throw a `\Symfony\Component\Serializer\ExceptionInterface`. This makes the docblock in line with that. Similar to this #29832 Commits ------- 0b44ad7 [Serializer] Docblock about throwing exceptions on serializer

…lizer (gmponos) This PR was squashed before being merged into the 3.4 branch (closes #29889). Discussion ---------- [Serializer] Docblock about throwing exceptions on serializer | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT Most of the serializers can throw a `\Symfony\Component\Serializer\ExceptionInterface`. This makes the docblock in line with that. Similar to this symfony/symfony#29832 Commits ------- 0b44ad79c6 [Serializer] Docblock about throwing exceptions on serializer

src/Symfony/Component/Validator/ConstraintValidatorInterface.php

gmponos · 2019-02-26T12:52:33Z

Done. I left only one exception as discussed.

fabpot · 2019-02-27T08:21:15Z

Thank you @gmponos.

…n (gmponos) This PR was squashed before being merged into the 3.4 branch (closes #29832). Discussion ---------- [Validator] Docblock about throwing a validator Exception | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT Most of the validators if I remember correctly can throw a `ValidatorException`. This makes the docblock in line with that. Commits ------- 481622d [Validator] Docblock about throwing a validator Exception

carsonbot added Status: Needs Review Validator labels Jan 10, 2019

ro0NL reviewed Jan 10, 2019

View reviewed changes

gmponos mentioned this pull request Jan 14, 2019

[Serializer] Docblock about throwing exceptions on serializer #29889

Merged

nicolas-grekas approved these changes Jan 24, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 24, 2019

nicolas-grekas added this to the 3.4 milestone Jan 24, 2019

javiereguiluz approved these changes Feb 7, 2019

View reviewed changes

xabbuh reviewed Feb 8, 2019

View reviewed changes

src/Symfony/Component/Validator/ConstraintValidatorInterface.php Outdated Show resolved Hide resolved

xabbuh reviewed Feb 8, 2019

View reviewed changes

src/Symfony/Component/Validator/ConstraintValidatorInterface.php Outdated Show resolved Hide resolved

fabpot approved these changes Feb 27, 2019

View reviewed changes

[Validator] Docblock about throwing a validator Exception

481622d

fabpot force-pushed the patch-3 branch from 2412556 to 481622d Compare February 27, 2019 08:21

gmponos requested review from dunglas and lyrixx as code owners February 27, 2019 08:21

carsonbot added Status: Needs Review and removed Status: Reviewed labels Feb 27, 2019

fabpot merged commit 481622d into symfony:3.4 Feb 27, 2019

gmponos deleted the patch-3 branch February 27, 2019 09:41

Uh oh!

[Validator] Docblock about throwing a validator Exception #29832

[Validator] Docblock about throwing a validator Exception #29832

Uh oh!

Conversation

gmponos commented Jan 10, 2019

Uh oh!

xabbuh commented Jan 10, 2019

Uh oh!

ro0NL Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

gmponos Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

ro0NL Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

gmponos Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

gmponos Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

ro0NL commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmponos commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL commented Jan 10, 2019

Uh oh!

gmponos commented Jan 10, 2019

Uh oh!

gmponos commented Jan 14, 2019

Uh oh!

nicolas-grekas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gmponos commented Feb 26, 2019

Uh oh!

fabpot commented Feb 27, 2019

Uh oh!

Uh oh!

gmponos Jan 10, 2019 •

edited

Loading

ro0NL commented Jan 10, 2019 •

edited

Loading

gmponos commented Jan 10, 2019 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading