Support Enum as workflow places #44211
Comments
|
Looking into it. 👍 |
|
@sxsws what you are suggesting here is not enum support for the workflow component. In your proposal, place names are still arbitrary strings, meaning that there is no benefit of using an enum for that at all. |
|
and what then pray tell is the difference then between an enum and a class of constants for workflow? |
|
@sxsws this ticket is precisely about investigating native support for enums in the workflow component (using an enum for transition name and an enum for place names, or something like that) instead of relying on arbitrary strings (which can be stored as class constants, but without type safety about avoiding to use other values) |
|
It's pretty challenging to bring native support to workflow component. This PR (still a WIP) is about bringing support to places only, not transition name. But that could be another improvement! The PR is already pretty big now 😄 |
|
@stof if necessary, it can easily be determined if a constant belongs to a class. |
|
Here is the pull-request: #44306 |
|
Hello, I look at the PR, and there is many changes and I wonder if it really worth it. I read #44203 and there is something that can not work with the inital discussion. For a state machine, one could store the the current state in a property typed https://github.com/symfony/symfony/discussions/44203Enum`. But this could not work with a Workflow since the the marking can contains many places. So it has to be a collection. Even if colored pretri net are not fully implemented, the need for store many token in one place make the use of enum even harder. I understand some people want to use enum with state machine, but instead of #44306 (that does not work with workflow) I would implement a new MarkingStoreInterface that convert a string to a marking. WDYT? |
|
Hi Grégoire! Yeah, honestly I agree with you. I really love your solution of implementing MarkingStoreInterface. I think this is too much for a feature that doesn't seem to be an excessive need. Also, that's the whole point of the framework: being flexible enough to implement this type of use-case easily by implementing offered interfaces. So that's a +1 for me to abandon this PR. 👍 |
|
I'm closing this issue because adding a native support for enum in the workflow component does not worth it and cannot work anyway in all situation. However, we may accept a PR with a new marking store that work only with a state machine. Thanks everyone involved in this discussion. |
|
Would like to know the reasoning a bit more about why it isn't possible. For now I've had to do: Would be nice if we could use a string backed enum for the transitions instead of calling |
|
Having to deal to enum (backed or not) or string at the same time add a huge amount of code, and I think it does not worth it.
is this a real concern? I never hit this issue. You can use enum like you said, of PHP constant to mitigate that |
Why close the issue then? :) I thought at first look that it was resolved as won't fix forever (which was a bit disturbing) |
|
Transition and state names are always constant so they must be referenceable/tracable. People are now using php enums for the states of state machines, so please wake up from your 2010 mentality and accept the fact that the way people do coding changes. Having hardcoded strings laying around anywhere is just sloppy. Enums for workflows should be first class citizens and strings should not be used anymore. Okay maybe I am a bit harsh and there are cases where usage of strings instead of enums are better, but that's only because of my frustration because this issue was closed. Edit: <exhales...> friggin boomers... |
|
@sakalys we're simply having a technical discussion in this issue and are weighing the costs of maintenance to the benefits. If we were to write the Workflow component from scratch now, we surely would have used enums. But an existing library brings with it new challenges and sometimes you need to stick to a less-ideal scenario. In any case, all that we're doing here is having a technical discussion about enums and the Workflow component. Please keep it technical and refrain from classifying people as "having a 2010 mentality", "boomers", etc. |
|
The issue is closed sir, and all of the peoples recurring requests for this are being linked to this issue and being closed as duplicate. There's no discussions here. Just a closed ticket, because some member didn't like it. |
Again, we're staying fully technical here. There is no liking or disliking on usings enums for places, but there is a maintenance cost consideration. With the current insights, it looks like the people in here agree that the extra maintenance costs are not worth it. I'm pretty sure we all can see the benefit of the end-situation for users (and the amount of issues about this confirms it). But what we are missing is a solution on how to get there. If you have new insights on how to get there, please tell us and we can reconsider the situation (in which case we would reopen the issue). This happens often in "less than ideal" situations like this, when a new PHP feature or extra experience gives someone a bright idea on how to fix it and we manage to implement the feature request in e.g. Symfony 8. |
|
Fine. I appreciate the diplomatic attitude of yours @wouterj. |
Well, you can use enum in your workflows: https://www.strangebuzz.com/en/blog/using-php-enumerations-with-your-symfony-workflows |
|
I also posted an alternate solution here, for the record: #54582 (comment) |
|
@alexandre-daubois thanks for sharing, this is definitely usable! |
| Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Kind of relates to symfony#44211 | License | MIT Supporting Enums in Workflow (and overall in Symfony) is a highly debated topic. While using Enums is not always a good choice, I personally found that Backed Enums are really suited for status and places because they ensure type safety in your model and naturally validate the set of allowed values. This is why I think supporting them in Workflow could be nice if not too complicated. While trying to implementing this in my current project using custom code, I figured we could go with a really narrow scope to support them out-of-the-box (but only for Single State Machine): in such case only `MethodMarkingStore` has to support Enums. In my mind, the way the workflow uses strings internally in the `Marking` is an implementation detail and what really matters to users is being able to control the values stored in their objects. Also, I don’t believe supporting enums for transitions is necessary as they are mostly configuration and validating transitions is already part of the component’s job (and in such case I would recommend using constants anyway). Finally, supporting Enum values in the configuration is not a target at the moment either because it can already be done as one can use `!php/enum` if they are using yaml files or use PHP config and directly use the enums. And improving this experience can be done later on if deemed necessary. Supporting this use case currently requires some boilerplate in the user project (with a dedicated getter for instance). Or completely reimplementing a marking store. Therefore I’m sharing what is in my mind a small patch that can greatly improve the situation for users as it allows them to use Backed Enums in their model right away with their getters (ideally we would not need this patch but as PHP will not allow Enums to implement `\Stringable` soon we need to do it ourself). The complicated part could be the setters but now that PHP supports union types we can only document how users can support them: ```php class MyObject { // ... private ObjectStatus $status = ObjectStatus::Draft; public function getStatus(): ObjectStatus { return $this->status; } public function setStatus(ObjectStatus|string $status): static { if (\is_string($status)) { $status = ObjectStatus::from($status); } $this->status = $status; return $this; } } ```
| Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Kind of relates to symfony#44211 | License | MIT Supporting Enums in Workflow (and overall in Symfony) is a highly debated topic. While using Enums is not always a good choice, I personally found that Backed Enums are really suited for status and places because they ensure type safety in your model and naturally validate the set of allowed values. This is why I think supporting them in Workflow could be nice if not too complicated. While trying to implementing this in my current project using custom code, I figured we could go with a really narrow scope to support them out-of-the-box (but only for Single State Machine): in such case only `MethodMarkingStore` has to support Enums. In my mind, the way the workflow uses strings internally in the `Marking` is an implementation detail and what really matters to users is being able to control the values stored in their objects. Also, I don’t believe supporting enums for transitions is necessary as they are mostly configuration and validating transitions is already part of the component’s job (and in such case I would recommend using constants anyway). Finally, supporting Enum values in the configuration is not a target at the moment either because it can already be done as one can use `!php/enum` if they are using yaml files or use PHP config and directly use the enums. And improving this experience can be done later on if deemed necessary. Supporting this use case currently requires some boilerplate in the user project (with a dedicated getter for instance). Or completely reimplementing a marking store. Therefore I’m sharing what is in my mind a small patch that can greatly improve the situation for users as it allows them to use Backed Enums in their model right away with their getters (ideally we would not need this patch but as PHP will not allow Enums to implement `\Stringable` soon we need to do it ourself). The complicated part could be the setters but now that PHP supports union types we can only document how users can support them: ```php class MyObject { // ... private ObjectStatus $status = ObjectStatus::Draft; public function getStatus(): ObjectStatus { return $this->status; } public function setStatus(ObjectStatus|string $status): static { if (\is_string($status)) { $status = ObjectStatus::from($status); } $this->status = $status; return $this; } } ```
Discussed in #44203
Originally posted by stephanvierkant November 22, 2021
I've tried the new Enum in PHP 8.1 and I love that Symfony supports this new feature in the Form component.
I'd love to have this support in the Workflow component as well.
I've changed my status attribute from a string to an Enum field and now this doesn't work anymore:
$workflow->can($entity, $transition->getName()I'd be great is Symfony would support Enum in the Workflow Component as well!
The text was updated successfully, but these errors were encountered: