Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.2k
[Workflow] Added explaination on context in events and initial marking#14678
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
@nicolas-grekas@lyrixx |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Should this target 5.1, since it tells about 5.1? |
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.
@nicolas-grekas
Nope !
It does not tell about 5.1
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedDec 8, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I meant this. Maybe that's not relevant, I didn't check deeper. |
I don't know why it is in my PR. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Antoine Makdessi <antoine.makdessi@agriconomie.com>
@nicolas-grekas Is there more I should do ? |
lyrixx commentedDec 15, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
So, I guess it is not a problem if it's merge here ? |
I think this is fine. But since 5.1 is still maintained, a PR targeting this branch should be open too |
Uh oh!
There was an error while loading.Please reload this page.
components/workflow.rst Outdated
Initialization | ||
-------------- | ||
If you want to initiate your workflow, you can call ``getMarking``:: |
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.
If you want to initiate your workflow, you can call ``getMarking``:: | |
If you want to initiate your workflow, you can call ``getMarking()`` method:: |
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.
And I'm not super confortable with this sentence. What does mean "initiate your workflow"?
I do understand what you want to say, but for new comers, it could be confusing.
What about something like
When the object enter the workflow for the first time, the workflow will initialize the
property
with theinitial_place
.
If you want to have always a valid marking in your database, you can use thegetMarking()
method to initialize the object property with the initial place::
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 I understand this sentence :When the object enter the workflow for the first time, the workflow will initialize the property with the initial_place.
And instead ofinitial_place
, I think we should useinitial_marking
as in the configuration :https://symfony.com/doc/current/workflow.html#configuration
So, I would say :
If the property of your object is null and you want to set it with the
initial_marking
in the configuration, you can call thegetMarking()
method to initialize le object property::
WDYT ?
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.
you are right about initial marking :) I used the old name, sorry.
You sentence is correct, let's go for it.
Minor detail:
- in the configuration+ from the configuration
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.
DONE !
If it's ok for you, I let you resolve the conversation and maybe... merge the PR ?
Co-authored-by: Grégoire Pineau <lyrixx@lyrixx.info>
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.
Nice 👍🏼
Note for merge: This PR could be split in 3 parts
- The "Initialization" should go to the lowest maintained branche (4.4)
- versionadded 5.1 + final note => 5.1
- versionadded 5.2 => 5.2
I've added the "Initialization" section to 4.4 manually ... but then I tried to merge this in 5.1 and I have lots and lots of conflicts 😞 |
Hi! Thanks for your contribution. I rebased this branch in#15200 and will merge that PR (there was a merge commit in the branch, which caused lots of conflicts when merging this PR). |
No description provided.