Skip to content

[Security] Lazy load user providers #23295

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
Jul 11, 2017
Merged

[Security] Lazy load user providers #23295

merged 1 commit into from
Jul 11, 2017

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Jun 25, 2017

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

@chalasr chalasr force-pushed the lazy-user-providers branch from 067288a to 183214f Compare June 25, 2017 11:58
@chalasr chalasr changed the base branch from master to 3.4 June 25, 2017 12:27
@chalasr chalasr force-pushed the lazy-user-providers branch from 183214f to 22a43f6 Compare June 25, 2017 12:27
@chalasr
Copy link
Member Author

chalasr commented Jun 25, 2017

build failure with high deps is normal

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jul 3, 2017
nicolas-grekas
nicolas-grekas reviewed Jul 3, 2017
View reviewed changes
src/Symfony/Component/Security/Core/User/ChainUserProvider.php Outdated
/**
* @param iterable $providers
*/
public function __construct(/* iterable */ $providers)
Copy link
Member

Choose a reason for hiding this comment

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

the comment should be removed

nicolas-grekas
nicolas-grekas reviewed Jul 3, 2017
View reviewed changes
src/Symfony/Component/Security/Http/Firewall/ContextListener.php Outdated
@@ -44,7 +44,7 @@ class ContextListener implements ListenerInterface
private $registered;
private $trustResolver;

public function __construct(TokenStorageInterface $tokenStorage, array $userProviders, $contextKey, LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, AuthenticationTrustResolverInterface $trustResolver = null)
public function __construct(TokenStorageInterface $tokenStorage, /* iterable */ $userProviders, $contextKey, LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, AuthenticationTrustResolverInterface $trustResolver = null)
Copy link
Member

Choose a reason for hiding this comment

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

comment to be removed, that's the job of a docblock, we should not follow this practice IMHO

nicolas-grekas
nicolas-grekas approved these changes Jul 3, 2017
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

with minor comments

@chalasr chalasr force-pushed the lazy-user-providers branch 7 times, most recently from e52986c to 70bcfe2 Compare July 3, 2017 12:25
@chalasr
Copy link
Member Author

chalasr commented Jul 3, 2017

@nicolas-grekas comments addressed. build failure unrelated (see #23360)

stof
stof requested changes Jul 3, 2017
View reviewed changes
src/Symfony/Component/Security/Http/Tests/Firewall/ContextListenerTest.php Outdated
@@ -47,19 +47,6 @@ public function testItRequiresContextKey()
);
}

/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage User provider "stdClass" must implement "Symfony\Component\Security\Core\User\UserProviderInterface
Copy link
Member

Choose a reason for hiding this comment

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

we should still have a test covering this case. You have not removed the exception entirely. You just moved it later in the usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

test readded

@chalasr chalasr force-pushed the lazy-user-providers branch from 70bcfe2 to 4a98272 Compare July 3, 2017 13:40
@chalasr chalasr force-pushed the lazy-user-providers branch from 4a98272 to d7914a6 Compare July 3, 2017 13:41
@chalasr
Copy link
Member Author

chalasr commented Jul 5, 2017

ready.

chalasr
chalasr commented Jul 9, 2017
View reviewed changes
src/Symfony/Component/Security/Http/Firewall/ContextListener.php
* @param EventDispatcherInterface|null $dispatcher
* @param AuthenticationTrustResolverInterface|null $trustResolver
*/
public function __construct(TokenStorageInterface $tokenStorage, $userProviders, $contextKey, LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, AuthenticationTrustResolverInterface $trustResolver = null)
Copy link
Member Author

Choose a reason for hiding this comment

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

full docblock to be removed + iterable typehint to be added to the $userProviders arg when merging into master.

chalasr
chalasr commented Jul 9, 2017
View reviewed changes
src/Symfony/Component/Security/Core/User/ChainUserProvider.php
/**
* @param iterable|UserProviderInterface[] $providers
*/
public function __construct($providers)
Copy link
Member Author

Choose a reason for hiding this comment

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

same, docblock to typehint when merging into master

@nicolas-grekas
Copy link
Member

Thank you @chalasr.

@nicolas-grekas nicolas-grekas merged commit d7914a6 into symfony:3.4 Jul 11, 2017
nicolas-grekas added a commit that referenced this pull request Jul 11, 2017
@nicolas-grekas
feature #23295 [Security] Lazy load user providers (chalasr)
050d686 
This PR was merged into the 3.4 branch.

Discussion
----------

[Security] Lazy load user providers

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

Commits
-------

d7914a6 [Security] Lazy load user providers
@chalasr chalasr deleted the lazy-user-providers branch July 11, 2017 14:15
@javiereguiluz javiereguiluz mentioned this pull request Jul 16, 2017
Closed
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 requested changes

@fabpot fabpot fabpot approved these changes

@dunglas dunglas dunglas approved these changes

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

@ogizanagi ogizanagi ogizanagi approved these changes

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

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