Skip to content

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

Closed
Caroga opened this issue Apr 12, 2024 · 14 comments
Closed

Add BackedEnums support in Symfony Workflow Marking Store #54582

Caroga opened this issue Apr 12, 2024 · 14 comments

Comments

@Caroga
Copy link

Caroga commented Apr 12, 2024

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:

enum CustomerStatus: string
{
    case New = 'new';
    case AccountActive = 'account_active';
    case AccountActivationExpired = 'account_activation_expired';
    case AccountDeletedByCustomer = 'account_deleted_by_customer';
    case AccountDisabledByAdmin = 'account_disabled_by_admin';
    case AccountDeletedByAdmin = 'account_deleted_by_admin';
}

Create an entity and use the enum above as enumType:

 #[ORM\Column(type: 'string', length: 255, enumType: CustomerStatus::class)]
    private CustomerStatus $status;

    public function getStatus(): CustomerStatus
    {
        return $this->status;
    }

    public function setStatus(CustomerStatus $status): static
    {
        $this->status = $status;

        return $this;
    }

Configure the following workflow:

framework:
  workflows:
    customer:
      type: 'state_machine'
      marking_store:
        type: 'method'
        property: 'status'
      initial_marking: 'new'
      supports:
        - 'App\Entity\Customer'
      places:
        new: ~
        account_active:
        account_activation_expired:
        account_deleted_by_customer:
        account_disabled_by_admin:
        account_deleted_by_admin:
      transitions:
        activate:
          from: [ 'new', 'account_disabled_by_admin' ]
          to: 'account_active'
          guard: "is_granted('ROLE_ADMIN')"
        activation_expires:
          from: 'new'
          to: 'account_activation_expired'
          guard: "is_granted('ROLE_ADMIN')"
        delete_by_customer:
          from: [ 'new', 'account_active' ]
          to: 'account_deleted_by_customer'
          guard: "is_granted('ROLE_CUSTOMER')"
        disable_by_admin:
          from: [ 'new', 'account_active' ]
          to: 'account_disabled_by_admin'
          guard: "is_granted('ROLE_ADMIN')"
        delete_by_admin:
          from: [ 'new', 'account_active' ]
          to: 'account_deleted_by_admin'
          guard: "is_granted('ROLE_ADMIN')"

Retrieve the enabled transitions using the workflow component:

 public function __construct(private WorkflowInterface $customerStateMachine)
    {}

public function something() 
{
    $this->customerStateMachine->getEnabledTransitions(new Customer()); // throws error
}

Will throw the following error:

Object of class App\Enum\CustomerStatus could not be converted to string

Possible Solution

in vendor/symfony/workflow/MarkingStore/MethodMarkingStore.php:67

if ($this->singleState) {
    $marking = [(string) $marking => 1]; // throws error due to explicit cast to string
} elseif (!\is_array($marking)) {
    throw new LogicException(sprintf('The marking stored in "%s::$%s" is not an array and the Workflow\'s Marking store is instantiated with $singleState=false.', get_debug_type($subject), $this->property));
}

Check if type is backed enum, get value and set as string:

if ($this->singleState) {
    if ($marking instanceof \BackedEnum){
        $marking = $marking->value;
    }
    $marking = [(string) $marking => 1];
} elseif (!\is_array($marking)) {
    throw new LogicException(sprintf('The marking stored in "%s::$%s" is not an array and the Workflow\'s Marking store is instantiated with $singleState=false.', get_debug_type($subject), $this->property));
}

Additional Context

No response

@derrabus
Copy link
Member

Can you elaborate why this is a bug? Do we document or advertise the usage of enums in that context anywhere?

@Caroga
Copy link
Author

Caroga commented Apr 12, 2024

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 BackedEnum to support a toString implementation also.

Anyway, if this is a bug or a feature request doesn't matter to me. At the moment the workflow component is preventing the usage of BackedEnum types on entity level in Doctrine and other places. Do you agree that supporting the BackedEnum type would be beneficial?
I do believe that more users will encounter this error when implementing BackedEnums in their application, as I think this is a legitimate use-case.

Thank you for your response!

@derrabus
Copy link
Member

I am not aware of documentation or advertisement about the usage of enums in this context,

Not a bug then. 😃

I do consider this to be a fault on the PHP side, as one would expect BackedEnum to support a toString implementation also.

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.

@Caroga
Copy link
Author

Caroga commented Apr 12, 2024

Not a bug then. 😃

Agreed!

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.

I understand this decision in general regarding \Enums, but I would like to ask you to revisit this conclusion in the case of a \BackedEnum.
As the \BackedEnum is declared as a scalar equivalent, therefore it would make an exception to this rule.
In my opinion, this would not fall under silently accepting an \Enum.

@derrabus
Copy link
Member

derrabus commented Apr 13, 2024

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 string|\BackedEnum parameter types and complexity overhead through special handling of enums. This is why we are very careful with opening interfaces for enums.

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.

@Caroga
Copy link
Author

Caroga commented Apr 13, 2024

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 \Enums but of \BackedEnums which is a PHP native scalar equivalent for a string or int.
Both of these types are accepted within the workflow component when requested for in the marking store as they are both implicitly cast to a string when requested.

The \BackedEnum class provides several methods/parameters by design to allow for requesting the name and/or value of the case. Since the class is solid this means a concrete implementation of this interface is expected.
Compared to implementing a regular class with a __toString() method this is the same approach, only now supported natively by PHP.

I want to sum up the reasons why I am convinced that the workflow component should support the \BackedEnum type:

  • \BackedEnum is a PHP native scalar equivalent,
  • It can hold either int or string types and both are supported by the MarkingStore within the Workflow component duo to implicit casting to string.
  • All \BackedEnum cases must have a unique scalar equivalent defined explicitly, which adds perfectly to the Workflow component.
  • This use case is not specific to my situation, more similar setups will emerge when other applications are upgraded to PHP 8.1 and allow for the usage of enumType on Doctrine entities.
  • The usage of enumTypes on Doctrine is advertised in the documentation for both Doctrine as well as EasyAdmin. Therefore not supporting this would break the support for enumTypes on Doctrine entities and force the developer to choose between components.

I am convinced this is a step forward and does not open the door for implementations on \Enums, as \BackedEnums documentation clearly states that a Backed Enum may contain only Backed Cases so there is no risk of supporting mixed types.

I hope that the above reasons/arguments are enough to agree upon a reconsideration of the team's stand for introducing \BackedEnums. I am more than happy to discuss this further (also on Slack).

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 Marking Store not compatible with Backed Enums to Add BackedEnums support in Symfony Workflow Marking Store to better reflect the goal of this feature request. Feel free to change this if necessary.

@Caroga Caroga changed the title Marking Store not compatible with Backed Enums Add BackedEnums support in Symfony Workflow Marking Store Apr 13, 2024
@smnandre
Copy link
Member

which is a PHP native scalar equivalent for a string or int

Not exactly. BackedEnum cases have a scalar equivalent. But a BackedEnum is not equivalent to a scalar.

[...] as they are both implicitly cast to a string when requested.

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"

https://3v4l.org/uXKG0

@Caroga
Copy link
Author

Caroga commented Apr 13, 2024

Not exactly. BackedEnum cases have a scalar equivalent. But a BackedEnum is not equivalent to a scalar.

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)."

You cannot implicitly cast a backedenum to a string, and an Enum class cannot implements Stringable.

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 code will throw a Fatal Error "Object of class Suit could not be converted to string"

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

https://3v4l.org/GYKUl

@derrabus
Copy link
Member

Where this case differs, in my opinion, is that it is not an implementation of \Enums but of \BackedEnums

No. That makes zero difference.

which is a PHP native scalar equivalent for a string or int.

No. Backed enums have a scalar representation, but they're not scalar equivalents. They're objects.

@Caroga
Copy link
Author

Caroga commented Apr 13, 2024

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?

@derrabus
Copy link
Member

derrabus commented Apr 13, 2024

I've found the discussion I was looking for: #44211. Please have a look at that discussion, especially at @lyrixx' assessment and conclusion.


What about the rest of my arguments for implementation?

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. 😓

This use case is not specific to my situation, more similar setups will emerge when other applications are upgraded to PHP 8.1 and allow for the usage of enumType on Doctrine entities.

If I may make that observation: You're pretty late to the party as PHP 8.1 is approaching EOL.

The usage of enumTypes on Doctrine is advertised in the documentation for both Doctrine as well as EasyAdmin. Therefore not supporting this would break the support for enumTypes on Doctrine entities and force the developer to choose between components.

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. 🙂

@Aerendir
Copy link
Contributor

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’.

@alexandre-daubois
Copy link
Member

alexandre-daubois commented Apr 15, 2024

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!

@Caroga
Copy link
Author

Caroga commented Apr 17, 2024

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.
Thanks for all of your time, input, and effort. This is much appreciated!

I will close this ticket for now and hope someone else will find this resourceful when thinking of implementing (Backed)Enums in the workflow component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants