Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Favor LogicException for missing classes & functions#28536
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
ro0NL commentedSep 21, 2018
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes-ish |
| BC breaks? | no-ish |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #28513 (comment) |
| License | MIT |
| Doc PR | symfony/symfony-docs#... |
| if (Email::VALIDATION_MODE_STRICT ===$constraint->mode) { | ||
| if (!class_exists('\Egulias\EmailValidator\EmailValidator')) { | ||
| thrownewRuntimeException('Strict email validation requires egulias/email-validator ~1.2|~2.0'); | ||
| thrownewLogicException('Strict email validation requires egulias/email-validator ~1.2|~2.0'); |
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.
That's a BC break as LogicException does not extend RuntimeException. Not sure it is worth it.
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.
True, but nobody can rely on this exception being thrown because a package is missing, don't you think?
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.
True, but I don't see why Logic is better than Runtime. We are using Runtime for such exceptions on the Messenger component for instance and Logic on some other components.
nicolas-grekasSep 24, 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.
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.
To me, a logic exception means the code is broken (composer.json here): it's a programmer mistake. Runtime exception means some unexpected behavior was observed (e.g. broken connection, etc.)
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.
Works for me, but we should fixed this across the board then.
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.
Done.
| thrownewLogicException('To enable the locking feature you must install the symfony/lock component.'); | ||
| } | ||
| if (null !==$this->lock) { |
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.
theLogicException below looks odd :)
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.
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.
on 2.8 actually? and unrelated to the Lock component, I was too fast :)
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.
just curious if it should be Runtime.. though Logic might be valid if the developer should call in specific order.
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.
yep, that makes sense also, asking devs to take care and pointing their mistake otherwise with 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.
This method should not be called twice. IMHO, this is a developer's misconception and should be a LogicException
| /** | ||
| * @author Roland Franssen <franssen.roland@gmail.com> | ||
| */ | ||
| class LogicExceptionextends \LogicExceptionimplements ExceptionInterface |
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.
added to not downgrade from namespaced to root namespace exception
nicolas-grekas commentedSep 24, 2018
Thank you@ro0NL. |
…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 is a follow-up ofsymfony#28536
…nglet)This PR was merged into the 5.4 branch.Discussion----------[DomCrawler] Fix Crawler::filter throw phpdoc| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | not really (phpdoc-fix)| New feature? | no| Deprecations? | no| Tickets | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->The `Crawler::filter` method is calling```$converter = $this->createCssSelectorConverter();```which is throwing a `LogicException` since#28536So the phpdoc should be````@throws` \LogicException if the CssSelector Component is not available```and not````@throws` \RuntimeException if the CssSelector Component is not available```Commits-------ae19695 Fix Crawler::filter throw phpdoc