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

Use the new State Machine abstraction #15738

Merged
merged 13 commits into from Jan 31, 2024
Merged

Conversation

jakubtobiasz
Copy link
Member

Q A
Branch? 1.13
Bug fix? no
New feature? kinda
BC breaks? no
Deprecations? no
Related tickets based on the #15729
License MIT

@jakubtobiasz jakubtobiasz requested a review from a team as a code owner January 17, 2024 10:06
@probot-autolabeler probot-autolabeler bot added Admin AdminBundle related issues and PRs. API APIs related issues and PRs. Maintenance CI configurations, READMEs, releases, etc. labels Jan 17, 2024
Copy link

github-actions bot commented Jan 17, 2024

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

@jakubtobiasz jakubtobiasz force-pushed the SYL-3284 branch 7 times, most recently from f28391f to eebd7f8 Compare January 22, 2024 17:41
@jakubtobiasz jakubtobiasz force-pushed the SYL-3284 branch 3 times, most recently from 11421b5 to d4db541 Compare January 25, 2024 12:41
@jakubtobiasz jakubtobiasz force-pushed the SYL-3284 branch 2 times, most recently from 8c3fc7a to b1a1d7e Compare January 26, 2024 07:13
Wojdylak
Wojdylak previously approved these changes Jan 29, 2024
src/Sylius/Abstraction/StateMachine/config/services.xml Outdated Show resolved Hide resolved
/**
* @throws StateMachineExecutionException
*/
public function getTransitionFromState(object $subject, string $graphName, string $fromState): ?string
Copy link
Member

Choose a reason for hiding this comment

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

public before private 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

It's discussable. I've talked with Janek about it, and we agreed we should all talk about it. I'm in favor of leaving it as-is, and eventually change it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll discuss it further at a later date, either way, if we'd want to revert it and keep the public => protected => private order that'll be fixed along with adding an ecs rule.

Wojdylak
Wojdylak previously approved these changes Jan 30, 2024
@NoResponseMate NoResponseMate merged commit e688d18 into Sylius:1.13 Jan 31, 2024
28 checks passed
@NoResponseMate
Copy link
Contributor

Thank you, @jakubtobiasz!

NoResponseMate added a commit that referenced this pull request Feb 23, 2024
This PR was merged into the 1.13 branch.

Discussion
----------

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

Commits
-------
  Remove missed sm.factory usages
  [Behat] Replace sm.factory to sylius_abstraction.state_machine
  [Workflow] Add check to verify if a transaction can be applied
  [Behat] Change workflow exception message
  [Api] Add OrderCancellationFailedException class
  [Api] Add PaymentCompletionFailedException class
  [Api] Add ProductReviewAcceptanceFailedException class
  [Api] Add ProductReviewRejectionFailedException class
  [StateMachine] Organize the interfaces in CompositeStateMachine class
  [Api] Remove unnecessary code
  [StateMachine] Use WorkflowExceptionInterface instead of InvalidArgumentException
  [StateMachine] Refactor CompositeStateMachine to use inline commands
  [Api] Add semi-generic exception StateMachineTransitionFailedException
  [Core][Workflow] Rename names of test functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin AdminBundle related issues and PRs. API APIs related issues and PRs. Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants