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

[Workflow] ANNOUNCE event should be optional #35286

Closed
Harry-Dunne opened this issue Jan 9, 2020 · 7 comments · Fixed by #35322
Closed

[Workflow] ANNOUNCE event should be optional #35286

Harry-Dunne opened this issue Jan 9, 2020 · 7 comments · Fixed by #35322

Comments

@Harry-Dunne
Copy link
Contributor

@Harry-Dunne Harry-Dunne commented Jan 9, 2020

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Symfony version 4.4.2

The workflow.announce event has a side effect : it fires guard events for all accessible transitions from the subject's place, even if there is no subscriber registered for this event. AFAIK it's the only event of the workflow component with this behavior.

Code in guard event, usually business rules, can be costly to execute. This cost is useless if the announce event is not leveraged, so the dispatching of this event should be optional.

@xabbuh xabbuh added the Workflow label Jan 10, 2020
@xabbuh

This comment has been minimized.

Copy link
Member

@xabbuh xabbuh commented Jan 10, 2020

Can you be a bit more precise what you mean? Reading the code I do not see any difference in how we dispatch the different events.

@Harry-Dunne

This comment has been minimized.

Copy link
Contributor Author

@Harry-Dunne Harry-Dunne commented Jan 13, 2020

foreach ($this->getEnabledTransitions($subject) as $transition) {

When workflow.announce event is dispatched, Workflow::getEnabledTransitions() method is called, and thus all guard events are dispatched for each possible transition from the subject's place.

Others events (except guard events themselves, obviously) don't fire guard events and are less expensive.

@lyrixx

This comment has been minimized.

Copy link
Member

@lyrixx lyrixx commented Jan 13, 2020

@Harry-Dunne How would you configure this behavior? (from the end developer perspective) Thanks

@Harry-Dunne

This comment has been minimized.

Copy link
Contributor Author

@Harry-Dunne Harry-Dunne commented Jan 13, 2020

The simplest way I can figure is adding an optional boolean argument to Workflow::apply() method.

public function apply(object $subject, string $transitionName, array $context = [], bool $dispatchAnnounceEvent=true)
{
        [...]
        if ($dispatchAnnounceEvent) {
            $this->announce($subject, $transition, $marking);
        }
        [...]

This argument is set to true as default for BC.

@lyrixx

This comment has been minimized.

Copy link
Member

@lyrixx lyrixx commented Jan 13, 2020

I usually don't like to add flag to methods because we may end with

public function apply(object $subject, string $transitionName, array $context = [], bool $dispatchAnnounceEvent=true, $flag2, $flag3, ...)

And when we use it, it's not really undestandable if you don't know the API:

$this->workflow->apply($subject, 'confirm', [], true);

I propose 4 things here:

  1. Use a bitfield argument (more flexible on the long term)
  2. Use an array argument (more flexible on the long term)
  3. Add a new method applyWithoutAnnonce
  4. Leverage the context

My preference goes to option 4/

$this->workflow->apply($subject, 'confirm', ['disable_annonce_event' => true]);
@noniagriconomie

This comment has been minimized.

Copy link
Contributor

@noniagriconomie noniagriconomie commented Jan 13, 2020

Hi @Harry-Dunne

Code in guard event, usually business rules, can be costly to execute

I agree with this, but what about keeping the component generic, and use specific project code to "avoid" this?
Can you like "memory cache" the result of a first call to your business rules guard logics, so that the n times the workflow needs it, the result is already here?

The apply() IMO must/should remains "simple"/"basic"

@lyrixx +1 for context, maybe with a symfony prefix so that is does not overlap with dev business context keys :)

@Harry-Dunne

This comment has been minimized.

Copy link
Contributor Author

@Harry-Dunne Harry-Dunne commented Jan 13, 2020

Hi @noniagriconomie

Can you like "memory cache" the result of a first call to your business rules guard logics, so that the n times the workflow needs it, the result is already here?

I don't think it's possible in this case... Guard events dispatched by announce event correspond to transitions not yet applied, preemptively. Conditions in code listening these events may be different from those of the currently applied transition.

@lyrixx +1 for the context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.