[Workflow] Allow use of UnitEnum and BackedEnum in workflow places #44306
Conversation
f498bb5 to
48616a3
Compare
7b8b394 to
632cd5b
Compare
IIRC, those nbToken are useful for non-state-machine workflows, which can have multiple places. |
There was a problem hiding this comment.
Let's say, I'd create an enum soley for the purpose of having a closed declaration fo all possible places of a workflow. Why would I want to enumerate the enumeration again?
Istead of declaring…
new Definition([Suit::Hearts, Suit::Diamonds, Suit::Clubs, Suit::Spades], …);… wouldn't I want to do this instead?
new Definition(Suit::class, …);| public static function getTypedValue(string $place): string|\UnitEnum | ||
| { | ||
| try { | ||
| return \constant($place); |
There was a problem hiding this comment.
should we look for :: in the string first to avoid trying to load constants in such case ?
There was a problem hiding this comment.
Looking at this class, I really wonder if we shouldn't limit this feature to backed enums. If a developer does not want us to serialize an enum they declared, we should not guess how to serialize it anyway.
There was a problem hiding this comment.
At first I thought of only add support of BackedEnums and being able to use BackedEnum::tryFrom but in any case, I've been confronted to the problem that in the marking store, we don't really have any way to guess which enumeration we should use to tryFrom 😕
There was a problem hiding this comment.
which enumeration we should use
How many enumerations should a user be allowed to use for a single workflow? Should they be allowed to mix and match values from different enums or even enum values and scalars?
There was a problem hiding this comment.
It's a choice I had a hard time making 😅 I'm torn between "we can do it, why limit ourselves" and "it doesn't make sense to allow 3 different enumerations and scalars for a workflow". So I proposed this PR by accepting the most permissive solution, but if you feel that this is not the right option, I will gladly make the changes 😄
| public static function getTypedValue(string $place): string|\UnitEnum | ||
| { | ||
| try { | ||
| return \constant($place); |
There was a problem hiding this comment.
What about strings that may look like something that is a constant name in something else than an enum ? String place names can be anything.
There was a problem hiding this comment.
You're right, I'll add a check to be sure that if \constant doesn't throw an exception, we're dealing with an enumeration. 👍
32398a8 to
d4e033f
Compare
d4e033f to
06e8b23
Compare
|
I started to review the PR, but the lot of changes scares me :) I raised some consideration in the issue ; let's continue the discussion there |
|
It's always heartbreaking to refuse a PR, but we agree with @alexandre-daubois (PR author) that it does not worth it. Thanks again for the hard work, and sorry for the waste of time. |
|
This would be better solved at the engine-level, see php/php-src#8825 |
ℹ️ Meant to be merged on 6.1, waiting for the branch to open.
One thing should be noted here. Currently, subject marking is done with the following form:
['place_a' => 1, 'place_b' => 1], which is not possible to reproduce with enumerations. Indeed, you're not allowed to use enumeration cases as array keys.That's why, when using enumerations, subject marking is stored using a simple list of cases, like
[FooEnum::Bar, FooEnum::Baz].I think this solution is ok, as it seems
$nbToken(the 1 affected to places in marking) seems to not be used anywhere at this time.