8000 [5.0][Security] Minor clarification of the new isGranted signature by wouterj · Pull Request #34074 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 11, 2019
Merged

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

merged 1 commit into from
Nov 11, 2019

Conversation

wouterj
Copy link
Member
@wouterj 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.

@wouterj wouterj mentioned this pull request Oct 22, 2019
Merged
@nicolas-grekas nicolas-grekas added this to the 5.0 milestone Oct 23, 2019
@nicolas-grekas
Copy link
Member

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
Copy link
Contributor

@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 security/single-attribute-isgranted-5.0 branch from 3e7a36f to 991c0a4 Compare November 8, 2019 23:22
@wouterj
Clarified single attribute to isGranted() a bit more
e41e6b4 
Also, allow the array type for a single attribute.
@wouterj wouterj force-pushed the security/single-attribute-isgranted-5.0 branch from 991c0a4 to e41e6b4 Compare November 8, 2019 23:23
@wouterj
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
Copy link
Member Author
wouterj commented Nov 9, 2019

Build failure seems unrelated btw

nicolas-grekas
nicolas-grekas approved these changes Nov 11, 2019
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member
fabpot commented Nov 11, 2019

Thank you @wouterj.

fabpot added a commit that referenced this pull request Nov 11, 2019
@fabpot
minor #34074 [5.0][Security] Minor clarification of the new isGranted…
81a177d 
… 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
@wouterj wouterj deleted the security/single-attribute-isgranted-5.0 branch November 11, 2019 13:39
@fancyweb fancyweb mentioned this pull request Aug 17, 2020
Closed
Nijusan pushed a commit to Nijusan/symfony that referenced this pull request Aug 18, 2020
fix: clarify parameter name to comply with deprecations from symfony#…
7045fe9 
…34074
fabpot pushed a commit to Nijusan/symfony that referenced this pull request Aug 18, 2020
@fabpot
fix: clarify parameter name to comply with deprecations from symfony#…
91249ed 
…34074
fabpot added a commit that referenced this pull request Aug 18, 2020
@fabpot
minor #37877 fix: clarify parameter name to comply with deprecations …
4bdb222 
…from #34074 (Dennis Langen)

This PR was submitted for the master branch but it was squashed and merged into the 5.1 branch instead.

Discussion
----------

fix: clarify parameter name to comply with deprecations from #34074

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #37824
| License       | MIT
| Doc PR        |

[FrameworkBundle] Clarify parameter name from `$attributes` to `$attribute` in `AbstractController:denyUnlessGranted`

Commits
-------

91249ed fix: clarify parameter name to comply with deprecations from #34074
fabpot added a commit that referenced this pull request Aug 18, 2020
@fabpot
Merge branch '5.1'
fbcdbf9 
* 5.1:
  fix: clarify parameter name to comply with deprecations from #34074
  [Sendgrid-Mailer] Fixed envelope recipients on sendgridApiTransport
  mark the AssertingContextualValidator class as internal
  Fix the parameter names in the SecurityFactoryInterface::create() method
  [Serializer][ClassDiscriminatorMapping] Fix getMappedObjectType() when a discriminator child extends another one
  make return type correct
hultberg pushed a commit to hultberg/symfony that referenced this pull request Sep 17, 2021
@fabpot
Merge branch '5.1'
c0807b8 
* 5.1:
  fix: clarify parameter name to comply with deprecations from symfony#34074
  [Sendgrid-Mailer] Fixed envelope recipients on sendgridApiTransport
  mark the AssertingContextualValidator class as internal
  Fix the parameter names in the SecurityFactoryInterface::create() method
  [Serializer][ClassDiscriminatorMapping] Fix getMappedObjectType() when a discriminator child extends another one
  make return type correct
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz left review comments

@jvasseur jvasseur jvasseur left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@vudaltsov vudaltsov vudaltsov approved these changes

Assignees
No one assigned
Labels
Security Status: Reviewed
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
7 participants
@wouterj @nicolas-grekas @jvasseur @fabpot @javiereguiluz @vudaltsov @carsonbot
0