yohang / Finite

A Simple PHP Finite State Machine
http://yohan.giarel.li/Finite
MIT License
1.31k stars 188 forks source link

Add a callback to cascade transitions #33

Closed winzou closed 10 years ago

winzou commented 10 years ago

Hi,

This PR adds 2 useful things:

  1. The ability to pass arguments to callbacks. Currently, we are limited to the arguments $object and $transitionEvent. With this PR you can define arguments to pass to your callback directly from the graph configuration (via the args parameter).
  2. A simple callback that you can use (in combination with the callback arguments :) to cascade some transitions. I updated the callback.php example with an automatic transition from accepted to archived. This can be useful in many cases. The callback supports as well the cascade when dealing with the same object but different graph, and even with another object reachable from the main object via a PropertyAccessor (for example, when your shipment goes from ready to shipped, you may want to automatically update your order (reachable with shipment.order).

Thanks,

winzou commented 10 years ago

@yohang I added:

In overall it adds a great flexibility to what we can do through callbacks configuration. What do you think?

yohang commented 10 years ago

Hi,

The built-in callback can be really useful. About the args configuration, I think it's a bit "too much", as an user point of view, it adds a lot of magic, and it is a bit hard to document.

Why not keeping $object and $event, and simply add an 'extra_args' parameter ?

winzou commented 10 years ago

Hi @yohang

The args configuration, this is what makes the callback system really powerful. Let me explain why (we had this a lot with sylius): you have application services, that have already their arguments. Then you have events that are triggered. You need a dummy class in the middle that maps the event to your existing service. This is new layer just for arguments mapping, so useless!

With the arg configuration I offer, you can choose what argument you send to the callback: you don't need this stupid layer in the middle. This is why it's so great. Your extra_args doesn't cover this.

I don't think it adds a lot of magic... Users just need to know that they can use Expression Language with the 2 given variables: object and event. And if they don't customize arguments, then they still have the $object and $event like before (default configuration).

What do you think? This is something we do need in Sylius so if you don't accept it I will implement this feature there, but I would prefer to have it integrated here of course :) (You can see it in action already there: https://github.com/Sylius/Sylius/pull/1447)

yohang commented 10 years ago

Well, ok, I see the point.

Auto-wiring of existing business services is indeed a killer feature, and we keep the default behavior.

As we are already using some Symfony components, I suppose that adding ExpressionLanguage is not a problem.

pjedrzejewski commented 10 years ago

:+1:

winzou commented 10 years ago

Cool! :) I have rebased the PR.

pjedrzejewski commented 10 years ago

It is awesome to see good ideas flow between projects!

winzou commented 10 years ago

@yohang is there anything missing in order to merge?

kayue commented 10 years ago

ping @yohang :)

yohang commented 10 years ago

@winzou Yes, time is missing ! I'll try to merge today.

Byte-Lab commented 10 years ago

Hi @yohang , just wanted to send a friendly request that you merge this PR whenever possible. Thanks :-)

dnagirl commented 10 years ago

This PR would solve a big problem for me. I'm really looking forward to the merge.

hacfi commented 10 years ago

I really like the new features but I’m not sure if ExpressionLanguage should be a dependency. We could do something like https://github.com/symfony/symfony/pull/11283/files to only add it to the suggested packages.