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

[Workflow] Override guard listener class #29751

Open
wants to merge 2 commits into
base: 3.4
from

Conversation

Projects
None yet
4 participants
@ochretien
Copy link

ochretien commented Jan 2, 2019

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

@ochretien ochretien force-pushed the ochretien:override_guard_listener_class branch from f914940 to b97ef81 Jan 3, 2019

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

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

Show resolved Hide resolved src/Symfony/Bundle/FrameworkBundle/Resources/config/workflow.xml
private $trustResolver;
private $roleHierarchy;
private $validator;
protected $configuration;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 3, 2019

Member

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

This comment has been minimized.

@ochretien

ochretien Jan 3, 2019

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.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 3, 2019

Member

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

This comment has been minimized.

@ochretien

ochretien Jan 3, 2019

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

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

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jan 4, 2019

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

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

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