[Workflow] Override guard listener class #29751

ochretien · 2019-01-02T21:17:33Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT

Visibility of properties ans dome methods of the GuardListener make difficult to customize workflow guard configuration.
I would like to add the inheritance of the GuardListener along with the possibility to override the default class.
Configuration allow to globally override default Symfony GuardListener with a workflow.guard_listener parameter or for a single workflow through a guard_listener configuration option.

In the next configuration exemple, both workflow.issue.listener.guard and workflow.pull_request.listener.guard are instances of AppBundle\EventListener\GuardListener:

parameters:
    workflow.guard_listener: AppBundle\EventListener\GuardListener

framework:
    workflows:
        issue:
            marking_store:
                type: single_state
                arguments:
                    - state
            supports: AppBundle\Entity\Issue
            initial_place: created
            places:
                - created
                - affected
                - closed
            transitions:
                affect:
                    guard: "is_valid(subject, ['affectable'])"
                    from: created
                    to:   affected
                close:
                    from: completed
                    to: closed

        pull_request:
            marking_store:
                type: multiple_state
                arguments:
                    - states
            supports: AppBundle\Entity\PullRequest
            initial_place: opened
            places:
                - opened
                - travis_build
                - travis_build_ok
                - travis_build_fail
                - dev_review
                - dev_review_ok
                - dev_review_fail
                - merged
            transitions:
                review:
                    from: opened
                    to:   [travis_build, dev_review]
                travis_build_success:
                    from: travis_build
                    to:   travis_build_ok
                travis_build_failure:
                    from: travis_build
                    to:   travis_build_fail
                dev_review_success:
                    from: dev_review
                    to:   dev_review_ok
                dev_review_failure:
                    from: dev_review
                    to:   dev_review_fail
                merge:
                    from: [travis_build_ok, dev_review_ok]
                    to:   merged

In the next configuration exemple, only workflow.issue.listener.guard is an instance of AppBundle\EventListener\GuardListener:

framework:
    workflows:
        issue:
            marking_store:
                type: single_state
                arguments:
                    - state
            supports: AppBundle\Entity\Issue
            guard_listener: AppBundle\EventListener\GuardListener
            initial_place: created
            places:
                - created
                - affected
                - closed
            transitions:
                affect:
                    guard: "is_valid(subject, ['affectable'])"
                    from: created
                    to:   affected
                close:
                    from: completed
                    to: closed

        pull_request:
            marking_store:
                type: multiple_state
                arguments:
                    - states
            supports: AppBundle\Entity\PullRequest
            initial_place: opened
            places:
                - opened
                - travis_build
                - travis_build_ok
                - travis_build_fail
                - dev_review
                - dev_review_ok
                - dev_review_fail
                - merged
            transitions:
                review:
                    guard: "is_fully_authenticated()"
                    from: opened
                    to:   [travis_build, dev_review]
                travis_build_success:
                    from: travis_build
                    to:   travis_build_ok
                travis_build_failure:
                    from: travis_build
                    to:   travis_build_fail
                dev_review_success:
                    from: dev_review
                    to:   dev_review_ok
                dev_review_failure:
                    from: dev_review
                    to:   dev_review_fail
                merge:
                    from: [travis_build_ok, dev_review_ok]
                    to:   merged

nicolas-grekas

Thanks for the PR. Here are some comments. Please note that this would be considered as a new feature, thus it should target master.

src/Symfony/Bundle/FrameworkBundle/Resources/config/workflow.xml

nicolas-grekas · 2019-01-03T13:54:36Z

src/Symfony/Component/Workflow/EventListener/GuardListener.php

-    private $trustResolver;
-    private $roleHierarchy;
-    private $validator;
+    protected $configuration;


That's a no-go: we strongly favor private properties because it helps maintenance a lot.

Thanks for the feedback. So as I understand it, it is preferable to create my own new class completely override the listener.guard services declaration.

yes - you can decorate the core listener if it serves your purpose, but only via composition, ie using its public API.

Since there is a no-go on changing the GuardListener properties visibility and there is other ways to serve my purpose, it seems this pull request can be closed

lyrixx · 2019-01-04T10:14:28Z

Hello,
I'm not sure this is the right approach. guard key is an easy way to Guard some transitions.
In your case, I would leverage the metadata key and I would set-up a custom implementation of a Guard Listener.

More over, as @nicolas-grekas told you, we will not change the properties visibility.

Finally, the GuardListener is quite small, you can easily copy it to your projet. In Symfony we have some legacy code, see #24454 ; Your guard will be simpler.

So for now I'm 👎 with this PR

nicolas-grekas · 2019-01-25T11:03:16Z

Closing as explained, thanks for proposing!

ochretien requested a review from lyrixx as a code owner January 2, 2019 21:17

carsonbot added Status: Needs Review Feature labels Jan 2, 2019

ochretien added 2 commits January 3, 2019 11:55

[Workflow] Added configuration option along with parameter

b72fc53

[Workflow] Added inheritance to 'GuardListener'

b97ef81

ochretien force-pushed the override_guard_listener_class branch from f914940 to b97ef81 Compare January 3, 2019 10:55

nicolas-grekas reviewed Jan 3, 2019

View reviewed changes

nicolas-grekas added this to the next milestone Jan 3, 2019

lyrixx changed the title ~~Override guard listener class~~ [Workflow] Override guard listener class Jan 4, 2019

nicolas-grekas closed this Jan 25, 2019

nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Workflow] Override guard listener class #29751

[Workflow] Override guard listener class #29751

ochretien commented Jan 2, 2019

nicolas-grekas left a comment

nicolas-grekas Jan 3, 2019

ochretien Jan 3, 2019

nicolas-grekas Jan 3, 2019

ochretien Jan 3, 2019

lyrixx commented Jan 4, 2019 •

edited

Loading

nicolas-grekas commented Jan 25, 2019

[Workflow] Override guard listener class #29751

[Workflow] Override guard listener class #29751

Conversation

ochretien commented Jan 2, 2019

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas Jan 3, 2019

Choose a reason for hiding this comment

ochretien Jan 3, 2019

Choose a reason for hiding this comment

nicolas-grekas Jan 3, 2019

Choose a reason for hiding this comment

ochretien Jan 3, 2019

Choose a reason for hiding this comment

lyrixx commented Jan 4, 2019 • edited Loading

nicolas-grekas commented Jan 25, 2019

lyrixx commented Jan 4, 2019 •

edited

Loading