-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[FrameworkBundle] Add support for configuring workflow places with a BackedEnum
#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
base: 7.4
Are you sure you want to change the base?
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]
?
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. |
'from' => Places::A->value, | ||
'to' => Places::B->value, |
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.
'from' => Places::A->value, | |
'to' => Places::B->value, | |
'from' => Places::A, | |
'to' => Places::B, |
In a following 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.
May be 🤞🏼
@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 ? |
Examples:
and