Skip to content

[SecurityBundle] Lazy load security listeners #23114

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
Jun 9, 2017
Merged

[SecurityBundle] Lazy load security listeners #23114

merged 1 commit into from
Jun 9, 2017

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Jun 9, 2017 •
edited
Loading

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? yes (edge case)
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Each of them is heavy and unused if a previous one sets a response or breaks in the middle.

@chalasr chalasr force-pushed the security/lazy-listeners branch from abcbb4b to f3bbf31 Compare June 9, 2017 08:06
nicolas-grekas
nicolas-grekas reviewed Jun 9, 2017
View reviewed changes
src/Symfony/Bundle/SecurityBundle/Security/FirewallContext.php Outdated
@@ -25,7 +25,7 @@ class FirewallContext
private $exceptionListener;
private $config;

public function __construct(array $listeners, ExceptionListener $exceptionListener = null, FirewallConfig $config = null)
public function __construct(/* iterable */ $listeners, ExceptionListener $exceptionListener = null, FirewallConfig $config = null)
Copy link
Member

@nicolas-grekas nicolas-grekas Jun 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

to prevent any BC break, should getListeners() use iterator_to_array() then?

Copy link
Member Author

Choose a reason for hiding this comment

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

that would break laziness as the firewall uses getListeners() for iterating over listeners. Not sure what should be the condition for transforming to array, we did not in #21516 nor #21450

Copy link
Member

@nicolas-grekas nicolas-grekas Jun 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

do we care about laziness when getListeners is called? I think we don't. Right now, it's used only for debugging, where we don't care being lazy or not.

Copy link
Member

Choose a reason for hiding this comment

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

we did not in #21516 nor #21450

but there are no getters there...

Copy link
Member Author

Choose a reason for hiding this comment

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

We do care, it's always used https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Http/Firewall.php#L61

Copy link
Member

Choose a reason for hiding this comment

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

OK, I missed that. Then we should be clear about that in the CHANGELOG at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

CHANGELOG+UPGRADE entry added

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jun 9, 2017
@chalasr chalasr changed the base branch from master to 3.4 June 9, 2017 09:20
@chalasr chalasr force-pushed the security/lazy-listeners branch 2 times, most recently from 3f97646 to 7e3aa5f Compare June 9, 2017 09:21
nicolas-grekas
nicolas-grekas reviewed Jun 9, 2017
View reviewed changes
src/Symfony/Bundle/SecurityBundle/Security/FirewallContext.php Outdated
@@ -49,6 +49,12 @@ public function getContext()

public function getListeners()
{
if (is_array($this->listeners)) {
@trigger_error
Copy link
Member

Choose a reason for hiding this comment

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

to be removed :)

src/Symfony/Bundle/SecurityBundle/Security/FirewallContext.php Outdated
@@ -49,6 +49,12 @@ public function getContext()

public function getListeners()
Copy link
Member

Choose a reason for hiding this comment

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

lets add a return type hint here

UPGRADE-3.4.md Outdated
SecurityBundle
--------------

* `FirewallContext::getListeners()` now returns a `\Traversable` object by default instead of an
Copy link
Member

Choose a reason for hiding this comment

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

FirewallContext::getListeners() now returns \Traversable|array?

@chalasr chalasr force-pushed the security/lazy-listeners branch from 7e3aa5f to 09d71e4 Compare June 9, 2017 10:54
@chalasr
Copy link
Member Author

chalasr commented Jun 9, 2017

All fixed, typehints to be uncommented when merging into master

@chalasr chalasr force-pushed the security/lazy-listeners branch 3 times, most recently from cadac61 to a2e5531 Compare June 9, 2017 15:29
@chalasr
Copy link
Member Author

chalasr commented Jun 9, 2017 •
edited
Loading

Build failure unrelated, seems like apcu ext is unavailable in travis builds.

@chalasr chalasr force-pushed the security/lazy-listeners branch 2 times, most recently from c3807c9 to 1ec82ed Compare June 9, 2017 15:50
@chalasr chalasr force-pushed the security/lazy-listeners branch from 1ec82ed to e3ee6bc Compare June 9, 2017 15:58
@fabpot
Copy link
Member

fabpot commented Jun 9, 2017

Thank you @chalasr.

@fabpot fabpot merged commit e3ee6bc into symfony:3.4 Jun 9, 2017
fabpot added a commit that referenced this pull request Jun 9, 2017
@fabpot
feature #23114 [SecurityBundle] Lazy load security listeners (chalasr)
63ecc9c 
This PR was merged into the 3.4 branch.

Discussion
----------

[SecurityBundle] Lazy load security listeners

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes (edge case)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Each of them is heavy and unused if a previous one sets a response or breaks in the middle.

Commits
-------

e3ee6bc Lazy load security listeners
@chalasr chalasr deleted the security/lazy-listeners branch June 9, 2017 18:25
stof
stof reviewed Sep 8, 2017
View reviewed changes
src/Symfony/Bundle/SecurityBundle/CHANGELOG.md
3.4.0
-----

* [BC BREAK] `FirewallContext::getListeners()` now returns `\Traversable|array`
Copy link
Member

Choose a reason for hiding this comment

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

\Traversable|array is iterable

This was referenced Oct 18, 2017
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

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

Assignees
No one assigned
Labels
Feature SecurityBundle Status: Needs Review
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@chalasr @fabpot @nicolas-grekas @stof @carsonbot