Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[SecurityBundle] Rename FirewallContext#getContext()#20417
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[SecurityBundle] Rename FirewallContext#getContext()#20417
Uh oh!
There was an error while loading.Please reload this page.
Conversation
9ab32f1 to033da86CompareFabienPapet commentedNov 5, 2016
Shouldn't the UPGRADE-4.0.md file be updated as well ? |
chalasr commentedNov 5, 2016
@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 to9482521CompareTaluu commentedNov 5, 2016
AFAIK, you need to update it |
9482521 toca99176Comparechalasr commentedNov 5, 2016
UPGRADE updated. Thanks |
ca99176 to6b889f0Compare| */ | ||
| publicfunctiongetContext() | ||
| { | ||
| @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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The %() method is [...]
xabbuh commentedNov 7, 2016
👍 |
6c00f60 tob6a29b2Compareb6a29b2 to03b330cComparechalasr commentedNov 20, 2016
Updated to target 3.3 |
d309b18 toe91f331Comparee91f331 tof09a056Comparef09a056 tofe775e5Comparefe775e5 toee66b49Comparechalasr commentedDec 8, 2016
This is ready to go |
| SecurityBundle | ||
| -------------- | ||
| * The`FirewallContext::getContext()` method has been removed, use the`getListeners()` method instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
looks like this line could be wrapped to be in line with the other lines in the file
fabpot commentedDec 9, 2016
Thank you@chalasr. |
…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/aAs 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()
| return$this->getListeners(); | ||
| } | ||
| publicfunctiongetListeners() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
| publicfunctiontestGetContextTriggersDeprecation() | ||
| { | ||
| (newFirewallContext(array(),$this->getExceptionListenerMock(),newFirewallConfig('main','request_matcher','user_checker'))) | ||
| ->getContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
… (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@chalasrCommits-------ba65078 [SecurityBundle] Enhance FirewallContext::getListeners()
… (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@chalasrCommits-------ba650783f5 [SecurityBundle] Enhance FirewallContext::getListeners()
Uh oh!
There was an error while loading.Please reload this page.
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.