Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
Added missing events description#7528
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
javiereguiluz 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.
@Guite thanks for your contribution!
Although I don't use the Workflow component, I've proposed some changes. The reason is that we must be extra careful when describing an event to explain exactly when it's triggered. That's why I used "just before" and "just after" in the description. Please tell me if I did it right. Thanks!
Guite commentedFeb 27, 2017
@javiereguiluz looks fine for me, but should be reviewed by someone who is more experienced with the internals. |
workflow/usage.rst Outdated
| The following events are dispatched for all workflows: | ||
| * ``workflow.guard``: occurs just before starting a transition. It allows you to | ||
| prevent the transition by calling ``$event->setBlocked(true);`` as shown above. |
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.
It is also fired when testing available transitions.
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.
@HeahDude added
workflow/usage.rst Outdated
| * ``workflow.guard``: occurs just before starting a transition. It allows you to | ||
| prevent the transition by calling ``$event->setBlocked(true);`` as shown above. | ||
| * ``workflow.leave``: occurs just after an object has left it's current state. |
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.
We should mention that all events give carries the subject and the transition.
Also I think we should clarify what state is available in the event:
* ``workflow.leave``: carries the marking with the initial places, occurs just before the transition.* ``workflow.transition``: carries the marking with the current places, occurs during the transition.* ``workflow.enter``: carries the marking with the new places, occurs just after the transition.
Actually while checking this I've openedsymfony/symfony#21793.
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.
@HeahDude added
HeahDude 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@Guite, I've made some other comments, but please wait for other members opinion before doing any changes, as we also need to see ifsymfony/symfony#21793 gets merged.
| * ``workflow.transition``: occurs just before starting to transition to the new state. | ||
| * ``workflow.enter``: occurs just after the object has entered into the new state. | ||
| * ``workflow.guard``: occurs just before a transition is started and when testing which transitions are available. It allows you to define that the transition is not allowed by calling ``$event->setBlocked(true);`` as shown above. | ||
| * ``workflow.leave``: carries the marking with the initial places, occurs just after an object has left it's current state. |
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.
it's =>its.
To avoid confusion, maybe we should not use "initial" and say something like:carries the marking with the places held before an object leaves it's current state.
| * ``workflow.enter``: occurs just after the object has entered into the new state. | ||
| * ``workflow.guard``: occurs just before a transition is started and when testing which transitions are available. It allows you to define that the transition is not allowed by calling ``$event->setBlocked(true);`` as shown above. | ||
| * ``workflow.leave``: carries the marking with the initial places, occurs just after an object has left it's current state. | ||
| * ``workflow.transition``: carries the marking with the current places, occurs just before starting to transition to the new state. |
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.
carries the marking with the places held just after leaving the previous state, and just before entering into the new state.
| * ``workflow.guard``: occurs just before a transition is started and when testing which transitions are available. It allows you to define that the transition is not allowed by calling ``$event->setBlocked(true);`` as shown above. | ||
| * ``workflow.leave``: carries the marking with the initial places, occurs just after an object has left it's current state. | ||
| * ``workflow.transition``: carries the marking with the current places, occurs just before starting to transition to the new state. | ||
| * ``workflow.enter``: carries the marking with the new places, occurs just after the object has entered into the new state. |
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.
This one needs no "correction", but to be consistent with others, what about:carries the marking with the places held after the object has entered into the new state.
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.
Sosymfony/symfony#21793 is merged now and this should be reworded to:
carries the marking with the current places, occurs just before the object enters into the new places.
…HeahDude)This PR was merged into the 3.2 branch.Discussion----------[Workflow] Fixed marking state on leave and enter events| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | ~| License | MIT| Doc PR | related tosymfony/symfony-docs#7528It seems weird to me to dispatch an event while the marking is not yet marked by the new places.The event is still "marked" by old places when leaving occurs, my guess is that the symmetry should be mirrored.Do you agree? Should I add a test?Commits-------175858a [Workflow] Fixed marking state on leave and enter events
lyrixx commentedMar 8, 2017
The PR has been merged. And BTW, if you can add a note in the doc aboutsymfony/symfony#21925 it would be awesome ! |
HeahDude commentedMar 9, 2017
The change@lyrixx suggests should be done in another PR targeting master since this one needs to be merged in 3.2. |
javiereguiluz commentedAug 4, 2017
This is ready for the final review. Thanks! |
Guite commentedAug 4, 2017
This is rebased for Should I add |
xabbuh commentedAug 9, 2017
I think this should be rearranged to fit with the existingUsing Events section. |
Guite commentedAug 9, 2017
@xabbuh you are right. I think this one can be closed, since the existing section covers the whole topic already. |
xabbuh commentedAug 9, 2017
@Guite Thanks for the feedback. Closing then. If you want to add more information to the existing section, please feel free to create new PRs. :) |
This PR adds descriptions for all events dispatched by the workflow component.