-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added docs for Workflow component #6871
Conversation
can define the workflow like this:: | ||
|
||
$states = ['draft', 'review', 'rejected', 'published']; | ||
$transactions[] = new Transition('to_review', ['draft', 'rejected'], 'review'); |
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 will not work. See symfony/symfony#19605
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.
please add use statements for classes in the first code example you're using them.
First a more cosmetic issue: Please add a line break after the first word that crosses the 72th character. It seems like your lines are now longer than this.
I don't know the component, but the docs you've written now are lacking some important details:
It looks fine to me. However, maybe @lyrixx has some other opinion, as it seems like your concept of the Workflow component (as state machine) differs from his view. At last, if you have some time, please restructure the workflow component docs in a little different way:
|
|
||
.. code-block:: php | ||
|
||
$post = new \stdClass(); |
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.
let's use an imaginary BlogPost
class here.
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.
Please prepend this example with // ...
, to indicate that this code should be added to the previous code example.
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 would rather show the definition of the BlogPost class since it is important that there is a field called currentState
.
Oh, and completely forgot to say a big thanks for starting this! Would be great to have this component documented before the release! |
public static function getSubscribedEvents() | ||
{ | ||
return array( | ||
'workflow.blogpost.guad.to_review' => array('guardReview'), |
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 guess there is a typo here, isn't it ?
- 'workflow.blogpost.guad.to_review'
+ 'workflow.blogpost.guard.to_review'
Is it related to the Guard
component ? I don't see any security stuff here.
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.
Thank you.
No it is not related to the guard component. It is just named that way. I was also confused in the beginning.
Thank you for the feedback. Really appreciate that. I will update this PR accordingly. Expect an update tonight or within the next coming days. |
@Nyholm last comment (I will probably be crushed for that), but I think we can't escape the explanation about what is a state machine, and how this component differs from a state machine. I was pretty sure the component creator have explained it somewhere :) else we will have a lot of (legitimate) questions about "why not a finite state machine ?" , "why workflow and not a state machine?" etc ... |
I think you are correct. I do also want to research if one can consider a state machine as a special case of a workflow. |
----- | ||
|
||
The workflow component gives you an object oriented way to work with state machines. A state machine lets you | ||
define *places* (or *states*) and *transactions*. A transaction describes the action to get from one place to another. |
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 think you should replace "transaction" with "transition" everywhere.
It would be nice to describe the purpose of the component more. By now, I fail to understand the component intent: OK, it can be useful for dumping graphs. But I'm pretty sure it should not be used to describe "primary" domain logic to handle these transitions in the domain model. Model should take care of its internal state by itself: namespace Acme\BlogPosting;
final class BlogPost
{
public function publish() {
if ($this->status->equals(PostStatus::published())) {
return;
}
Assertion::true($this->status->equals(PostStatus::waitingForReview()));
$this->apply(new BlogPostWasPublished($this->id));
}
private function whenBlogPostWasPublished(BlogPostWasPublished $event) {
$this->status = PostStatus::published();
}
} Not sure why somebody would replace explicit model invariants with generic Symfony's component. It's OK to handle "secondary" domain logic though (reflection of a domain model), e.g. if you want to hide some senseless transitions from UI. But it has nothing to do with domain model. |
And just to be clear, example from author of the component looks like a strong anti-pattern for me. Anaemic domain model with implicit I don't want to look offensive, but I hope it wouldn't be proclaimed as something like "best practice for your business logic". Because it's not. |
Nothing about this property couples it to the framework... |
I'm sorry, but I'm not sure I understand you correctly. Coupling with a framework is not the main issue here, but anaemia & vagueness is. |
You can name this property anything you want, so that removes the vagueness? (I got your first point, but the model still is as anaemic as you want afaics) |
No, it doesn't. Still, it is either array or Further, the component raises vague events like
The thing is the component breaks encapsulation of the model (at least, given examples do). The component wants to control transitions of the |
@unkind you may be interested by this study about Petri net and workflow management => https://wwwis.win.tue.nl/~wvdaalst/publications/p53.pdf Let's focus on docs for now :) |
How does it conflict with my observations?
I won't say anything unless docs would provide wrong examples based on procedural approaches instead of object-oriented ones. |
$workflow = new Workflow($definition, $marking); | ||
|
||
The ``Workflow`` can now help you to decide what actions that are allowed | ||
on a blog post depending on what *place* it is in. This will keep your domain |
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 not about domain logic, it's more about Workflow management logic, isn'it ?
I'd say instead "This will keep your Workflow logic in one place and not spread all over your applications.", what do you think ?
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.
Interesting. I would always consider this the domain logic because the "workflow management logic" is a part of your domain.
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.
The theory behind "Workflow management" is to put the Workflow management outside of your domain, if I've well understood. BlogPost entity shouldn't be modified every time the rules of publishing evolve, for instance.
I'm pretty noob of this field, so correct me if I'm wrong :)
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.
BlogPost entity shouldn't be modified every time the rules of publishing evolve, for instance.
Correct, you just edit the workflow when the rules of publishing changes.
The theory behind "Workflow management" is to put the Workflow management outside of your domain, if I've well understood.
Outside the domain or outside your model? The way I see it there is only three places you can put code/config:
- In the framework
- In your domain
- The glue between your domain and the framework.
Can @lyrixx give some input?
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 to understand the question / debate here.
Basically, the the data are stored in the model, and you can put the places / transitions everywhere you want. I don't have a rule of thumb for that. IMHO, Simple things are always better than perfect code.
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 write an issue with a summary of your thoughts with pros and cons, I'll be happy to submit a PR for that issue.
I can't do it, because the primary purpose of this Workflow component is not obvious for me:
It would be nice to describe the purpose of the component more.
The component is designed as very generic thing, so in theory it probably may have some interesting applications. However, provided examples don't impress. Compare it with rich domain model:
final class Article
{
private $id;
private $title;
private $approvedByJournalist;
private $approvedBySpellchecker;
private $published;
public static function postDraft(int $articleId, ArticleTitle $title): Article
{
$draft = new self();
$draft->apply(new DraftArticleWasPosted($articleId, $title));
$draft->apply(new ArticleWasSentForApproval($this->id));
return $draft;
}
private function __construct() {}
public function approveByJournalist()
{
if ($this->approvedByJournalist) {
return;
}
$this->apply(new ArticleWasApprovedByJournalist($this->id));
$this->publishIfItIsReady();
}
public function approveBySpellchecker()
{
if ($this->approvedBySpellchecker) {
return;
}
$this->apply(new ArticleWasApprovedBySpellchecker($this->id));
$this->publishIfItIsReady();
}
public function changeTitle(ArticleTitle $newTitle)
{
if ($this->title->equals($newTitle)) {
return;
}
$this->apply(new ArticleTitleWasChanged($this->id, $newTitle));
$this->apply(new ArticleWasSentForApproval($this->id));
}
private function publishIfItIsReady()
{
if ($this->approvedByJournalist && $this->approvedBySpellchecker) {
$this->apply(new ArticleWasPublished($this->id));
}
}
// ...
private function onArticleWasApprovedByJournalist(ArticleWasApprovedByJournalist $event)
{
$this->approvedByJournalist = true;
}
private function onArticleWasSentForApproval(ArticleWasSentForApproval $event)
{
$this->approvedByJournalist = false;
$this->approvedBySpellchecker = false;
$this->published = false;
}
private function onArticleTitleWasChanged(ArticleTitleWasChanged $event)
{
$this->title = $event->getNewTitle();
}
}
We see use-cases (postDraft
, approveBySpellchecker
, approveBySpellchecker
, changeTitle
), events (ArticleWasApprovedByJournalist
, ..., ArticleTitleWasChanged
) and it's not hard to understand lifecycle of the Article
from this code. We lose this expressiveness.
Further, that sample project checks permissions with GuardEvent
hook:
public function onTransitionJournalist(GuardEvent $event)
{
if (!$this->checker->isGranted('ROLE_JOURNALIST')) {
$event->setBlocked(true);
}
}
... and how you can solve it with commands:
final class ApproveArticleByJournalistHandler
{
/**
* @acl_role ROLE_JOURNALIST
*/
public function handle(ApproveArticleByJournalist $command)
{
$blogPost = $this->blogPostRepository->find($command->getArticleId());
$blogPost->approveByJournalist();
$this->blogPostRepository->save($blogPost);
}
}
And again we lose expressiveness by generic GuardEvent
hook. It's harder to find it in the project, your have to search for string "workflow.article.guard.journalist_approval" instead of typing class name "ApproveArticleByJournalis...
".
I would like to know if I'm wrong.
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.
IMHO, all this debate is totally out of the scope of this PR.
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.
How is it possible to write docs for the component without clear applications, examples?
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 did not said that. But you you are telling us the Workflow component is useless and it's better to write code like your example. It's really a matter of taste (I have a strong opinion on that ; but I let people making their own choices). So I'm juste saying: The component is here, and it's useless to discuss if it's better to use CQRS / ES over Workflow Component.
For the record, the discussion is about the following sentence:
The workflow will keep your domain logic in one place and not spread all over your application
IMHO, it's more important to document how the component works.
I like the @Nyholm's work ; so let's focus on what matter.
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.
@unkind: I hear you. Your arguments are valid.
@mickaelandrieu: Your arguments are also valid.
This is, as Grégoire says, out of scope for this PR. @unkind, I've asked you to create an issue on the symfony/symfony repository. I am happy to make a PR to try to improve the Workflow component regarding your input. Let's work together on this, but not in this PR.
Massive thank you for the feedback. I've updated the PR accordingly. I've splited up the docs into multiple parts. One for the component, one for "usage" and one for dumping. What this what you had in mind @wouterj? That is great @unkind that you challenging this. I appreciate that. Though, Im not the author of the component Im happy if we discuss what the documentation and the examples in the documentations should say and not say (just as you have done so far). There are clearly right and wrong ways of using any component. I do agree with you that models should manage their own state. But when your models grows that could be a lot of messy code. Also, the workflows may have dependencies outside of the model. Say that you not should be allowed to publish when "[external event(boss is in the office?)]" is happening. Then it is a good thing to move this code outside of the model. I will continue this PR by filling in the placeholders (twig and state machines) |
Documented support for DefinitionBuilder
There you go. The docs does reflect the code in master |
{% endif %} | ||
|
||
{# Or loop through the enabled transistions #} | ||
{% for transition in workflow_transitions(article) %} |
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.
workflow_transitions(post)
instead of article
to be consistent with the whole document ?
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.
Thank you! I've updated the PR
$marking = new SingleStateMarkingStore('marking'); | ||
$stateMachine = new StateMachine($definition, $marking); | ||
|
||
|
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.
multiple blank lines should always be just one
|
||
.. code-block:: php | ||
|
||
use Symfony\Component\Workflow\Definition; |
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.
please add a comment at the top of the file:
// app/config/config.php
$definition = new Definition($states, $transitions, 'start'); | ||
|
||
$marking = new SingleStateMarkingStore('marking'); | ||
$stateMachine = new StateMachine($definition, $marking); |
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.
Actually, we need to show how to configure the DI extension config instead of initially building the workflow itself (same below).
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 correct. I'll fix this.
Thank you for your feedback
|
||
.. image:: /_images/components/workflow/simple.png | ||
|
||
Workflows could be more complicated when they describes a real business case. The |
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.
describe
.. image:: /_images/components/workflow/simple.png | ||
|
||
Workflows could be more complicated when they describes a real business case. The | ||
workflow below descirbes the process to fill in a job application. |
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.
describes
.. image:: /_images/components/workflow/job_application.png | ||
|
||
When you fill in a job application in this example there are 4 to 7 steps depending | ||
on the what job you are applying for. Some jobs requires personality test, logic tests |
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.
require personality tests
logic is not mixed with the controllers, models or view. The order of the steps can be changed | ||
by changing the configuration only. | ||
|
||
|
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.
two blank lines
|
||
.. note:: | ||
|
||
The ``dot`` command is a part of Graphviz. You can download it and read |
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.
is part of
commonly have cyclic path in the definition graph, but it is common for a state | ||
machine. | ||
|
||
Example of State Machine |
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.
of a
Thank you Christian |
rejected: | ||
from: review | ||
to: closed | ||
reopened: |
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.
What do you think about renaming the last four transitions to request_change
, accept
, reject
, and reopen
as we are describing actions and not events that just happened?
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 correct
👍 Great! Thank you for your patience, Tobias! |
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.
Thank you for your job on this and sorry for adding other comments on this PR. It's just some suggestions :)
A set of places and transitions creates a **definition**. A workflow needs | ||
a ``Definition`` and a way to write the states to the objects (i.e. an | ||
instance of a :class:`Symfony\\Component\\Workflow\\MarkingStore\\MarkingStoreInterface`). | ||
|
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 should also the initial place with something like: If a definition has no explicit initial place, it uses the first place defined.
instance of a :class:`Symfony\\Component\\Workflow\\MarkingStore\\MarkingStoreInterface`). | ||
|
||
Consider the following example for a blog post. A post can have one of a number | ||
of predefined statuses (`draft`, `review`, `rejected`, `published`). In a workflow, |
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 suggest to use drafted
and needs_review
instead of draft
and review
. That allows more readability, one may needs more statuses like reviewed
and needs_correction
. (I do :D). What about adding archived
too?
|
||
$definition = $builder->build(); | ||
|
||
$marking = new SingleStateMarkingStore('currentState'); |
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.
What about status
?
======== | ||
|
||
A workflow is a model of a process in your application. It may be the process | ||
of how a blog post goes from draft, review and publish. Another example is when |
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.
"drafted, reviewed and published"?
supports: | ||
- AppBundle\Entity\PullRequest | ||
places: | ||
- start |
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've started with many comments, so here a resume of what I suggest for the following:
pull_request:
type: 'state_machine'
supports:
- AppBundle\Entity\PullRequest
places:
- started
- coding
- travis
- needs_review
- reviewed
- merged
- closed
transitions:
test:
from: [started, coding]
to: travis
submit:
from: travis
to: needs_review
request_change:
from: [travis, needs_review, reviewed]
to: coding
accept:
from: needs_review
to: reviewed
reject:
from: [coding, needs_review, reviewed]
to: closed
reopen:
from: closed
to: needs_review
merge:
from: reviewed
to: merged
Wdyt?
Actually when trying to dump it I've found a bug and submitted symfony/symfony#20497 :)
marking_store: | ||
type: 'multiple_state' # or 'single_state' | ||
arguments: | ||
- 'currentPlace' |
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.
What about
arguments:
- 'status' # The name of property that holds the marking in the object
supports: | ||
- AppBundle\Entity\BlogPost | ||
places: | ||
- draft |
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.
- drafted # By default the initial state is the first place
- to_review
while renaming the transition 'submit' ?
public static function getSubscribedEvents() | ||
{ | ||
return array( | ||
'workflow.blogpost.guard.to_review' => array('guardReview'), |
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.
The method does not need to be in an array.
…abbuh) This PR was merged into the 3.2-dev branch. Discussion ---------- Added XML support for Workflow configuration | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#6871 Commits ------- 94a7e7e [Workflow] streamline XML schema definition 6381caa Added XML support for Workflow configuration
@@ -5,8 +5,8 @@ | |||
The Workflow Component | |||
====================== | |||
|
|||
The Workflow component provides tools for managing a workflow or finite state | |||
machine. | |||
The Workflow component provides tools for managing a workflow or finite |
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.
isn't a Petri net instead ?
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.
@mickaelandrieu a workflow is a subset of a petri net (and a state machine a subset of workflow). The component does not support all features necessary to implement a petri net
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 unclear to me right now, but it's not a problem in the docs: I just need to learn a little bit more about all of theses implementations ^^
Thank you @stof
Creating a Workflow | ||
------------------- | ||
|
||
The workflow component gives you an object oriented way to define a process |
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.
The more I read this, the more I think we should rename - at least in docs - Workflow
component to Workflow Engine
component. Am I crazy ? /c @Nyholm
Thank you Tobias. |
This PR was submitted for the master branch but it was merged into the 3.2 branch instead (closes #6871). Discussion ---------- Added docs for Workflow component This will fix #6677 I want some feedback: - Is there any more features I should cover? - Do you like my blog post project? Im not happy with the terminology of the classes in this component. Ie "places", "Marking" and "GuardEvent". But I'll make sure to open a separate issue about that. Commits ------- ba49091 Added docs for Workflow component
I decided to merge this PR now as Symfony 3.2 was released and @Nyholm delivered a great talk today at SymfonyCon in Berlin. So people will probably want to read some documentation for the new component. Though I left a comment in #6677 so that we do not forget to talk about the latest comments here. |
This will fix #6677
I want some feedback:
Im not happy with the terminology of the classes in this component. Ie "places", "Marking" and "GuardEvent". But I'll make sure to open a separate issue about that.