-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Add BackedEnums support in Symfony Workflow Marking Store #54582
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
Comments
Can you elaborate why this is a bug? Do we document or advertise the usage of enums in that context anywhere? |
I am not aware of documentation or advertisement about the usage of enums in this context, but I consider it a bug in the terms of supporting a native element with a string declaration. Since Doctrine supports the usage of Backed Enums, and EasyAdmin follows up on this, it does seem to fall out of order. I do consider this to be a fault on the PHP side, as one would expect Anyway, if this is a Thank you for your response! |
Not a bug then. 😃
Well, enums are not strings in PHP and that's by design. We should not try to fix something that's not broken. We've decided against silently accepting enums where a string is expected in other places and I think we've had a discussion around the Workflow component as well, but I can't seem to find it. Maybe @lyrixx knows more. |
Agreed!
I understand this decision in general regarding |
The thing is, accepting enums where a string is expected is a slippery slope. It will lead to more requests and PR stating, "but you allow enums in X, shouldn't they be allowed in Y as well?". And eventually that codebase is cluttered with But that is a very generic statement. I'm not familiar enough with the workflow component to have an informed opinion on whether allowing enums makes sense in your particular case. |
I understand your statement and reluctance to allow for a mix of parameter types. Where this case differs, in my opinion, is that it is not an implementation of The I want to sum up the reasons why I am convinced that the workflow component should support the
I am convinced this is a step forward and does not open the door for implementations on I hope that the above reasons/arguments are enough to agree upon a reconsideration of the team's stand for introducing Thank you for your time and consideration, I hope that you, @lyrixx, or anyone else with more insight can help us find the best outcome for this (even if this means no support, I am grateful to have this opportunity to discuss it). P.s.: I changed the name from |
Not exactly. BackedEnum cases have a scalar equivalent. But a BackedEnum is not equivalent to a scalar.
You cannot implicitly cast a backedenum to a string, and an Enum class cannot implements Stringable. enum Suit: string
{
case Hearts = 'H';
case Diamonds = 'D';
case Clubs = 'C';
case Spades = 'S';
}
$suit = Suit::Hearts;
echo (string) $suit; This code will throw a Fatal Error "Object of class Suit could not be converted to string" |
Correct, their cases hold a scalar type. But they are limited to either string/int. "A Backed Enum may be backed by types of int or string, and a given enumeration supports only a single type at a time (that is, no union of int|string)."
This is true, but the proposed solution takes that into account and uses the native support for retrieving the value of the case. Which is documented "Backed Cases have an additional read-only property, value, which is the value specified in the definition."
This is also the problem that I wish to solve with this feature request. Also the proposed solution works flawlessly. <?php
enum Suit: string
{
case Hearts = 'H';
case Diamonds = 'D';
case Clubs = 'C';
case Spades = 'S';
}
$suit = Suit::Hearts;
var_dump((string) $suit->value); // throws no error |
No. That makes zero difference.
No. Backed enums have a scalar representation, but they're not scalar equivalents. They're objects. |
You are all correct, BackedEnums are objects that hold scalar types, I wrote this incorrectly. They hold native support for retrieving their value though. What about the rest of my arguments for implementation? |
I've found the discussion I was looking for: #44211. Please have a look at that discussion, especially at @lyrixx' assessment and conclusion.
Ex falso quodlibet. Since you made most of your arguments based on your false assumption, I didn't want to dissect them one by one. 😓
If I may make that observation: You're pretty late to the party as PHP 8.1 is approaching EOL.
I'm not buying this argument, sorry. Yes, Doctrine ORM and Symfony Form (and thus EasyAdmin) are able to hydrate enums, but that really doesn't mean we have to use them for everything. I do see why enums seem like a good fit for the workflow component: A workflow has a finite set of states just like an enum does. Makes sense. And I'm pretty sure that if the Workflow component were designed today, it would be designed around enums or at least with enums in mind. And if you come up with a smart implementation, it will surely be considered. But trust me, it'll be more complicated than changing three lines of code. If you want to work on the topic, I don't want to stop you. I'm merely doing expectation management here. 🙂 |
My2cents: before enums we used constants. Now that we have backed enuns, it seems to me to be a useless verbosity being forced to get the scalar value from the enum when it could be handled internally. I understand it may lead to similar requests in other places and, in my opinion, I think it should be supported also in other places. Enums are really helpful to have a closed set of possible values but the fact they are not converted automatically by php to their scalar value is really noisy. I would also like to use them, for example, as keys in arrays: it would be really helpful while now I continue to be forced to use constants or to use ‘->value’. |
This feature is requested every now and then actually. I even tried to implement this myself. As @derrabus said, it is not a "three lines update" at all. I would surely do things differently from what I did two years ago here: #44306, but I can definitely say that it requires profound changes in the code. It was actually pretty complicated to get this right. After a few discussion with @lyrixx, he convinced me that this was actually a bad idea because of the nature of the component itself. It actually doesn't really make sense, we did agree here. The TL;DR is that enums are used to represent a finite set of values in a context and workflow are already doing exactly this by limiting possible places and transitions to the ones declared in the configuration. Among other things, implementing this in state machines could be straightforward, but things are getting over complicated when it comes to workflows where multiple states are possible at the same time. For the record, I did implement a marking store for an app to use enums if you really want to use them: class EnumReflectionMarkingStore implements MarkingStoreInterface
{
public function __construct(protected readonly string $property)
{
}
#[\Override]
public function getMarking(object $subject): Marking
{
$method = 'get'.ucfirst($this->property);
if (!method_exists($subject, $method)) {
throw new LogicException(sprintf('The method "%s::%s()" does not exist.', \get_debug_type($subject), $method));
}
if (!($v = $subject->{$method}()) instanceof \BackedEnum) {
throw new LogicException(sprintf('The method "%s::%s()" must return an enum.', \get_debug_type($subject), $method));
}
/** @infection-ignore-all */
return new Marking([$v->value => 1]);
}
/**
* @param array<string, mixed> $context
*/
#[\Override]
public function setMarking(object $subject, Marking $marking, array $context = []): void
{
$method = 'set'.ucfirst($this->property);
if (!method_exists($subject, $method)) {
throw new LogicException(sprintf('The method "%s::%s()" does not exist.', \get_debug_type($subject), $method));
}
$r = new \ReflectionMethod($subject, $method);
$params = $r->getParameters();
if (\count($params) !== 1 || !($type = $params[0]->getType()) instanceof \ReflectionNamedType) {
throw new LogicException(sprintf('The method "%s::%s()" must have exactly one parameter with a single type.', \get_debug_type($subject), $method));
}
$enumClass = $type->getName();
if (!enum_exists($enumClass)) {
throw new LogicException(sprintf('The enum class "%s" does not exist.', $enumClass));
}
$marking = key($marking->getPlaces());
try {
$marking = [$enumClass, 'from']($marking);
} catch (\Error) {
throw new InvalidArgumentException(sprintf('The value "%s" is not a valid case of the "%s" enum.', $marking, $enumClass));
}
$subject->{$method}($marking);
}
}
// Then create the marking store for your entity
final class DeploymentStatusMarkingStore extends EnumReflectionMarkingStore
{
public function __construct()
{
parent::__construct('status');
}
} Then you would use like this in your config: // config/packages/workflow.php
return static function (FrameworkConfig $frameworkConfig): void {
$deploymentStatus = $frameworkConfig
->workflows()
->workflows('deployment_status');
$deploymentStatus
->type('state_machine')
->supports([MyEntity::class])
->initialMarking([Status::Pending->value]);
$deploymentStatus->markingStore()
->service(DeploymentStatusMarkingStore::class);
// ...
} I'm not a decision-maker, but I think I would be 👎 on this feature request. It's been debated multiple of times and it was a dead-end every time. But who knows, someone may come with a clever and great solution that may be accepted! |
Hi all, thanks for your replies! I'm sorry for not getting back to you sooner, but I'm currently in a different timezone and my agenda did not allow for a quick response. I appreciate the opinions, insights, and examples on this subject. Even though I have a different opinion I can respect the decision and accept this situation. I will close this ticket for now and hope someone else will find this resourceful when thinking of implementing |
Uh oh!
There was an error while loading. Please reload this page.
Symfony version(s) affected
7.0.x
Description
Configure a backed enum as enumType on your doctrine entity together with the workflow component throws a "unable to convert to string"-error when retrieving the
enabled transitions
.How to reproduce
Create a backed enum:
Create an entity and use the enum above as enumType:
Configure the following workflow:
Retrieve the enabled transitions using the workflow component:
Will throw the following error:
Possible Solution
in vendor/symfony/workflow/MarkingStore/MethodMarkingStore.php:67
Check if type is backed enum, get value and set as string:
Additional Context
No response
The text was updated successfully, but these errors were encountered: