Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

Hey. Thank you for this PR.

Could you please give me a link to where in the docs it says that $context should be part of the Guard event. Im not sure if this is a bug fix or a new feature or a bug in the docs.
A quick search on the merged issues says that we never intended to add $context on guard events (which means it is a new feature)

Hello @Nyholm

check on https://symfony.com/doc/current/workflow.html#using-events

In Symfony 5.2, the context is accessible in all events:
`
// $context must be an array
$context = ['context_key' => 'context_value'];
$workflow->apply($subject, $transitionName, $context);

// in an event listener
$context = $event->getContext(); // returns ['context']
`
based on this, I don't see my pull request as a new feature.

I think it is useful to have an instance of $context on GuardEvent so developers can pass custom payload if needed to subscribers of GuardEvent.

Have a nice day

In the PR that introduced this feature guard events were not changed on purpose: #37539 (comment)

This looks like something that needs to be changed in the documentation instead.

Adding an additional parameter to all those methods is a breaking change and has to come with a proper deprecation layer. There's no way we can do this in a bugfix release.

I agree with you if this changes become has a breaking change, but it is backward compatibility because it is optional just for the case if needed, so I don't see the need to deprecated anything.

Hello @xabbuh

In the PR that introduced this feature guard events were not changed on purpose: #37539 (comment)

This looks like something that needs to be changed in the documentation instead.

After read the thread you suggest, I see your point, but let me talk about my use case and why I see the need to have access to context on GuardEvent.

I am working on some chatbots where the conversation state is handled by Workflow with state_machine, manage the transitions between states must be validated based con incoming message, so here it comes the GuardEvent which validate and allow the transition between states, but it requires the incoming message is data as payload to do its job.

The $context is useful in so many ways to all events of the workflow if the developer require it.

I hope you see my point, otherwise I am open to discuss better approach to implement this use case.

Have a nice day

it is backward compatibility because it is optional

No, it isn't. For example, if I had a custom implementation of WorkflowInterface in my app, that implementation won't have your new optional parameter. If we merged your PR as is, my code would break next time I upgrade the workflow component.

See https://3v4l.org/3uko6

I don't see the need to deprecated anything.

Yes, we would need to deprecate implementations of WorkflowInterface without the new optional parameter. We cannot do this is a bugfix release.

Your use-case might be valid, but this change is by our definition a new feature. I'm moving this to the 6.1 milestone which is our next feature release.

Awesome. Now it is correctly labeled as a new feature in 6.1.

May I ask @lyrixx to elaborate on this comment where you say you are agains guard events having context?

Hi @derrabus

You have all the right, I have done the requested changes.

Thanks for your support

Hi @lyrixx I would like to listen your opinion too about why not $context on GuardEvent.

Thanks in advance

Hi @lyrixx I would like to listen your opinion too about why not $context on GuardEvent.

Hello,

I always refused to add the context to the apply method because the context (must) contains only "contextual" content (date, user, for example)

To me, the decision to accept of deny a transition should only occurs against the subject that pass into the workflow.

If you want to vote on somethings else, it should be on the subject. BTW, this value does't have to be persisted.

Thanks, @lyrixx, for your reply

I understand your argument, it makes sense from your point of view, but the end developer using workflow must have the choice to decide what is needed as contextual based on his needs.

Workflow is an excellent component, but in my opinion must be consistent with the docs about the context is available to all events, not a few of them only, because it makes confuse use it.

Take this only has a constructive opinion, I am open to contribute and help to improve this component.

, but in my opinion must be consistent with the docs about the context is available to all events, not a few of them only, because it makes confuse use it.

Sure, the doc must be fixed :) Would you mind to submit a PR? That would be lovely. Thanks

Already done