8000 [Workflow] Questions and ideas · Issue #20774 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Workflow] Questions and ideas #20774

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.

Sign up for GitHub

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

Jump to bottom
Closed
Padam87 opened this issue Dec 6, 2016 · 8 comments
Closed

[Workflow] Questions and ideas #20774

Padam87 opened this issue Dec 6, 2016 · 8 comments
Labels
Feature Workflow

Comments

@Padam87
Copy link
Contributor
Padam87 commented Dec 6, 2016 •
edited
Loading

Hi,

I decided to give the workflow component a try, and I have a couple of questions and suggestions.

graph

I have started out with this workflow. I used to do these type of jobs with https://github.com/yohang/Finite, but the new component looks awesome :)

Events

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Workflow/Workflow.php#L129

The marking store is updated only after the enter event has been fired. This is not convenient, since I can't flush the entity manager in the listener.
The announce event is for the next transition as I gather, so listening to that for a flush is not the way to go.
All in all, I have to add $em->flush() in my controller, outside my other logic related to that event to store the markings.

Could we add an entered event to the mix ? :)

Transition parameters

It would be useful to have the ability to pass parameters to transitions. In my past projects with https://github.com/yohang/Finite, parameters were a necessity sooner or later. I can't provide my exact use cases, but in the example above the start_shipping method could have a tracking_number parameter. I could of course set it in the controller, but then half of the transition logic will be in my listener, and half in the controller. eg

if ($workflow->can($order, 'start_shipping')) {
    $order->setTrackingNumber('ASD123');
    $workflow->apply($order, 'start_shipping');
    $em->flush()
}
public function onStartShipping(Event $event)
{
    /** @var Order $order */
    $order = $event->getSubject();

    $this->orderMailer->send(/* ... */) // The mail contains the tracking code
}

With finite, I used the OptionsResolver in my listeners to validate the passed options. In finite, we have also added a built in resolver. (This may be a bit of overkill, especially because of the yaml configuration.)

I would like to submit a PR enabling this:

if ($workflow->can($order, 'start_shipping')) {
    $workflow->apply($order, 'start_shipping', ['tracking_number' => 'ASD123']);
}

Would that be okay?

One workflow, or multiple state machines?

The example could be implemented with 2 state machines. (orderStatus, paymentStatus and guards to validate)

In the documentation I read this:

It is also worth noting that a workflow does not commonly have cyclic path in the definition graph, but it is common for a state machine.

Would you recommend using a multi state workflow if there are cyclic paths (about 4-5) in a project?

Thanks!
This was referenced Dec 6, 2016
Closed
Closed
Closed
@lyrixx
Copy link
Member
lyrixx commented Dec 6, 2016

Event

The announce event is for the next transition as I gather, so listening to that for a flush is not the way to go.
All in all, I have to add $em->flush() in my controller, outside my other logic related to that event to store the markings.

Could we add an entered event to the mix ? :)

From my point of view, it's a best practice to flush in the controller and note in a listener. If you flush in the listener, your model could be corrupted. For example, if you plan to send an email after a transition, nothing can guarantee you the email was send. But it's too late because you already save into the DB the new marking.

@lyrixx
Copy link
Member
lyrixx commented Dec 6, 2016

Transition parameters

I'm not sure about this use case. You are mixing the concern here. I mean: You are using the event as a data bag and IMHO this is not the right place do it.

In your example, the tracking number should be a property of the Order. With the new implementation, the tracking number is volatile, and IMHO, it's not a good idea.

@Padam87
Copy link
Contributor Author
Padam87 commented Dec 6, 2016

Check the issues in finite... many people had problems with these there.

@lyrixx
Copy link
Member
lyrixx commented Dec 6, 2016

One workflow, or multiple state machines?

In this example, and after a very quick read of it, I would use a workflow and not a state machine to let the Engine decide if a transition can be enabled or not.

@lyrixx
Copy link
Member
lyrixx commented Dec 6, 2016 •
edited
Loading

@Padam87 Having only big issue for many questions is really not easy :/

@Padam87
Copy link
Contributor Author
Padam87 commented Dec 6, 2016

Yep, that was a bad idea :)

I still think all of these are valid features, parameters come in handy in the real world, even if it does not make sense in an ideal word.

One time I had to pass parameters from an API call result, and on time from an rabbit RPC request.

@lyrixx
Copy link
Member
lyrixx commented Dec 13, 2016

@Padam87 Could we close this issue. I think I answered all your questions. Moreover there are related PR open or closed. Feel free to re-open new issue (but with only one topic ;) ) if needed.

@Padam87
Copy link
Contributor Author

Yes, can be closed, thanks!

@Padam87 Padam87 closed this as completed Dec 13, 2016
lyrixx added a commit that referenced this issue Feb 13, 2017
@lyrixx
feature #20787 [Workflow] Added an entered event (Padam87)
772d8e6 
This PR was submitted for the 3.2 branch but it was merged into the 3.3-dev branch instead (closes #20787).

Discussion
----------

[Workflow] Added an entered event

| Q             | A
| ------------- | ---
| Branch?       |  3.2
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20774 (partially) ; #21433
| License       | MIT
| Doc PR        | -

Commits
-------

7a562bd [Workflow] Added an entered event
@Padam87 Padam87 mentioned this issue Jul 23, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature Workflow
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@javiereguiluz @lyrixx @Padam87
0