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] 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

Merged
fabpot merged 1 commit intosymfony:masterfromlyrixx:workflow-metadata
Mar 21, 2018

Conversation

@lyrixx
Copy link
Member

@lyrixxlyrixx commentedFeb 8, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?yes
Deprecations?yes
Tests pass?yes
Fixed tickets#23257
LicenseMIT
Doc PRTODO

This is an attempt tofix#23257. I first started to implement
Ẁorkflow::getMetadata(),Transition::getMetadata() and
Place::getMetadata().BUT, there are noPlace 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.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:

blog_publishing:supports:        -AppBundle\Entity\BlogPostmetada:label:Blog publishingdescription:Manages blog publishingplaces:draft:metadata:description:Blog has just been createdcolor:greyreview:metadata:description:Blog is waiting for reviewcolor:bluetransitions:to_review:from:draftto:reviewmetadata:label:Submit for reviewroute:admin.blog.review

I think is very good for the DX. Simple to understand.

All metadata will live in aMetadataStoreInterface. If metadata are set via
the 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:

publicfunctiononReview(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 class

or in twig:

{%fortransitioninworkflow_transitions(post)%}    <ahref="{{ workflow_metadata_transition(post, route) }}">         {{ workflow_metadata_transition(post, transition) }}   </a>{%endfor%}

WDYT ?

Should I continue this way, or should I introduce aPlace class (there will be
so many deprecation ...)

@lyrixx
Copy link
MemberAuthor

Note: This PR is far from finished. I want to gather feedback before.

ping@ostrolucky@userdude

@nicolas-grekas
Copy link
Member

should I introduce a Place class

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
Copy link
Contributor

ostrolucky commentedFeb 8, 2018
edited
Loading

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
Copy link
MemberAuthor

Okay, the PR is ready ;)

@nicolas-grekasnicolas-grekas added this to the4.1 milestoneFeb 9, 2018
* 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:

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?

Copy link
MemberAuthor

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.

Copy link
Member

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?

Copy link
MemberAuthor

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.

Copy link
MemberAuthor

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(

Choose a reason for hiding this comment

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

booo

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Oups, Fixed ;)

@lyrixxlyrixxforce-pushed theworkflow-metadata branch 3 times, most recently from7ad3c3d to2eedbcaCompareFebruary 15, 2018 18:09
@lyrixx
Copy link
MemberAuthor

This PR is ready.

@lyrixxlyrixxforce-pushed theworkflow-metadata branch 2 times, most recently fromb9e048d to2ec8dfeCompareFebruary 16, 2018 09:47
@lyrixx
Copy link
MemberAuthor

I would like to merge this PR today (because it's open for a week now), so I'm calling for a review :) Thanks
(cc /@stof@nicolas-grekas@Nyholm@xabbuh )

walva reacted with thumbs up emoji

Copy link
Contributor

@ostroluckyostrolucky left a 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()
Copy link
Contributor

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?

Copy link
MemberAuthor

@lyrixxlyrixxFeb 22, 2018
edited by nicolas-grekas
Loading

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
Copy link
Contributor

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.

Copy link
MemberAuthor

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

Copy link
Contributor

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);
Copy link
Contributor

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?

Copy link
Contributor

@ostroluckyostroluckyFeb 18, 2018
edited
Loading

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?

Copy link
MemberAuthor

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.

Copy link
Contributor

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)
Copy link
Contributor

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 reacted with thumbs up emoji
@lyrixx
Copy link
MemberAuthor

I just rebased this PR. It's ready. I would like to merge it this week.

@lyrixxlyrixxforce-pushed theworkflow-metadata branch 3 times, most recently from1d109dc to3481e18CompareMarch 20, 2018 14:09
@lyrixxlyrixxforce-pushed theworkflow-metadata branch 2 times, most recently from5e43142 to4663c22CompareMarch 21, 2018 09:40
@lyrixx
Copy link
MemberAuthor

@fabpot Thanks for your review. I have addressed your comments


<framework:place>first</framework:place>

after:

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

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

Choose a reason for hiding this comment

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

occur (no "s")

@lyrixx
Copy link
MemberAuthor

Thanks@nicolas-grekas for the review. I have addressed your comments.

@fabpot
Copy link
Member

Thank you@lyrixx.

@fabpotfabpot merged commitbd1f2c8 intosymfony:masterMar 21, 2018
fabpot added a commit that referenced this pull requestMar 21, 2018
…(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
@lyrixxlyrixx deleted the workflow-metadata branchMarch 21, 2018 10:20
@fabpotfabpot mentioned this pull requestMay 7, 2018
fabpot added a commit that referenced this pull requestMay 11, 2018
…(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().
fabpot added a commit that referenced this pull requestMar 19, 2019
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
fabpot added a commit that referenced this pull requestSep 10, 2023
…(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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xabbuhxabbuhxabbuh left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@ostroluckyostroluckyostrolucky requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

[Workflow] Allow to define arbitrary data for states/transitions

6 participants

@lyrixx@nicolas-grekas@ostrolucky@fabpot@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp