Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Workflow] implemented TransitionException to be thrown instead of Logic exception#26587
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
| /** | ||
| * @author Andrew Tch <andrew.tchircoff@gmail.com> | ||
| */ | ||
| class TransitionException extends \LogicException implements 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.
could it extend LogicException from the Workflow namespace? this would prevent a BC break
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.
Oh, so sorry - that was intended to have implement it to avoid BC. Fixed, added a simple test case also.
| /** | ||
| * @author Andrew Tch <andrew.tchircoff@gmail.com> | ||
| */ | ||
| class TransitionException extends LogicException implements 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.
implements ExceptionInterface (since already implemented byLogicException now :)
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. Sorry, my coding style is chaotic somehow )
lyrixx commentedMar 19, 2018
Hello@andrewtch Thanks for your PR but unfortunately, It's already implemented in#26076 |
andrewtch commentedMar 19, 2018
@lyrixx , I don't see how I can understand what transition actually caused the exception. Your's UndefinedTransictionException (https://github.com/symfony/symfony/pull/26076/files#diff-edae21a7fe491af4f1bfd5519f01d722) still falls back to simple string. |
andrewtch commentedMar 19, 2018
Can we merge this together to provide a trusted way to differentiate exceptions? |
lyrixx commentedMar 19, 2018
Ok, we will do that ;) |
xabbuh commentedMar 21, 2018
@andrewtch Can you resolve the conflicts here? |
lyrixx commentedMar 21, 2018
@andrewtch If you miss some time to finish this PR before the feature freeze, I can finish it for you. Don't hesitate to tell me. |
lyrixx commentedMar 23, 2018
Thanks@andrewtch for working on this feature. |
…ixx)This PR was merged into the 4.1-dev branch.Discussion----------[Workflow] Added a TransitionException| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#26587#26581| License | MIT| Doc PR |---Note for reviewer: `NotEnabledTransitionException` and `UndefinedTransitionException` were not released. So this is not a BC Break.Commits-------838548c Finished implementation of TransitionException9fa4f79 implemented TransitionException to be thrown instead of Logic exception
| /** | ||
| * @return string | ||
| */ | ||
| public function getTransitionName() |
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.
we could use a return type hint here
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.
You commented a closed pr :-)
| /** | ||
| * @return string | ||
| */ | ||
| public function getWorkflowName() |
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.
same here
Getting Logic exception from Workflow is somehow confusing - as there is no way to understand which transition triggered the exception, and there's no access to the subject being modified to format the exception in any other way.
Therefore, I've implemented TransitionException to be thrown at runtime (extending current LogicException to avoid BC).
getMessagesignature remains the same. All other exceptions are untouched (as they are tied to very logic of the workflow, and to to the subject).See#26581