Skip to content
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

Remove missed sm.factory usages #15858

Merged
merged 14 commits into from
Feb 23, 2024

Conversation

Wojdylak
Copy link
Member

@Wojdylak Wojdylak commented Feb 19, 2024

Q A
Branch? 1.13
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets #15738
License MIT

The apply method in the sm.callback.cascade_transition class (\SM\Callback\CascadeTransitionCallback) has a $soft parameter, which defaults to true. When $soft is set to true, the method first checks if the transition can be applied without throwing an exception. So I have added checking to symfony workflow events.

@Wojdylak Wojdylak requested a review from a team as a code owner February 19, 2024 10:03
@probot-autolabeler probot-autolabeler bot added API APIs related issues and PRs. Shop ShopBundle related issues and PRs. labels Feb 19, 2024
Copy link

github-actions bot commented Feb 19, 2024

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

@Wojdylak Wojdylak force-pushed the remove-missed-sm-factory-usages branch from edb70ff to b1daaae Compare February 19, 2024 14:11
diimpp
diimpp previously approved these changes Feb 22, 2024
$this->getStateMachine()->apply($review, ProductReviewTransitions::GRAPH, $transition);
$stateMachine = $this->getStateMachine();

if (false === $stateMachine->can($review, ProductReviewTransitions::GRAPH, $transition)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it better this way instead of just using a simple try-catch block?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the try-catch block catches all exceptions from the Symfony workflow, and the method can only checks if a transition is possible. In the try-catch block, we always receive the same exception, whereas in the second case, we receive different exceptions if the method apply throws an exception.

@NoResponseMate NoResponseMate merged commit bb0d574 into Sylius:1.13 Feb 23, 2024
28 checks passed
@NoResponseMate
Copy link
Contributor

Thank you, @Wojdylak!

@Wojdylak Wojdylak deleted the remove-missed-sm-factory-usages branch February 23, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs. Shop ShopBundle related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants