NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[SecurityBundle] Enhance FirewallContext::getListeners()#22475

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromro0NL:firewallcontext

Apr 26, 2017

Merged

[SecurityBundle] Enhance FirewallContext::getListeners()#22475

fabpot merged 1 commit intosymfony:masterfromro0NL:firewallcontext

Apr 26, 2017

Conversation

Copy link

Contributor

ro0NL commentedApr 19, 2017•
edited
Loading

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#...

I think@stof is right.. and the fact we can do this on master currently without the hassle.

cc@chalasr

carsonbot added Status: Needs Review SecurityBundle Feature labels

Apr 19, 2017

chalasr approved these changes

Apr 19, 2017

View reviewed changes

Copy link

Member

chalasr left a comment

Choose a reason for hiding this comment

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

👍

Copy link

ContributorAuthor

ro0NL commentedApr 19, 2017

OK.. im getting some root issues here; ie. theFirewallMapInterface::getListeners() actually documents this return type; and now tests are mysteriously broken.FirewallContext doesnt implement it.. but this change seems to have effect elsewere;

Call to undefined method Symfony\Component\Security\Http\Firewall\ContextListener::register() in /var/www/html/symfony/vendor/symfony/symfony/src/Symfony/Component/Security/Http/Firewall.php on line 64

Looking at it..@chalasr any headsup? Should we still do this PR?

Copy link

Member

chalasr commentedApr 19, 2017•
edited
Loading

@ro0NL

diff --git a/src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php b/src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.phpindex a511193..55272ec 100644--- a/src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php+++ b/src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php@@ -124,7 +124,7 @@ class _FirewallMap             return array(array(), null);         }-        return $context->getListeners();+        return array($context->getListeners(), $context->getExceptionListener());     }      /**

should make this green.
HavingFirewallMap::getListeners() returning the current return value ofFirewall::getListeners() (both exception and normal listeners) cannot be changed as the security component provides an array oriented FirewallMap, and its imposed by FirewallMapInterface anyway. This change is still fine to me :)

[SecurityBundle] Enhance FirewallContext::getListeners()

ba65078

Copy link

ContributorAuthor

ro0NL commentedApr 19, 2017

Of course.... thanks :)

ogizanagi approved these changes

Apr 19, 2017

View reviewed changes

nicolas-grekas requested a review fromstof

April 20, 2017 14:40

nicolas-grekas added this to the3.3 milestone

Apr 20, 2017

javiereguiluz approved these changes

Apr 24, 2017

View reviewed changes

Copy link

Member

javiereguiluz left a comment

Choose a reason for hiding this comment

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

👍

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels

Apr 24, 2017

Copy link

Member

fabpot commentedApr 26, 2017

Thank you@ro0NL.

fabpot merged commitba65078 intosymfony:master

Apr 26, 2017

fabpot added a commit that referenced this pull request

Apr 26, 2017

minor#22475[SecurityBundle] Enhance FirewallContext::getListeners()…

0257013

… (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()