[SecurityBundle] Rename FirewallContext#getContext() #20417
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9ab32f1 to
033da86
Compare
|
Shouldn't the UPGRADE-4.0.md file be updated as well ? |
|
@FabienPapet You're right, thought I'm not sure upgrade files are not automatically generated from merged commits, changelog ones seem to be so let's get the confirmation of a core team member |
033da86 to
9482521
Compare
|
AFAIK, you need to update it |
9482521 to
ca99176
Compare
|
UPGRADE updated. Thanks |
ca99176 to
6b889f0
Compare
xabbuh
reviewed
Nov 7, 2016
| public function getContext() | ||
| { | ||
| @trigger_error(sprintf('Method "%s()" is deprecated since version 3.2 and will be removed in 4.0. Use "%s::getListeners()" instead.', __METHOD__, __CLASS__), E_USER_DEPRECATED); |
|
👍 |
6c00f60 to
b6a29b2
Compare
b6a29b2 to
03b330c
Compare
|
Updated to target 3.3 |
d309b18 to
e91f331
Compare
e91f331 to
f09a056
Compare
f09a056 to
fe775e5
Compare
fe775e5 to
ee66b49
Compare
|
This is ready to go |
xabbuh
reviewed
Dec 9, 2016
| SecurityBundle | ||
| -------------- | ||
|
|
||
| * The `FirewallContext::getContext()` method has been removed, use the `getListeners()` method instead. |
There was a problem hiding this comment.
looks like this line could be wrapped to be in line with the other lines in the file
xabbuh
approved these changes
Dec 9, 2016
|
Thank you @chalasr. |
fabpot
added a commit
that referenced
this pull request
Dec 9, 2016
…chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [SecurityBundle] Rename FirewallContext#getContext() | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a As pointed out in #19398 (comment), the name of this method is misleading. Because a public service using this class is created for each defined firewall, I suggest to change it to `FirewallContext#getListeners()`, deprecating the current `getContext()` for removing it in 4.0. Commits ------- ee66b49 [SecurityBundle] Rename FirewallContext#getContext()
stof
reviewed
Dec 9, 2016
| return $this->getListeners(); | ||
| } | ||
|
|
||
| public function getListeners() |
There was a problem hiding this comment.
this is a bit confusing considering that this returns both an array of listeners and an exception listener. getListeners may be expected to return just $this->listeners
stof
reviewed
Dec 9, 2016
| public function testGetContextTriggersDeprecation() | ||
| { | ||
| (new FirewallContext(array(), $this->getExceptionListenerMock(), new FirewallConfig('main', 'request_matcher', 'user_checker'))) | ||
| ->getContext(); |
There was a problem hiding this comment.
you should also assert the return value, to ensure that the BC layer works fine (returning null would make your test pass, but the class would be broken)
fabpot
added a commit
that referenced
this pull request
Apr 26, 2017
… (ro0NL) This PR was merged into the 3.3-dev branch. Discussion ---------- [SecurityBundle] Enhance FirewallContext::getListeners() | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20417 (comment), #20417 (comment) | License | MIT | Doc PR | symfony/symfony-docs#... <!--highly recommended for new features--> I think @stof is right.. and the fact we can do this on master currently without the hassle. cc @chalasr Commits ------- ba65078 [SecurityBundle] Enhance FirewallContext::getListeners()
symfony-splitter
pushed a commit
to symfony/security-bundle
that referenced
this pull request
Apr 26, 2017
… (ro0NL) This PR was merged into the 3.3-dev branch. Discussion ---------- [SecurityBundle] Enhance FirewallContext::getListeners() | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#20417 (comment), symfony/symfony#20417 (comment) | License | MIT | Doc PR | symfony/symfony-docs#... <!--highly recommended for new features--> I think @stof is right.. and the fact we can do this on master currently without the hassle. cc @chalasr Commits ------- ba650783f5 [SecurityBundle] Enhance FirewallContext::getListeners()
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As pointed out in #19398 (comment), the name of this method is misleading.
Because a public service using this class is created for each defined firewall, I suggest to change it to
FirewallContext#getListeners(), deprecating the currentgetContext()for removing it in 4.0.