Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Validator] add exception when intl component not found#28513
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
[Validator] add exception when intl component not found#28513
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ronfroy commentedSep 19, 2018
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| License | MIT |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good idea. Here are some comments.
| */ | ||
| publicfunctionvalidate($value,Constraint$constraint) | ||
| { | ||
| if (!class_exists('Intl')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be Intl::class instead.
| publicfunctionvalidate($value,Constraint$constraint) | ||
| { | ||
| if (!class_exists('Intl')) { | ||
| thrownewRuntimeException('symfony/intl is required to use the Country constraint'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing dot at end of message, also I'd suggest to be a little bit more explicit (same in other messages below):The "symfony/intl" component is required to use the Country constraint.
ronfroy commentedSep 19, 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.
@nicolas-grekas fixed |
| } | ||
| if (!class_exists(Intl::class)) { | ||
| thrownewRuntimeException('The "symfony/intl" component is required to use the Country constraint.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
hmm, shouldn't it be aLogicException instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i think not, this is not a logic issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
looking at the code, we use LogicException quite often in similar situations, so could be worth for consistency at least (there are a few similar cases using RuntimeException, but they're not the majority and could be worth fixing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would have usedRuntimeException too ... but if we useLogicException in similar situations, it's better to keep consistency.@ronfroy can you please update this? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
how can we useLogicException elsewhere if this PR introduces it as a new class?
For this case we use theRuntimeException already 👍
symfony/src/Symfony/Component/Validator/Constraints/ExpressionValidator.php
Lines 56 to 63 in645b016
| if (null ===$this->expressionLanguage) { | |
| if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) { | |
| thrownewRuntimeException('Unable to use expressions as the Symfony ExpressionLanguage component is not installed.'); | |
| } | |
| $this->expressionLanguage =newExpressionLanguage(); | |
| } | |
| return$this->expressionLanguage; |
| } | ||
| if (!class_exists(Intl::class)) { | ||
| thrownewRuntimeException('The "symfony/intl" component is required to use the Country constraint.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would have usedRuntimeException too ... but if we useLogicException in similar situations, it's better to keep consistency.@ronfroy can you please update this? Thanks!
ronfroy commentedSep 20, 2018
done |
nicolas-grekas commentedSep 21, 2018
can you update src/Symfony/Component/Validator/Constraints/BicValidator.php and src/Symfony/Component/Validator/Mapping/Factory/BlackHoleMetadataFactory.php to use the new exception also please? |
ronfroy commentedSep 21, 2018
@nicolas-grekas done |
nicolas-grekas commentedSep 21, 2018
and src/Symfony/Component/Validator/ValidatorBuilder.php |
ro0NL commentedSep 21, 2018
for consitency we should change Runtime to Logic in |
| { | ||
| /** | ||
| * @expectedException \LogicException | ||
| * @expectedException \Symfony\Component\Validator\Exception\LogicException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not sure about the trailing;
| if (null ===$annotationReader) { | ||
| if (!class_exists('Doctrine\Common\Annotations\AnnotationReader') || !class_exists('Doctrine\Common\Cache\ArrayCache')) { | ||
| thrownew\RuntimeException('Enabling annotation based constraint mapping requires the packages doctrine/annotations and doctrine/cache to be installed.'); | ||
| if (!class_exists(Doctrine\Common\Annotations\AnnotationReader::class) || !class_exists(Doctrine\Common\Cache\ArrayCache::class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be moved as "use" statements.
nicolas-grekas commentedSep 21, 2018
in another PR :) |
nicolas-grekas commentedSep 21, 2018
Thank you@ronfroy. |
…(ronfroy)This PR was squashed before being merged into the 4.2-dev branch (closes#28513).Discussion----------[Validator] add exception when intl component not found| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| License | MIT<!--Add some exception when the Intl component is not found but required on some constraints-->Commits-------b6f29f4 [Validator] add exception when intl component not found
…ro0NL)This PR was merged into the 4.2-dev branch.Discussion----------Favor LogicException for missing classes & functions| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes-ish| BC breaks? | no-ish <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets |#28513 (comment)| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features --><!--Write a short README entry for your feature/bugfix here (replace this comment block.)This will help people understand your PR and can be used as a start of the Doc PR.Additionally: - Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the master branch.-->Commits-------c762735 Favor LogicException for missing classes & functions
This PR was merged into the 4.2-dev branch.Discussion----------[Form] Check for Intl availibility| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Same as#28513 for the form componentCommits-------73c688c [Form] Check for Intl availibility