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

[Validator] Docblock about throwing a validator Exception #29832

Open
wants to merge 5 commits into
base: 3.4
from

Conversation

Projects
None yet
4 participants
@gmponos
Copy link
Contributor

gmponos commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Member

xabbuh commented Jan 10, 2019

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.

@@ -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

This comment has been minimized.

@ro0NL

ro0NL Jan 10, 2019

Contributor

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

  • NoSuchMetadataException
  • MissingOptionsException

I'd prefer the expected ones here:

  • UnexpectedTypeException
  • UnexpectedValueException
  • ConstraintDefinitionException

This comment has been minimized.

@gmponos

gmponos Jan 10, 2019

Contributor

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

This comment has been minimized.

@ro0NL

ro0NL Jan 10, 2019

Contributor

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)

This comment has been minimized.

@gmponos

gmponos Jan 10, 2019

Contributor

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...

This comment has been minimized.

@ro0NL

ro0NL Jan 10, 2019

Contributor

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.

This comment has been minimized.

@gmponos

gmponos Jan 10, 2019

Contributor

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

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Contributor

gmponos commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Contributor

ro0NL commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Contributor

gmponos commented Jan 10, 2019

@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

This comment has been minimized.

Copy link
Contributor

gmponos commented Jan 14, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment