Added support for guards when advancing workflow from a command #23906

GDIBass · 2017-08-16T23:40:18Z

Q	A
Branch?	3.3
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#23904
License	MIT
Doc PR	symfony/symfony-docs

Added support for advancing a workflow from the command line when the transition has guards.

stloyd · 2017-08-17T07:05:54Z

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

@@ -55,6 +56,11 @@ private function getVariables(GuardEvent $event)
    {
        $token = $this->tokenStorage->getToken();

+        if ($token == null) {


null === $token

nicolas-grekas · 2017-08-17T08:50:46Z

ping @Nyholm @lyrixx

lyrixx · 2017-08-17T09:06:16Z

Hello.

@nicolas-grekas you can simply add the workflow label instead of pinging me ;) (Both are fine anyway)

@GDIBass:

I'm 👎 with the current implementation.
It's the developer responsibility to set the right token when using the security from the CLI.

I understand your PR it's a convenient fallback but IMHO it will lead to many WTF for people that are not aware to see "feature"/fallaback.

May be we can instead throw an exception if the token is null. WDYT?

GDIBass · 2017-08-17T15:02:16Z

@lyrixx Hrmm, that makes sense.

In my own implementation, I'm only writing expressions that check the subject. It would be nice if we could only throw an exception only if the expression required the user (like it used an is_granted check), but that would mean modifying the Expression language, right?

lyrixx · 2017-08-17T15:14:45Z

It is possible but it will need more work (I mean much more CPU work). Not sure it worth it.

GDIBass · 2017-08-17T15:17:40Z

How about something like this?

https://pastebin.com/34hgUi7j

lyrixx · 2017-08-17T15:23:08Z

it will lead to PHP warnings in the expression evaluation.

GDIBass · 2017-08-17T15:33:59Z

Ok, gotcha. So I guess just something like this:

https://pastebin.com/cc9iazXs

lyrixx · 2017-08-17T21:23:28Z

~~Do you want to create a pull request ?~~

Edit: oups, it's already a PR. :D sorry

stloyd · 2017-08-18T06:45:03Z

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

@@ -55,6 +56,10 @@ private function getVariables(GuardEvent $event)
    {
        $token = $this->tokenStorage->getToken();

+        if (null === $token) {
+            throw new \Exception("No token is set");


Please don't use top level exceptions.

lyrixx · 2017-08-18T08:57:48Z

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

@@ -55,6 +55,10 @@ private function getVariables(GuardEvent $event)
    {
        $token = $this->tokenStorage->getToken();

+        if (null === $token) {
+            throw new \Exception('No token is set');


I don't know why @stloyd's comment is lost ; but he was right.
You should use an exception that lives in the Workflow component. If the existing exception does not match
this case, you will have to create a new one.

lyrixx · 2017-08-18T08:59:01Z

src/Symfony/Component/Workflow/Tests/EventListener/GuardListenerTest.php

+    public function testWithNoTokenStorage()
+    {
+        $event = $this->createEvent();
+        $this->tokenStorage = null;


this does not work.
You should configure the token storage to return null

Cool, thanks for your patience here. First time contributing!

I'll get this done right away

Simperfit · 2017-08-24T06:57:34Z

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

@@ -55,6 +56,10 @@ private function getVariables(GuardEvent $event)
    {
        $token = $this->tokenStorage->getToken();

+        if (null === $token) {
+            throw new InvalidTokenConfigurationException('No token is set');


Could maybe be something like sprintf('There are no token available for the transition event %s', $eventName) ?

WDYT @lyrixx ?

So:

throw new InvalidTokenConfigurationException(sprintf('There are no token available for the transition event %s', $event->getWorkflowName()));

Simperfit · 2017-08-24T06:59:39Z

Travis failure seems unrelated

xabbuh · 2017-09-15T13:06:21Z

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

@@ -55,6 +56,10 @@ private function getVariables(GuardEvent $event)
    {
        $token = $this->tokenStorage->getToken();

+        if (null === $token) {
+            throw new InvalidTokenConfigurationException(sprintf('There are no token available for workflow %s', $event->getWorkflowName()));


[...] no tokens [...]

and the exception message should end with a dot

xabbuh · 2017-09-15T13:07:43Z

src/Symfony/Component/Workflow/Tests/EventListener/GuardListenerTest.php

+     * @expectedException \Symfony\Component\Workflow\Exception\InvalidTokenConfigurationException
+     * @expectedExceptionMessage There are no token available for workflow unnamed
+     */
+    public function testWithNoTokenStorage()


actually, there is a token storage but no token

lyrixx · 2017-10-05T16:42:26Z

👍

fabpot · 2017-10-05T22:37:22Z

Thank you @GDIBass.

…mmand (GDIBass) This PR was merged into the 3.3 branch. Discussion ---------- Added support for guards when advancing workflow from a command | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23904 | License | MIT | Doc PR | symfony/symfony-docs Added support for advancing a workflow from the command line when the transition has guards. Commits ------- 15fd863 Updated Test name and exception name to be more accurate fefc202 newline at end of file 8f2fa6b changed exception message 49839e3 Ahh, I see. It actually wants a newline! 2c9d1e2 Removed newline 92308b4 Created new Exception to throw and modified tests. 2ebc71a Created new Exception to throw and modified tests 3a8b2ed Code standard fixes 22b44e2 Changed automatic token generation to throw an exception instead 8ab59cb Updated if statement 324c208 Code standards update b044ffb Added support for guards when advancing workflow from a command

GDIBass added 2 commits August 16, 2017 23:21

Added support for guards when advancing workflow from a command

b044ffb

Code standards update

324c208

carsonbot added Status: Needs Review Bug labels Aug 16, 2017

stloyd reviewed Aug 17, 2017

View reviewed changes

Updated if statement

8ab59cb

nicolas-grekas added this to the 3.3 milestone Aug 17, 2017

lyrixx added the Workflow label Aug 17, 2017

GDIBass mentioned this pull request Aug 17, 2017

Cannot advance a workflow with a guard from a command, even if the guard never checks the user #23904

Closed

Changed automatic token generation to throw an exception instead

22b44e2

stloyd reviewed Aug 18, 2017

View reviewed changes

Code standard fixes

3a8b2ed

lyrixx reviewed Aug 18, 2017

View reviewed changes

GDIBass added 4 commits August 18, 2017 08:37

Created new Exception to throw and modified tests

2ebc71a

Created new Exception to throw and modified tests.

92308b4

Removed newline

2c9d1e2

Ahh, I see. It actually wants a newline!

49839e3

Simperfit reviewed Aug 24, 2017

View reviewed changes

GDIBass added 2 commits August 25, 2017 11:25

changed exception message

8f2fa6b

newline at end of file

fefc202

Simperfit approved these changes Aug 26, 2017

View reviewed changes

xabbuh reviewed Sep 15, 2017

View reviewed changes

Updated Test name and exception name to be more accurate

15fd863

fabpot approved these changes Oct 5, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 5, 2017

fabpot merged commit 15fd863 into symfony:3.3 Oct 5, 2017

fabpot mentioned this pull request Oct 5, 2017

Release v3.3.10 #24452

Merged

lyrixx mentioned this pull request Oct 6, 2017

[Workfow] Management of expressions in guard Listerner (and all others components) #24454

Closed

lyrixx mentioned this pull request Feb 7, 2021

[Worflow] Fixed GuardListener when using the new Security system #39671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for guards when advancing workflow from a command #23906

Added support for guards when advancing workflow from a command #23906

GDIBass commented Aug 16, 2017 •

edited by fabpot

Loading

stloyd Aug 17, 2017

GDIBass Aug 17, 2017

nicolas-grekas commented Aug 17, 2017

lyrixx commented Aug 17, 2017

GDIBass commented Aug 17, 2017

lyrixx commented Aug 17, 2017

GDIBass commented Aug 17, 2017

lyrixx commented Aug 17, 2017

GDIBass commented Aug 17, 2017

lyrixx commented Aug 17, 2017 •

edited

Loading

stloyd Aug 18, 2017

lyrixx Aug 18, 2017

lyrixx Aug 18, 2017

GDIBass Aug 18, 2017

Simperfit Aug 24, 2017

GDIBass Aug 25, 2017

Simperfit commented Aug 24, 2017

xabbuh Sep 15, 2017

xabbuh Sep 15, 2017

GDIBass Sep 19, 2017

xabbuh Sep 15, 2017

GDIBass Sep 19, 2017

lyrixx commented Oct 5, 2017

fabpot commented Oct 5, 2017

Added support for guards when advancing workflow from a command #23906

Added support for guards when advancing workflow from a command #23906

Conversation

GDIBass commented Aug 16, 2017 • edited by fabpot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 17, 2017

lyrixx commented Aug 17, 2017

GDIBass commented Aug 17, 2017

lyrixx commented Aug 17, 2017

GDIBass commented Aug 17, 2017

lyrixx commented Aug 17, 2017

GDIBass commented Aug 17, 2017

lyrixx commented Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented Oct 5, 2017

fabpot commented Oct 5, 2017

GDIBass commented Aug 16, 2017 •

edited by fabpot

Loading

lyrixx commented Aug 17, 2017 •

edited

Loading