Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Workflow] Add a MetadataStore to fetch some metadata#26092
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
lyrixx commentedFeb 8, 2018
Note: This PR is far from finished. I want to gather feedback before. |
nicolas-grekas commentedFeb 8, 2018
IMHO, this PR makes sense. A Place class would introduce a complex BC/FC layer, and create another issue: the strict equality operator to compare object would not work anymore, forcing to compare places using a method (eg getName()) - thus adding boilerplate. The current design is not stupid, extending should require refactoring it - and in fact it doesn't :) |
ostrolucky commentedFeb 8, 2018 • 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.
Don't have time to review this now, but just want to say that triple equal operator issue is same issue php enumeration libraries face and can be solved by replacing constructor with a factory method which caches instances in a private property and returns same instance for place which has already been instantiated before. This guarantees there cannot exist multiple instances for same place. |
lyrixx commentedFeb 9, 2018
Okay, the PR is ready ;) |
UPGRADE-4.1.md Outdated
| * Deprecated`SupportStrategyInterface` in favor of`WorkflowSupportStrategyInterface`. | ||
| * Deprecated the class`ClassInstanceSupportStrategy` in favor of the class`InstanceOfSupportStrategy`. | ||
| * Deprecated passing the workflow name as 4th parameter of`Event` constructor in favor of the workflow itself. | ||
| *[BCBREAK] The configuration of a place in XML has changed: |
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.
can't we have a BC/FC XSD?
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 was not able to do that. My skill on with XSD are too limited. But I search and I could not find a way because the XML would become un-deterministic. But as I said, I really really doubt someone is using XML to configure workflows. I'm pretty sure it's dead code ;)
But anyway, If someone could give me the XSD to matche both format, it would be nice.
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 if the place name was either configured using the attribute or the node content? This could be handle in the extension, right?
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.
Sorry I missed your comment. It might be possible, but it will be hard and it will add many code for almost nothing. I know nobody defining its workflow in XML. Obviously I don't know every workflow dev. But I really think it does not worth it.
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 removed the BC Break \o/
| */ | ||
| publicfunctiongetMetadata($subject,string$key,$metadataSubject =null,$name =null) | ||
| { | ||
| // dump( |
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.
booo
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.
Oups, Fixed ;)
7ad3c3d to2eedbcaComparelyrixx commentedFeb 15, 2018
This PR is ready. |
b9e048d to2ec8dfeComparelyrixx commentedFeb 16, 2018
I would like to merge this PR today (because it's open for a week now), so I'm calling for a review :) Thanks |
ostrolucky 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.
Thank you for working on this 👍
| return$this->workflow; | ||
| } | ||
| publicfunctiongetWorkflowName() |
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.
Maybe deprecate this method, since there is getWorkflow() now?
lyrixxFeb 22, 2018 • edited by nicolas-grekas
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by nicolas-grekas
Uh oh!
There was an error while loading.Please reload this page.
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 don't think it worth it. Deprecation is always boring for end user. Here it does not harm to keep this method.
| * | ||
| * @author Grégoire Pineau <lyrixx@lyrixx.info> | ||
| */ | ||
| finalclass MetadataBag |
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's the rationale of having this over plain array? So far all I see are disadvantages. No count(), no all(), no iterator, inability to use array_* functions, cannot serialize by default.. And also impossible to add such methods in user space, as this is final class. I'm big 👎 against creating dependencies on final classes.
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 started with a simple array, then I thought I will be cleaner to have a class. And finally I added the final keyword in order to be able to update the code with ease (BC layer...)
Here is the rational ;)
I understand your point of view and I think I will remove this class ;)
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 mean it's good for typehinting for sure. My main annoyance is that user is stuck with very limited stock functionality with no way to extend it. If you decide to still have it instead of array, please at least mark it@final instead of making itfinal, so userspace can still extend it on its own risk and meanwhile report his use case for us to consider opening it up, instead of having his hands tied. This is standard way in symfony codebase. See#25788
| * Use a string (the place name) to get place metadata | ||
| * Use a Transition instance to get transition metadata | ||
| */ | ||
| publicfunctiongetMetadata(string$key,$subject =null); |
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's the rationale of having this? This is unnecessary all-in-one magic method, all of which operations can be replaced by other methods. And it's even required by interface. It's just helper method, I don't see why is it required?
ostroluckyFeb 18, 2018 • 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.
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.
Okay, I can see how this is useful now. This serves as workaround for not having Ẁorkflow::getMetadata(), Transition::getMetadata() and
Place::getMetadata(). This is useful e.g. when having only list of places retrieved viadefinition.places for retrieving their metadata in same loop, such as
{%forplaceinfsm.definition.places %} <tr> <td> {{place }} </td> <td> {{ workflow_metadata('description',place) }} </td> </tr>{%endfor %}instead of doing
{%forplaceinfsm.definition.places %} <tr> <td> {{place }} </td> <td> {{fsm.definition.placeMetadata(place).get('description') }} </td> </tr>{%endfor %}Not sure if I like it, but I see the rationale. What do other people 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.
It's much much easier to use. More over is was asked in the initial issue.
I will keep it fore sure.
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.
Fine by me, I can see it's useful now
| private$placesMetadata; | ||
| private$transitionsMetadata; | ||
| publicfunction__construct(MetadataBag$workflowMetadata =null,array$placesMetadata =null,\SplObjectStorage$transitionsMetadata =null) |
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.
array $placesMetadata = array(), then you don't need do ternary
lyrixx commentedMar 19, 2018
I just rebased this PR. It's ready. I would like to merge it this week. |
1d109dc to3481e18Compare5e43142 to4663c22Comparelyrixx commentedMar 21, 2018
@fabpot Thanks for your review. I have addressed your comments |
UPGRADE-4.1.md Outdated
| <framework:place>first</framework:place> | ||
| after: |
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.
do we need before/after? looks like a leftover of the previous deprecation, isn't it?
| * Use a string (the place name) to get place metadata | ||
| * Use a Transition instance to get transition metadata | ||
| */ | ||
| publicfunctiongetMetadata($subject,string$key,$metadataSubject =null,string$name =null): ?string |
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.
?string (no space)
| },$places); | ||
| } | ||
| // It's an indexed array, we let the validation occurs |
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.
occur (no "s")
lyrixx commentedMar 21, 2018
Thanks@nicolas-grekas for the review. I have addressed your comments. |
fabpot commentedMar 21, 2018
Thank you@lyrixx. |
…(lyrixx)This PR was merged into the 4.1-dev branch.Discussion----------[Workflow] Add a MetadataStore to fetch some metadata| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | yes (little)| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#23257| License | MIT| Doc PR | TODO---This is an attempt tofix#23257. I first started to implement`Ẁorkflow::getMetadata()`, `Transition::getMetadata()` and`Place::getMetadata()`. **BUT**, there are no `Place` class. For now it's just a`string`. So dealing with BC is a nightmare.So I tried to find another way to fix the issue. [Thiscomment](#23257 (comment))summary well the two options. But this PR is (will be) a mix of theses 2options.First it will be possible to configure the workflow/metadata like this:```yamlblog_publishing: supports: - AppBundle\Entity\BlogPost metada: label: Blog publishing description: Manages blog publishing places: draft: metadata: description: Blog has just been created color: grey review: metadata: description: Blog is waiting for review color: blue transitions: to_review: from: draft to: review metadata: label: Submit for review route: admin.blog.review```I think is very good for the DX. Simple to understand.All metadata will live in a `MetadataStoreInterface`. If metadata are set viathe configuration (workflows.yaml), then we will use the`InMemoryMetadataStore`.Having a MetadataStoreInterface allow user to get dynamic value for a place /transitions. It's really flexible. (But is it a valid use case ?)Then, to retrieve these data, the end user will have to write this code:```phppublic function onReview(Event $event) { $metadataStore = $event->getWorkflow()->getMetadataStore(); foreach ($event->getTransition()->getTos() as $place) { $this->flashbag->add('info', $metadataStore->getPlaceMetadata($place)->get('description')); }}```Note: I might add some shortcut to the Event classor in twig:```jinja{% for transition in workflow_transitions(post) %} <a href="{{ workflow_metadata_transition(post, route) }}"> {{ workflow_metadata_transition(post, transition) }} </a>{% endfor %}```---WDYT ?Should I continue this way, or should I introduce a `Place` class (there will beso many deprecation ...)Commits-------bd1f2c8 [Workflow] Add a MetadataStore
…(vudaltsov)This PR was squashed before being merged into the 4.1 branch (closes#27190).Discussion----------[Workflow] Added DefinitionBuilder::setMetadataStore().| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |This PR complements#26092.Commits-------2882f8d [Workflow] Added DefinitionBuilder::setMetadataStore().
This PR was squashed before being merged into the 4.3-dev branch (closes#29538).Discussion----------[Workflow] Add colors to workflow dumpsFixes#28874| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#28874, replaces#28933| License | MIT| Doc PR | TODO, requiressymfony/symfony-docs#9476Fetch data with the `MetadataStore` from#26092 in order to add colors to the dumps.Example of configuration:```yaml transitions: submit: from: start to: travis metadata: title: transition submit title dump_style: label: 'My custom label' arrow_color: '#0088FF' label_color: 'Red'```This code was developed as a bundle, examples can be found on its repository:https://github.com/alexislefebvre/SymfonyWorkflowStyleBundleCommits-------60ad109 [Workflow] Add colors to workflow dumps
…(derrabus)This PR was merged into the 6.4 branch.Discussion----------[TwigBridge] Remove obsolete Workflow feature detection| Q | A| ------------- | ---| Branch? | 6.4| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | N/A| License | MIT| Doc PR | N/AThe classes and methods that are detected here have been introduced with#24751 and#26092 (both Symfony 4.1)Commits-------7e9d5bb [TwigBridge] Remove obsolete Workflow feature detection
Uh oh!
There was an error while loading.Please reload this page.
This is an attempt tofix#23257. I first started to implement
Ẁorkflow::getMetadata(),Transition::getMetadata()andPlace::getMetadata().BUT, there are noPlaceclass. For now it's just astring. So dealing with BC is a nightmare.So I tried to find another way to fix the issue.This
comment
summary well the two options. But this PR is (will be) a mix of theses 2
options.
First it will be possible to configure the workflow/metadata like this:
I think is very good for the DX. Simple to understand.
All metadata will live in a
MetadataStoreInterface. If metadata are set viathe configuration (workflows.yaml), then we will use the
InMemoryMetadataStore.Having a MetadataStoreInterface allow user to get dynamic value for a place /
transitions. It's really flexible. (But is it a valid use case ?)
Then, to retrieve these data, the end user will have to write this code:
Note: I might add some shortcut to the Event class
or in twig:
WDYT ?
Should I continue this way, or should I introduce a
Placeclass (there will beso many deprecation ...)