-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[FrameworkBundle] Add support for configuring workflow places with glob patterns matching consts/backed enums #60204
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
Conversation
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.
I'd like one more step: when this is used, the marking store should pass the enum instance to the object instead of the string.
|
||
enum Places: string | ||
{ | ||
case A = 'a'; |
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.
In another PR, I want add to introduce an attribute to configure the Metadata. What would be the name of such attribute? #[WorkflowMetadata]
/ #[AsWorkflowMetadata]
?
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.
just #[Place]
?
Of course! I commented the @tucksaun PR! |
Maybe it's a naive question: would it be hard to also support UnitEnums? Recent implementations to support enums in workflow seem way simpler than what I tried a few years ago, so maybe UnitEnum wouldn't be a big deal now? |
I think it would be hard. And with unit enum, i fail to see what you save in your db |
Thinking about it, I'll need to rework a bit the PR. We'll need to store the fact it's an enum in the configuration, to be able to configure the marking store. ATM, we convert the enum in the configuration class, and it's too early and we loose the information |
@lyrixx nice work! This is similar to something I quickly tried and it seemed to work well.
Do we? I mean, we have to if we want to enforce integrity end-to-end. So does that mean we want to go with an enum-dedicated marking store that we configure automatically when one uses an enum like proposed here? |
I think so yes: when places is configured to an enum class, we should only support storing enums. |
src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/workflow_enum.php
Show resolved
Hide resolved
@lyrixx don't miss this part ;) |
Hello everyone ! I have been following this "Enum support" topic for a while since I often use the Workflow component. One argument someone gave me against enum support is that using an enum as closed list of places does not allow to extend it. I really like this PR for what it brings: improve DX without reducing possibilities. I'd like to point out a possible issue: what happen for workflows (not state machines) with multiple states at a time ? enum PlacesA: string
{
case PlaceOne = 'place_one';
case PlaceTwo = 'place_two';
}
enum PlacesB: string
{
case PlaceStart = 'place_one';
case PlaceEnd = 'place_two';
}
$marking = new Marking();
$marking->mark(PlacesA::PlaceOne->value); // $marking->getPlaces() = ['place_one' => 1]
$marking->mark(PlacesB::PlaceStart->value); // $marking->getPlaces() = ['place_one' => 2] It's not possible since the subject must return a valid marking representation (like ['string' => 1, '...']). WDYT ? |
BackedEnum
13f32e1
to
d5a5dfc
Compare
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 updated the PR (and its description, have a look) to:
- allow using FWCN::glob patterns to list places - works with enums and consts
- allow using backed enums to list places and reference them in transitions
…ob patterns matching consts/backed enums
d5a5dfc
to
72dd914
Compare
Thank you @lyrixx. |
This PR improves the DX on two aspects:
Example with consts:
and
Example with enums:
and