Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed
epitre wants to merge11 commits intosymfony:5.2fromShaurifr:eventWithContext

Conversation

epitre
Copy link
Contributor

No description provided.

@epitre
Copy link
ContributorAuthor

@nicolas-grekas@lyrixx
New Pull-Request here

@nicolas-grekasnicolas-grekas changed the titleEvent with context[Workflow] Added explaination on context in events and initial markingDec 8, 2020
@nicolas-grekas
Copy link
Member

Should this target 5.1, since it tells about 5.1?

Copy link
ContributorAuthor

@epitreepitre left a 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

@epitre
Copy link
ContributorAuthor

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedDec 8, 2020
edited
Loading

TheWorkflow::DISABLE_ANNOUNCE_EVENT constant was introduced in Symfony 5.1.

I meant this. Maybe that's not relevant, I didn't check deeper.

@epitre
Copy link
ContributorAuthor

I don't know why it is in my PR.
If you look at the old PR, it was already there :https://github.com/symfony/symfony-docs/pull/13947/files#diff-7e57c3d53ac6934b6aeb893604622003d26e9e1c85bdea5e5e524d3f3074b13eR355

epitreand others added2 commitsDecember 8, 2020 22:37
Co-authored-by: Antoine Makdessi <antoine.makdessi@agriconomie.com>
@epitre
Copy link
ContributorAuthor

@nicolas-grekas Is there more I should do ?
@lyrixx Do you have any idea where the doc for the SF5.1 in this pull-request comes from ?

@lyrixx
Copy link
Member

lyrixx commentedDec 15, 2020
edited
Loading

@epitre It was thisPR

@epitre
Copy link
ContributorAuthor

So, I guess it is not a problem if it's merge here ?

@lyrixx
Copy link
Member

I think this is fine. But since 5.1 is still maintained, a PR targeting this branch should be open too

Initialization
--------------

If you want to initiate your workflow, you can call ``getMarking``::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
If you want to initiate your workflow, you can call ``getMarking``::
If you want to initiate your workflow, you can call ``getMarking()`` method::

Copy link
Member

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 theproperty 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::

Copy link
ContributorAuthor

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 theinitial_marking in the configuration, you can call thegetMarking() method to initialize le object property::

WDYT ?

Copy link
Member

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

Copy link
ContributorAuthor

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 ?

epitreand others added3 commitsDecember 22, 2020 14:18
Copy link
Member

@lyrixxlyrixx left a 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

@javiereguiluz
Copy link
Member

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 😞

@wouterj
Copy link
Member

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).

@wouterjwouterj closed thisApr 7, 2021
wouterj added a commit that referenced this pull requestApr 7, 2021
… initial marking (epitre)This PR was submitted for the 5.x branch but it was merged into the 5.2 branch instead.Discussion----------[Workflow] Added explaination on context in events and initial markingFinihes#14678Commits-------5a8ce04 Added explaination on context in events and initial marking
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@noniagriconomienoniagriconomienoniagriconomie left review comments

@lyrixxlyrixxlyrixx approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@epitre@nicolas-grekas@lyrixx@javiereguiluz@wouterj@noniagriconomie@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp