Use Finite State Machine #1426

winzou · 2014-04-24T08:05:44Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Fixed tickets	#1037
License	MIT
Doc PR	-

Placeholder PR for finite state machine future implementation in Sylius.

kayue · 2014-04-24T08:32:16Z

@winzou Awesome! Can't wait!

pjedrzejewski · 2014-04-24T09:35:45Z

@winzou Thank for starting this, does Finite support yml configuration? If no, I was thinking about implementing our own service for defining the state machine configuration, which could be easily overridden by the developer.

pjedrzejewski · 2014-04-24T09:36:24Z

We would just need a set of default transitions to be required. Used by Sylius internally.

winzou · 2014-04-24T10:36:32Z

@pjedrzejewski @kayue here is how it looks like for now.

Do you think of a better place / better way of loading finite machine configurations? It needs to be in the bundles as each bundle with states will depend on finite. I would even like to put it in components. What do you think?

This is very basic implementation. Next steps are:

Deploy this system everywhere in sylius
Enhance the way states are managed thanks to all events/listeners/callback we can do with finite
Enhance again with some transition logging, etc.

Btw this PR needs yohang/Finite#29 to be merged before passing tests :)

pjedrzejewski · 2014-04-24T11:06:18Z

src/Sylius/Bundle/PaymentBundle/Resources/config/finite.yml

@@ -0,0 +1,35 @@
+finite_finite:
+    sylius_payment:
+        class:         Sylius\Component\Payment\Model\PaymentInterface


Why interface here, instead of class parameter like for the order? :)

pjedrzejewski · 2014-04-24T11:07:48Z

Awesome work, I really like the flexibility of Finite.

kayue · 2014-04-24T11:30:42Z

src/Sylius/Bundle/CoreBundle/Resources/config/finite.yml

+            refunded:
+                type: final
+        transitions:
+            submit:


Should we call it create?

winzou · 2014-04-25T07:23:31Z

Guys, this Finite thing is just crazy, I see endless possibilities with it!
For now I just replaced setState with StateMachine->apply, and I won't go much further in this PR. But in next steps, we will be able to rework most of our events with Finite events, it will be so powerfull!

pjedrzejewski · 2014-04-25T07:36:35Z

src/Sylius/Bundle/CoreBundle/EventListener/InventoryUnitListener.php

-                throw new \InvalidArgumentException(sprintf('Unexpected inventory state "%s".', $state));
-        }
+        $this->factory->get($unit, 'sylius_inventory_unit')->apply($transitionName);
+        $this->factory->get($unit, 'sylius_shipping_unit')->apply($transitionName);


The interface name implemented by InventoryUnit is ShippingItemInterface, I think we should change sylius_shipping_unit to sylius_shipping_item, what do you think?

Also, maybe we should check if the given transition is available with the can method? Or we should leave it and allow the exception to occur?

Yes for sylius_shipping_item, don't know how this unit fell here.
Regarding the check, this is indeed something we need to decide. But anyway, this particular listener will be so useless thanks to some events on transition!

Btw, side question, is this ShippingItem::getState usefull? I see it just reflects the parent shipment state... The only state that could differ between Shipment and one of its ShipmentItem is returned. But is it something we want to track in the ShippingState? It is already tracked in the InventoryUnitState and this seems more logical there. I'd like to delete it =)

I don't think we need extra check. All we can do is to throw another
exception..

I believe the can method is more useful when dealing with UI, it can tell
us what buttons we should show.

On Fri, Apr 25, 2014 at 3:49 PM, Alexandre Bacco
notifications@github.comwrote:

In src/Sylius/Bundle/CoreBundle/EventListener/InventoryUnitListener.php:

$unit->setShippingState(ShipmentInterface::STATE_ONHOLD);

- break;

case $unit::STATE_SOLD:

$unit->setShippingState(ShipmentInterface::STATE_READY);

- break;

case $unit::STATE_RETURNED:

$unit->setShippingState(ShipmentInterface::STATE_RETURNED);

- break;

default:

throw new \InvalidArgumentException(sprintf('Unexpected inventory state "%s".', $state));

}

$this->factory->get($unit, 'sylius_inventory_unit')->apply($transitionName);

$this->factory->get($unit, 'sylius_shipping_unit')->apply($transitionName);

Yes for sylius_shipping_item, don't know how this unit fell here.
Regarding the check, this is indeed something we need to decide. But
anyway, this particular listener will be so useless thanks to some events
on transition!

Btw, side question, is this ShippingItem::getState usefull? I see it just
reflects the parent shipment state... The only state that could differ
between Shipment and one of its ShipmentItem is returned. But is it
something we want to track in the ShippingState? It is already tracked in
the InventoryUnitState and this seems more logical there. I'd like to
delete it =)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1426/files#r11986029
.

@winzou Remember that it is Sylius implementation where InventoryUnit and ShipmentItem is the same thing. Shipping bundle can be used standalone, as well as Inventory. The inventory states indicates the availability of the particular product stock item, and the shipping state of unit represents what is happening regarding the shipment - is it ready to be packed, sometimes just one item is returned etc.

This looks really good. I am wondering if this could help streamlining processes. At the moment for example its not easy to keep track of all the possibile event sequences and listener actions when executing the checkout flow. Say I need to implement a different checkout flow ( for example "one click checkout" ) but still need to keep all events consistent with sylius architecture.
I need to go through all checkout steps, and write down a little graph of all events to make sure I don't miss something when I 'rearrange' the flow.
It would be nice if there was some class that could be responsable for handling this graph so I can look at it and see all possible graph path along with the events they trigger.

Do you think finite's transition functions could be a good place to centralize event triggering in a more readable way? (Maybe you have already been implying this and I missed it)

Sorry if I am getting a little OT here, but so far in extending Sylius for my own purposes understanding and keeping track of all events has been the most time consuming task :)

It will help you to track to object-centered workflow. This is where most of the business is done.
But it won't change all the checkout events, which are linked to the checkout workflow and not really to the object itself.

@winzou Yes I understand and it looks very good.

I guess you could consider the checkout process as an object as well :D, thats what I like about the flow bundle, processes become objects that have steps ( transitions and states ).
But maybe I am overcomplicating things.
Probably once the events are documented that will be enough to help people modifying the checkout process to their needs.

Thanks for your reply and sorry for the OT

winzou · 2014-04-25T08:35:16Z

@pjedrzejewski do you know where is Travis?

edit: ok it was just incredibly slow to run

winzou · 2014-04-27T04:30:14Z

I stop here for this PR. Good to review guys.
Can't wait for the next PRs I have in mind!

pjedrzejewski · 2014-04-28T18:23:14Z

app/config/config.yml

+    - { resource: @SyliusShippingBundle/Resources/config/finite.yml }
+    - { resource: @SyliusOrderBundle/Resources/config/finite.yml }
+    - { resource: @SyliusInventoryBundle/Resources/config/finite.yml }
+    - { resource: @SyliusCoreBundle/Resources/config/finite.yml }


These should be moved to main.yml file in CoreBundle.

Use Finite State Machine

pjedrzejewski · 2014-04-28T18:27:11Z

I'll move the files in separate PR. Thank you Alexandre, you are awesome. 👍

winzou · 2014-04-29T01:29:45Z

Let's start the party now! 🎉

kayue · 2014-04-30T14:04:43Z

src/Sylius/Bundle/CoreBundle/Resources/config/finite.yml

+    sylius_order_payment:
+        class:         %sylius.model.order.class%
+        property_path: paymentState
+        graph:         sylius_order_payment


@winzou Are we missing OrderPaymentTransitions::GRAPH?

Also should we use state machine in StateResolver?

Sylius/src/Sylius/Component/Core/OrderProcessing/StateResolver.php

Lines 28 to 31 in 1f76020

public function resolvePaymentState(OrderInterface $order)

{

$order->setPaymentState($order->getPayment()->getState());

}

Or even better, we can remove these StateResolver and rely on the events triggered by state machine.

Yes we are missing OrderPaymentTransitions::GRAPH as well as OrderShippingTransitions.
StateResolver is about to disappear. It's not using state machine right now indeed. I hope I can soon have the correct listeners to get rid of state resolver. Not ready yet.

Cool, just want to confirm, thanks! 👍

Use Finite State Machine

kayue · 2014-05-15T03:57:47Z

src/Sylius/Bundle/CoreBundle/Resources/config/finite.yml

+                to:   partially_shipped
+            return:
+                from: [partially_shipped, shipped]
+                to:   returned


@winzou Missing transitions to change shipping state to cancelled. Do you want to include this change in #1447?

Use Finite State Machine

winzou added Enhancement labels Apr 24, 2014

winzou self-assigned this Apr 24, 2014

pjedrzejewski reviewed Apr 24, 2014
View reviewed changes

kayue reviewed Apr 24, 2014
View reviewed changes

src/Sylius/Bundle/CoreBundle/Resources/config/finite.yml

refunded:

type: final

transitions:

submit:

Copy link

Contributor

kayue Apr 24, 2014

Choose a reason for hiding this comment

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

Should we call it create?

pjedrzejewski reviewed Apr 25, 2014
View reviewed changes

winzou added 6 commits April 27, 2014 12:00

Add Finite dependency

0e4d31a

Switch to string state for Order

a5d642f

Add states & transitions configuration

dc7e4e1

Simple Finite implementation

175d976

Use constant for transition names

7f228cb

Use Finite with Payment as well

5592df9

winzou changed the title ~~[WIP] Use Finite State Machine~~ Use Finite State Machine Apr 27, 2014

Use constant for graph names

61f589a

pjedrzejewski reviewed Apr 28, 2014
View reviewed changes

pjedrzejewski pushed a commit that referenced this pull request Apr 28, 2014

Merge pull request #1426 from winzou/state-machine

0b1bbba

Use Finite State Machine

pjedrzejewski merged commit 0b1bbba into Sylius:master Apr 28, 2014

winzou deleted the state-machine branch April 29, 2014 01:28

kayue reviewed Apr 30, 2014
View reviewed changes

tomzx pushed a commit to tomzx/Sylius that referenced this pull request May 9, 2014

Merge pull request Sylius#1426 from winzou/state-machine

d7d024d

Use Finite State Machine

kayue reviewed May 15, 2014
View reviewed changes

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019

Merge pull request Sylius#1426 from winzou/state-machine

055fa0b

Use Finite State Machine

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019

Merge pull request Sylius#1426 from winzou/state-machine

afa7d69

Use Finite State Machine

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Finite State Machine #1426

Use Finite State Machine #1426

winzou commented Apr 24, 2014

kayue commented Apr 24, 2014

pjedrzejewski commented Apr 24, 2014

pjedrzejewski commented Apr 24, 2014

winzou commented Apr 24, 2014

pjedrzejewski Apr 24, 2014

pjedrzejewski commented Apr 24, 2014

kayue Apr 24, 2014

winzou commented Apr 25, 2014

pjedrzejewski Apr 25, 2014

pjedrzejewski Apr 25, 2014

winzou Apr 25, 2014

kayue Apr 25, 2014

pjedrzejewski Apr 25, 2014

smarques Apr 25, 2014

winzou Apr 25, 2014

smarques Apr 25, 2014

winzou commented Apr 25, 2014

winzou commented Apr 27, 2014

pjedrzejewski Apr 28, 2014

pjedrzejewski commented Apr 28, 2014

winzou commented Apr 29, 2014

kayue Apr 30, 2014

winzou Apr 30, 2014

kayue Apr 30, 2014

kayue May 15, 2014

winzou May 15, 2014

	public function resolvePaymentState(OrderInterface $order)
	{
	$order->setPaymentState($order->getPayment()->getState());
	}

Use Finite State Machine #1426

Use Finite State Machine #1426

Conversation

winzou commented Apr 24, 2014

kayue commented Apr 24, 2014

pjedrzejewski commented Apr 24, 2014

pjedrzejewski commented Apr 24, 2014

winzou commented Apr 24, 2014

Choose a reason for hiding this comment

pjedrzejewski commented Apr 24, 2014

Choose a reason for hiding this comment

winzou commented Apr 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

- break;

- break;

- break;

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winzou commented Apr 25, 2014

winzou commented Apr 27, 2014

Choose a reason for hiding this comment

pjedrzejewski commented Apr 28, 2014

winzou commented Apr 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment