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

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

Merged
merged 12 commits into from Oct 5, 2017

Conversation

Projects
None yet
8 participants
@GDIBass
Contributor

GDIBass commented Aug 16, 2017

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.

GDIBass

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas
Member

nicolas-grekas commented Aug 17, 2017

@lyrixx lyrixx added the Workflow label Aug 17, 2017

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Aug 17, 2017

Member

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?

Member

lyrixx commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@GDIBass

GDIBass Aug 17, 2017

Contributor

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

Contributor

GDIBass commented Aug 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Aug 17, 2017

Member

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

Member

lyrixx commented Aug 17, 2017

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

@GDIBass

This comment has been minimized.

Show comment
Hide comment
@GDIBass

GDIBass Aug 17, 2017

Contributor

How about something like this?

https://pastebin.com/34hgUi7j

Contributor

GDIBass commented Aug 17, 2017

How about something like this?

https://pastebin.com/34hgUi7j

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Aug 17, 2017

Member

it will lead to PHP warnings in the expression evaluation.

Member

lyrixx commented Aug 17, 2017

it will lead to PHP warnings in the expression evaluation.

@GDIBass

This comment has been minimized.

Show comment
Hide comment
@GDIBass

GDIBass Aug 17, 2017

Contributor

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

https://pastebin.com/cc9iazXs

Contributor

GDIBass commented Aug 17, 2017

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

https://pastebin.com/cc9iazXs

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Aug 17, 2017

Member

Do you want to create a pull request ?

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

Member

lyrixx commented Aug 17, 2017

Do you want to create a pull request ?

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

GDIBass

GDIBass added some commits Aug 18, 2017

@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Aug 24, 2017

Contributor

Travis failure seems unrelated

Contributor

Simperfit commented Aug 24, 2017

Travis failure seems unrelated

GDIBass added some commits Aug 25, 2017

GDIBass
GDIBass
@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Oct 5, 2017

Member

👍

Member

lyrixx commented Oct 5, 2017

👍

@fabpot

fabpot approved these changes Oct 5, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 5, 2017

Member

Thank you @GDIBass.

Member

fabpot commented Oct 5, 2017

Thank you @GDIBass.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Oct 5, 2017

bug #23906 Added support for guards when advancing workflow from a co…
…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

@fabpot fabpot referenced this pull request Oct 5, 2017

Merged

Release v3.3.10 #24452

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