Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
RemoveGuardEvent::getContext() method and addHasContextTrait trait#51493
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
e9c9d72 to16b7e82Compare| */ | ||
| trait HasContextTrait | ||
| { | ||
| privatearray$context = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I made the propertyprivate instead ofprotected as all concrete classes using this trait are already allfinal. Although it may be a potential BC break, it should not as event classes are final and internal. Application code should are not supposed to extend the base abstractEvent class to produce new subtypes of workflow events.
| * @author Grégoire Pineau <lyrixx@lyrixx.info> | ||
| * @author Hugo Hamon <hugohamon@neuf.fr> | ||
| * | ||
| * @internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Trait is markedinternal as it's not supposed to be used outside of the Workflow component itself.
| * | ||
| * @internal | ||
| */ | ||
| trait HasContextTrait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure about the naming of this trait although it's purely made for an internal use. I was also thinking of naming itHoldContextTrait. Any other better suggestions are welcome and appreciated.
16b7e82 to56e8101CompareGuardEvent::getContext() method and addHasContextTrait traitGuardEvent::getContext() method and addHasContextTrait traitUh oh!
There was an error while loading.Please reload this page.
56e8101 to2aee3aeCompare
lyrixx left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks
GuardEvent::getContext() method and addHasContextTrait traitGuardEvent::getContext() method and addHasContextTrait traitfabpot commentedAug 30, 2023
Thank you@hhamon. |
Uh oh!
There was an error while loading.Please reload this page.
As discussed with@lyrixx, the
GuardEvent::getContextmethod was confusing as the context given as the 3rd argument to the WorflowInterface::appy() method is never passed along to the guard events. According to@lyrixx, the guard listeners must take any decisions based on the subject itself and not on the given contextual data (which are supposed to remain metadata). As a consequence, calling theGuardEvent::getContext()method always returned an empty context array so far.To prevent this confusion any longer, the method has been deprecated in Symfony 6.4 (see#51484) and removed in this PR. The
GuardEventclass hierarchy no longer has thegetContextmethod while other event classes keep having it.