Skip to content

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

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: 7.3
Choose a base branch
from
Open

[FrameworkBundle] Add support for configuring workflow places with a BackedEnum #60204

wants to merge 1 commit into from
+92 −0

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Apr 12, 2025

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

Examples:

framework:
    workflows:
        my_workflow_name:
            places: !php/enum Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Fixtures\Workflow\Places

and

<?php

namespace Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Fixtures\Workflow;

enum Places: string
{
    case A = 'a';
    case B = 'b';
    case C = 'c';
}

@carsonbot carsonbot added this to the 7.3 milestone Apr 12, 2025
@lyrixx lyrixx mentioned this pull request Apr 12, 2025
Open
nicolas-grekas
nicolas-grekas reviewed Apr 12, 2025
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

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.

lyrixx
lyrixx commented Apr 12, 2025
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/Workflow/Places.php

enum Places: string
{
case A = 'a';
Copy link
Member Author

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]?

@lyrixx
Copy link
Member Author

lyrixx commented Apr 12, 2025

like one more step: when this is used, the marking store should pass the enum instance to the object instead of the string.

Of course! I commented the @tucksaun PR!

@alexandre-daubois
Copy link
Member

alexandre-daubois commented Apr 12, 2025 •
edited
Loading

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?

@lyrixx
Copy link
Member Author

lyrixx commented Apr 13, 2025

I think it would be hard. And with unit enum, i fail to see what you save in your db

@lyrixx
Copy link
Member Author

lyrixx commented Apr 14, 2025

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

@tucksaun
Copy link
Contributor

@lyrixx nice work! This is similar to something I quickly tried and it seemed to work well.

We'll need to store the fact it's an enum in the configuration, to be able to configure the marking store.

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?

@nicolas-grekas
Copy link
Member

I think so yes: when places is configured to an enum class, we should only support storing enums.

smnandre
smnandre reviewed Apr 17, 2025
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/workflow_enum.php
Comment on lines +15 to +16
'from' => Places::A->value,
'to' => Places::B->value,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'from' => Places::A->value,
'to' => Places::B->value,
'from' => Places::A,
'to' => Places::B,

In a following PR ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be 🤞🏼

@nicolas-grekas
Copy link
Member

We'll need to store the fact it's an enum in the configuration

@lyrixx don't miss this part ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@smnandre smnandre smnandre left review comments

Assignees
No one assigned
Labels
Feature FrameworkBundle Status: Needs Review
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
6 participants
@lyrixx @alexandre-daubois @tucksaun @nicolas-grekas @smnandre @carsonbot