Skip to content

[Workflow] Add AsMarkingStore attribute #52566

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
Closed
wants to merge 1 commit into from
Closed

[Workflow] Add AsMarkingStore attribute #52566

wants to merge 1 commit into from

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Nov 13, 2023 •
edited
Loading

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

Originally, this idea came up after I implemented a new marking store as stated here.

In a project we're working on, we're using the Workflow component. We created a marking store to support enums for our markings. We'd like to make it a bit generic (understand: support enum and just give to the marking store the property that hold the marking on the subject). Here is the simplified marking store:

final readonly class DeploymentStatusMarkingStore implements MarkingStoreInterface
{
    public function __construct(private string $property)
    {
    }

    public function getMarking(object $subject): Marking
    {
        return new Marking(/* ... */);
    }

    public function setMarking(object $subject, Marking $marking, array $context = []): void
    {
        // ...

        try {
            $marking = [$enumClass, 'from']($marking);
        } catch (\Error) {
            throw new InvalidArgumentException(sprintf('The constant "%s" does not exist.', $marking));
        }

        $subject->{$method}($marking);
    }
}

The $property attribute holds the fieldname of the marking in the subject. However, in the current state, we must define manually the service to provide $property to the constructor:

return static function (FrameworkConfig $frameworkConfig, ContainerConfigurator $configurator): void {
    $configurator->services()
        ->set('workflow.marking_store.status_enum', DeploymentStatusMarkingStore::class)
        ->args(['status']); // This is the boilerplate that I'd like to remove

    // ...

    $deploymentStatus->markingStore()
        ->service('workflow.marking_store.status_enum');

    // ...
};

Yes! We can now use the marking store.

But I'd like to remove this boilerplate by introducing AsMarkingStore. Thanks to this attribute, we can now define our marking store like this:

#[AsMarkingStore('status')]
final readonly class DeploymentStatusMarkingStore extends AbstractMarkingStore
{
    public function getMarking(object $subject): Marking
    {
        // ...
    }

    public function setMarking(object $subject, Marking $marking, array $context = []): void
    {
        // ...
    }
}

The configuration is also simplified:

return static function (FrameworkConfig $frameworkConfig, ContainerConfigurator $configurator): void {
    // ...

    // No more marking store definition to do by hand!
    $deploymentStatus->markingStore()
        ->service(DeploymentStatusMarkingStore::class);

    // ...
};

This attribute also allows to configure the name of the property that hold the marking field name in the store:

#[AsMarkingStore(markingName: 'status', property: 'markingField')]
final readonly class SingleStateEnumMarkingStore implements MarkingStoreInterface
{
    public function __construct(private string $markingField)
    {
    }

    public function getMarking(object $subject): Marking
    {
        // ...
    }

    public function setMarking(object $subject, Marking $marking, array $context = []): void
    {
        // ...
    }
}

Thanks to this, we could even create in our app an AbstractEnumMarkingStore, making some store inherit this class and just define the AsMarkingStore attribute with the good field name.

@carsonbot carsonbot added this to the 7.0 milestone Nov 13, 2023
@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.0" but it seems your PR description refers to branch "7.1".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@alexandre-daubois alexandre-daubois force-pushed the as-marking-store-attr branch 3 times, most recently from cde5e56 to c3412c7 Compare November 13, 2023 14:15
@lyrixx
Copy link
Member

lyrixx commented Nov 13, 2023

I'm not really convinced by this PR. but I may have missed something.

You have this, and you want to remove it:

    $configurator->services()
        ->set('workflow.marking_store.status_enum', DeploymentStatusMarkingStore::class)
        ->args(['status']); // This is the boilerplate that I'd like to remove

OK, why not!

Instead you want to do this :

#[AsMarkingStore(markingName: 'status', property: 'markingField')]
final readonly class SingleStateEnumMarkingStore implements MarkingStoreInterface
{
    public function __construct(private string $markingField)
    {
    }

I got it right?

Instead, why don't you do:

-#[AsMarkingStore(markingName: 'status', property: 'markingField')]
 final readonly class SingleStateEnumMarkingStore implements MarkingStoreInterface
 {
-    public function __construct(private string $markingField)
+    public function __construct(private string $markingField = 'status')
     {
     }

@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Nov 13, 2023 •
edited
Loading

This would allow to do something like this:

abstract class AbstractEnumMarkingStore extends AbstractMarkingStore
{
    public function getMarking(object $subject): Marking
    {
        // ...
    }

    public function setMarking(object $subject, Marking $marking, array $context = []): void
    {
        // ...
    }
}

#[AsMarkingStore('deploymentStatus')]
class DeploymentStatusEnumStore extends AbstractEnumMarkingStore
{
}

#[AsMarkingStore('postStatus')]
class BlogPostEnumStore extends AbstractEnumMarkingStore
{
}

Instead of declaring the constructor everytime to pass the field name like this:

class DeploymentStatusEnumStore extends AbstractEnumMarkingStore
{
    public function __construct()
    {
        parent::__construct('deploymentStatus');
    }
}

It makes it even more concise (and also I like this attribute-style declaration, but that's a more subjective point of view 😄 )

Also, the PR adds a workflow.marking_store tag to the class (which could be generalized to MarkingStoreInterface by the way). This would allow to inject for instance a tagged locator with all those stores and create a more "global" marking store choosing the right store depending on some logic.

@lyrixx
Copy link
Member

lyrixx commented Nov 13, 2023

I don't like, but I'm not against it.

I found it weird that it configure a constructor this way. And it does not scale that good with many different service, you have to create a new class for each. IMHO, this is better to create only one class with N services definition. Or create N classes with a constructor override, like you explain in your last example.

Anyway, This is not related to the workflow, it could be generalized to any kind of service, couldn't it?

@joelwurtz
Copy link
Contributor

joelwurtz commented Nov 13, 2023 •
edited
Loading

I'm also not a fan of this, adding an attribute to avoid calling a parameter in the parent constructor seems superfluous comparing to the maintenance cost.

We can compare this when declaring a specific repository for Doctrine (calling the parent constructor with the class name could be replaced by an attribute) and i was never bother by this way of doing it.

@alexandre-daubois
Copy link
Member Author

With hindsight, I think you're both right. I may try a different approach, but not this one. Thank you both for your feedback 👍

@alexandre-daubois alexandre-daubois deleted the as-marking-store-attr branch November 14, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyrixx lyrixx Awaiting requested review from lyrixx lyrixx is a code owner

Assignees
No one assigned
Labels
Feature Status: Needs Review Workflow
Projects
None yet
Milestone
7.0
Development

Successfully merging this pull request may close these issues.
4 participants
@alexandre-daubois @carsonbot @lyrixx @joelwurtz