8000 [Workflow] Enter events · Issue #21433 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Workflow] Enter events #21433

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
ihorsamusenko opened this issue Jan 27, 2017 · 25 comments
Closed

[Workflow] Enter events #21433

ihorsamusenko opened this issue Jan 27, 2017 · 25 comments
Labels
Workflow

Comments

@ihorsamusenko
Copy link
ihorsamusenko commented Jan 27, 2017 •
edited
Loading
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? yes/no
Symfony version 3.2.0

I found a strange behaviour of Workflow Class. Particularly this part https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Workflow/Workflow.php#L139-L141 . Turns out, if a listener is subscribed on "workflow.entity.enter.place" it will receive the entity when it is not in the "place" yet.
So technically there is no an event like "right-after-a-transition".
Shouldn't we trigger ""workflow.entity.enter" events after changing the place for entity?

$this->transition($subject, $transition, $marking);
$this->markingStore->setMarking($subject, $marking);
$this->enter($subject, $transition, $marking);
@ihorsamusenko ihorsamusenko changed the title [Workflow] [Workflow] Enter events Jan 27, 2017
@ihorsamusenko
Copy link
Author

@lyrixx ping

@lyrixx
Copy link
Member
lyrixx commented Jan 27, 2017

Actually, this event is named enter and not entered.

Anyway, I have no objection to invert theses two lines But I would like to know your use case before it.

@stof
Copy link
Member
stof commented Jan 27, 2017

there is already another event after marking the place: announce. So I don't see any reason to change the place where enter is triggered. This would make both events equivalent (and would be a BC break).

@ihorsamusenko
Copy link
Author
ihorsamusenko commented Jan 27, 2017 •
edited
Loading

Well, lets imagine there is a Feature which has many Tasks. When the last task is closed the feature needs to be approved. In that case it would be convenient to have something like:

if ($workflow->can($task, 'close')) {
    $workflow->apply($task, 'close');
}

And have a listener which listens to "workflow.task.entered.closed" which do the job. But you cannot do that, because in that case you will get entity which is not entered new place yet.

Even the name "entered" tells us that an entity entered the place, while this is just about to enter it

@stof
Copy link
Member
stof commented Jan 27, 2017

hmm, announce is about newly enabled transitions, not 8000 about the place. So it is not the same event actually.

But we should rather add another event if there is a use case for it IMO. changing the behavior of the existing event would break BC, which could do weird things

@stof
Copy link
Member
stof commented Jan 27, 2017

@samusenkoiv the event name is not entered. It is enter. there is no workflow.task.entered.closed event

@ihorsamusenko
Copy link
Author
ihorsamusenko commented Jan 27, 2017 •
edited
Loading

@stof ok, so my argument about the name does count then =)
As for the "announce" event, it will be triggered only in case there are some other further possible places for an entity.

@ihorsamusenko
Copy link
Author

It would be reasonable to have "before_enter" and "after_enter", but that'd lead to BC breaks too.

@stof
Copy link
Member
stof commented Jan 27, 2017 •
edited
Loading

Well, we could add a entered event after updating the marking, without any BC break.

@ihorsamusenko
Copy link
Author

@stof yes we could. Do we agree on that, should I prepare a PR?

@Padam87
Copy link
Contributor
Padam87 commented Feb 1, 2017 •
edited
Loading

@samusenkoiv #20787

I wanted to do the same thing last month :)

@ihorsamusenko
Copy link
Author
8000 ihorsamusenko commented Feb 9, 2017

@lyrixx any thoughts?

@meDavid
Copy link
meDavid commented Feb 9, 2017

My use case is the following: automatic transitions. For automatic transitions I would now have listeners on the entered event that would (optionally) apply a secondary transition. It would make the following test succeed, which is not possible with the enter event:

	public function testCanContinue()
	{
		// The graph looks like:
		// +---+     +----+     +---+     +----+     +---+
		// | a | --> | t1 | --> | b | --> | t2 | --> | c |
		// +---+     +----+     +---+     +----+     +---+
		$definition = $this->createSimpleWorkflowDefinition();
		$subject = new \stdClass();
		$subject->marking = null;
		$eventDispatcher = new EventDispatcher();
		$workflow = new Workflow($definition, new SingleStateMarkingStore(), $eventDispatcher, 'workflow_name');
		$eventDispatcher->addListener('workflow.workflow_name.entered.b', function (Event $event) use ($workflow) {
			$workflow->apply($event->getSubject(), 't2');
		});

		$workflow->apply($subject, 't1');
		$this->assertEquals('c', $subject->marking);
	}

@Padam87
Copy link
Contributor
Padam87 commented Feb 9, 2017

@meDavid Seconded. I needed this too, settled for the announce event.

@lyrixx
Copy link
Member
lyrixx commented Feb 9, 2017

@meDavid Indeed, it's better to listen the announce event in your use case.

@samusenkoiv Could you explain why you could not use the announce event?

@ihorsamusenko
Copy link
Author
ihorsamusenko commented Feb 9, 2017 •
edited
Loading

@lyrixx I do not understand how I can do it with announce.
As far as figured out of the code:
Announce checks all possible further transitions, finds if some of them are eligible to perform and dispatches that those transitions can be performed right now ("workflow.%name%.announce.%eligible_transition").
Or am I missing something?

What I would like to have is being notified when an entity has changed its place. Just like enter, but enter is triggered before the entity gets to the place.

https://github.com/symfony/symfony/pull/20787/files solves this in a nice way, why don't like it?

@lyrixx
Copy link
Member
lyrixx commented Feb 9, 2017

If you want to auto apply transition, you can have a look at: lyrixx/SFLive-Paris2016-Workflow@9fda92a

https://github.com/symfony/symfony/pull/20787/files solves this in a nice way, why don't like it?

The implementations is good. But we already have many events, and If we can avoid to add new events it's better. If there is too many options, it's harder to understand each one. But if there is no solution for this use case, we will add it to the core ;)

@meDavid
Copy link
meDavid commented Feb 9, 2017

@lyrixx My usecase is indeed resolved using the announce event.

@ihorsamusenko
Copy link
Author
ihorsamusenko commented Feb 9, 2017 •
edited
Loading

@lyrixx I do not want to auto apply transition.

Yes, I do not see a solution. To be honest, I believe the enter event is useless, while entered is essential one. What needs to happen to include it in core?

But we already have many events.

We could add an option which would indicate which of the events we'd like to be dispatched. What do you think? I could work on it in case it has chances to be merged.

@lyrixx
Copy link
Member
lyrixx commented Feb 9, 2017

No, adding an option is even worst ;)

I still don't get your use case. because here it's about auto transition.

Could you explain again you use case please?

@ihorsamusenko
Copy link
Author
ihorsamusenko commented Feb 9, 2017 •
edited
Loading

@lyrixx well, from some perspective it is indeed about auto transition, but it is about auto transition of another entity.

So we have Feature entity which has many Task. When a task is in place "finished" we need to check all the others feature's tasks. If every task is in place "finished" then we can try to mark the feature as "completed".
With current events there is no way to do that.

I'll explain:
Feature f1 has two tasks t1 and t2. The t1 task has been "finished" for a while. The t2 task is about to go to "finished" place. So I have "leave", "transition", "enter" events. All of them are dispatched before the t2 task gets into "finished" place. Provided I listen to one of them, I would have such a code:

public function onTaskFinished(Event $event)
{
        // the t2 task
        $task = $event->getSubject();
        foreach ($task->getFeature()->getTasks() as $task) {
            // if at least one task is not finished
            if ($task->isFinished() === false) {
                return;
            }
        }
        
        // here I know that all tasks are finished, but they are not, because of the t2 task
}

And with entered event being dispatched after setting marking this would be possible.

@lyrixx
Copy link
Member
8000
lyrixx commented Feb 9, 2017

Ok, Nice I understand your use case.

What about:

public function onTaskFinished(Event $event)
{
    // the t2 task
    $currentTask = $event->getSubject();
    foreach ($task->getFeature()->getTasks() as $task) {
        // The current task is finished
        if ($task === $currentTask) {
            continue;
        }
        
        // if at least one task is not finished
        if ($task->isFinished() === false) {
            return;
        }
    }
    
    // here I know that all tasks are finished, but they are not, because of the t2 task
}

But anyway, even if it is possible to do that I don't like it so much. Because it will mean the Feature is finished before the last Task.

Now I'm 👍 for a new event.

I will re-open #20787 and review it, and merge it ;)

@Padam87 Padam87 mentioned this issue Feb 9, 2017
Closed
@ihorsamusenko
Copy link
Author

Indeed, such a condition is a solution here (although it seems hacky), but it is not possible if we go outside onTaskFinished.
I'm glad I was able to explain the objectives.

@lyrixx
Copy link
Member
lyrixx commented Feb 9, 2017

Yeah, every sensible decision is really easier with a clear use case. Thanks.

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
@lyrixx lyrixx closed this as completed Feb 13, 2017
@fduch
Copy link
Contributor
fduch commented Feb 13, 2017

@meDavid Indeed, it's better to listen the announce event in your use case.
@lyrixx My usecase is indeed resolved using the announce event.

@lyrixx @meDavid please note that announce event can redundantly waste the time computing enabled transitions (since it involves guards processing) in case when you simply need to perform cascade processing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Workflow
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@lyrixx @stof @Padam87 @fduch @meDavid @ihorsamusenko
0