Skip to content
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

[5.0][Security] Minor clarification of the new isGranted signature #34074

Merged

Conversation

@wouterj
Copy link
Member

wouterj commented Oct 22, 2019

Q A
Branch? 5.0
Bug fix? no
New feature? no
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

As we now only allow a single attribute for isGranted() in Symfony 5, let's adapt the PHPdoc and parameter name as well.

@nicolas-grekas nicolas-grekas added this to the 5.0 milestone Oct 23, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Oct 23, 2019

That's for 4.4.
Also AccessDecisionManagerInterface has the same issue, isn't it?
Or should we remove the deprecation in that says:
Passing more than one Security attribute to AccessDecisionManager::decide() is deprecated since Symfony 4.4.?

@jvasseur

This comment has been minimized.

Copy link
Contributor

jvasseur commented Oct 23, 2019

@nicolas-grekas the problem with the AccessDecisionManagerInterface is that we can't remove the current array type-hint without creating a BC break so the current state of things is that we still require an array but that array must have only one argument.

That's kind of broken but I'm not sure how we could create a migrations path to completely remove the usage of arrays in the authorization process (the voter interface has the same problem).

@wouterj wouterj force-pushed the wouterj:security/single-attribute-isgranted-5.0 branch from 3e7a36f to 991c0a4 Nov 8, 2019
Also, allow the array type for a single attribute.
@wouterj wouterj force-pushed the wouterj:security/single-attribute-isgranted-5.0 branch from 991c0a4 to e41e6b4 Nov 8, 2019
@wouterj

This comment has been minimized.

Copy link
Member Author

wouterj commented Nov 8, 2019

PR updated:

  • PHPdoc says mixed again (but mentions string + Expression in the description as being supported by Core)
  • The exception on array is removed. If the single attribute is an array, it should be allowed. This makes it a bit harder if one skips the 4.4 release and upgrades van 4.3 to 5.0 immediately, but I don't recommend anyone doing that anyway.

That's for 4.4.
Also AccessDecisionManagerInterface has the same issue, isn't it?
Or should we remove the deprecation in that says:
Passing more than one Security attribute to AccessDecisionManager::decide() is deprecated since Symfony 4.4.?

@nicolas-grekas I'm not so sure what you meant with this comment. I would say in 4.4, we still allow multiple attributes (but deprecated it), so we shouldn't have these changes there. As for the deprecation, I would prefer to keep it. Otherwise, it's impossible to provide a smooth upgrade path here.

@wouterj

This comment has been minimized.

Copy link
Member Author

wouterj commented Nov 9, 2019

Build failure seems unrelated btw

Copy link
Member

nicolas-grekas left a comment

I'm a bit sad that decide() still accepts a list of attibutes, thus the array wrapper
but we might continue working on this in 5.x I suppose.

@fabpot
fabpot approved these changes Nov 11, 2019
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Nov 11, 2019

Thank you @wouterj.

fabpot added a commit that referenced this pull request Nov 11, 2019
… signature (wouterj)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[5.0][Security] Minor clarification of the new isGranted signature

| Q             | A
| ------------- | ---
| Branch?       | 5.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | n/a
| License       | MIT
| Doc PR        | n/a

As we now only allow a single attribute for `isGranted()` in Symfony 5, let's adapt the PHPdoc and parameter name as well.

Commits
-------

e41e6b4 Clarified single attribute to isGranted() a bit more
@fabpot fabpot merged commit e41e6b4 into symfony:master Nov 11, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@wouterj wouterj deleted the wouterj:security/single-attribute-isgranted-5.0 branch Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.