Skip to content

[Security] deprecate multiple providers in context listener #21792

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

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Feb 27, 2017

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Passing multiple user providers to the context listener does not make
much sense. The listener is only responsible to refresh users for a
particular firewall. Thus, it must only be aware of the user provider
for this particular firewall.

Passing multiple user providers to the context listener does not make
much sense. The listener is only responsible to refresh users for a
particular firewall. Thus, it must only be aware of the user provider
for this particular firewall.
@xabbuh xabbuh force-pushed the deprecate-multiple-context-listener-providers-support branch from 66f295b to 53df0de Compare February 27, 2017 22:13
@xabbuh
Copy link
Member Author

xabbuh commented Feb 27, 2017

This will need a rebase with #21791 being merged up to master.

{
if (empty($contextKey)) {
throw new \InvalidArgumentException('$contextKey must not be empty.');
}

if (is_array($userProviders)) {
@trigger_error(sprintf('Being able to pass multiple user providers to the constructor of %s is deprecated since version 3.3 and will not be supported anymore in 4.0. Only pass the user provider for the current firewall context instead.', __CLASS__), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

missing " around %s

@@ -94,6 +94,9 @@ Process
Security
--------

* Deprecated the ability to pass multiple user providers to the `ContextListener`. Pass only the user provider responsible
for the active firewall instead.
Copy link
Member

Choose a reason for hiding this comment

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

looks like it's not wrapped at 80 chars

@fabpot
Copy link
Member

fabpot commented Feb 28, 2017

Thank you @xabbuh.

@fabpot fabpot merged commit 53df0de into symfony:master Feb 28, 2017
fabpot added a commit that referenced this pull request Feb 28, 2017
…tener (xabbuh)

This PR was squashed before being merged into the 3.3-dev branch (closes #21792).

Discussion
----------

[Security] deprecate multiple providers in context listener

 Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Passing multiple user providers to the context listener does not make
much sense. The listener is only responsible to refresh users for a
particular firewall. Thus, it must only be aware of the user provider
for this particular firewall.

Commits
-------

53df0de [Security] deprecate multiple providers in context listener
fbd9f88 [SecurityBundle] only pass relevant user provider
@xabbuh xabbuh deleted the deprecate-multiple-context-listener-providers-support branch February 28, 2017 06:34
@stof
Copy link
Member

stof commented Feb 28, 2017

@xabbuh a given firewall can rely on multiple user providers when multiple authentication listeners are enabled (as each of them can use a different user provider)

@xabbuh
Copy link
Member Author

xabbuh commented Feb 28, 2017

will be reverted by #21799

xabbuh added a commit to xabbuh/symfony that referenced this pull request Feb 28, 2017
… in context listener (xabbuh)"

This reverts commit 924c1f0, reversing
changes made to afff0ce.
nicolas-grekas added a commit that referenced this pull request Feb 28, 2017
…r provider (xabbuh)" (xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

Revert "bug #21791 [SecurityBundle] only pass relevant user provider (xabbuh)"

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21791
| License       | MIT
| Doc PR        |

#21791 was a mistake as pointed out by @slaci (see #21791 (comment)) and @stof (see #21792 (comment)).

Commits
-------

f6637dd Revert "bug #21791 [SecurityBundle] only pass relevant user provider (xabbuh)"
nicolas-grekas added a commit that referenced this pull request Feb 28, 2017
…ders in context listener (xabbuh)" (xabbuh)

This PR was merged into the 3.3-dev branch.

Discussion
----------

Revert "feature #21792 [Security] deprecate multiple providers in context listener (xabbuh)"

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21791
| License       | MIT
| Doc PR        |

#21792 was a mistake as pointed out by @slaci (see #21791 (comment)) and @stof (see #21792 (comment)).

Commits
-------

3cfa0c7 Revert "feature #21792 [Security] deprecate multiple providers in context listener (xabbuh)"
@fabpot fabpot mentioned this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants