Added UserLoaderInterface for loading users through Doctrine. #15947

mtrojanowski · 2015-09-27T21:21:19Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#11157
License	MIT
Doc PR	symfony/symfony-docs#4543

Based on the @weaverryan and @stof propositions from this discussion I created a new interface in the Doctrine Bridge which can be used to more easily implement a repository capable of returning a User.

mtrojanowski · 2015-09-27T21:22:44Z

src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php

I am not sure if the deprecation message is right here. Also what should be the Symfony version stated in this message? And if this behaviour will in fact be deprecated should I add a phpdoc comment with deprecation notice next to the loadUserByUsername method in UserProviderInterface?

This should be triggered only if the class does not implement UserLoaderInterface, to be able to implement both interfaces in case you need to keep BC with older versions

weaverryan · 2015-09-27T23:21:53Z

There's also logic down in refreshUser that that can call out to the entity repository, and it feels a little inconsistent to have a new interface for just the loadUserByUsername() method. But, in practice, that's the only of the 3 that needs to be overridden. So I think it's ok.

linaori · 2015-09-28T05:41:06Z

Maybe we can bring up this issue as a discussion again: #5678

stof · 2015-09-28T12:31:56Z

@weaverryan the whole point is that you should not change the way the user is refreshed. Refreshing is done by primary key on purpose, to avoid a security issue in case your username is mutable. this is precisely why we provide a dedicated new way

stof · 2015-09-28T12:32:25Z

This should go in the 2.8 branch when merging though. It is a new feature.

stof · 2015-09-28T12:34:50Z

src/Symfony/Bridge/Doctrine/Security/User/UserLoaderInterface.php

this description is wrong. The UserLoaderInterface is meant to load objects from Doctrine

I changed the description of the interface. I hope now it's more appropriate.

mtrojanowski · 2015-09-28T21:33:53Z

src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php

I rephrased the clause a bit as the deprecation warning should be thrown if the UserLoaderInterface is not being implemented.

mtrojanowski · 2015-09-28T21:36:17Z

@stof If it's not a problem I will close this PR and open another one for the 2.8 branch once we decide that all the changes here are ok.

stof · 2015-09-29T08:46:20Z

@mtrojanowski not really needed as we can switch branch with our merging tool

mtrojanowski · 2015-09-29T20:36:46Z

@stof Great :) Do you think anything else need to be fixed here?

xabbuh · 2015-09-30T19:41:04Z

src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php

this should be kept on one line

xabbuh · 2015-09-30T21:30:06Z

src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php

I think you should include the namespace in the interface name.

fabpot · 2015-10-07T12:53:50Z

👍 ping @symfony/deciders

stof · 2015-10-07T12:59:23Z

src/Symfony/Bridge/Doctrine/Security/User/UserLoaderInterface.php

I'm wondering whether we should ask people to throw this exception themselves, or just return UserInterface|null. the EntityUserProvider already handles the case of null anyway, and this would be more in line with the way Doctrine return values work (this would allow to implement it as a findOneBy() call for instance)

This one should be removed.

fabpot · 2015-10-11T11:38:37Z

@mtrojanowski Can you add some tests?

fabpot · 2015-10-11T11:39:23Z

src/Symfony/Bridge/Doctrine/Security/User/UserLoaderInterface.php

Should be @return UserInterface|null

fabpot · 2015-10-13T07:51:45Z

@mtrojanowski Looks good now, can you add some tests for this new feature?

fabpot · 2015-10-13T07:53:59Z

You should also add a note in the CHANGELOG file of the Doctrine Bridge.

mtrojanowski · 2015-10-13T07:57:53Z

@fabpot I added some tests for the EntityUserProvider that check if proper interface is being used. I will also add a note to CHANGELOG.

fabpot · 2015-10-16T11:51:42Z

Thank you @mtrojanowski.

@weaverryan

…ctrine. (mtrojanowski) This PR was submitted for the 2.7 branch but it was merged into the 2.8 branch instead (closes #15947). Discussion ---------- Added UserLoaderInterface for loading users through Doctrine. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #11157 | License | MIT | Doc PR | symfony/symfony-docs#4543 Based on the @weaverryan and @stof propositions from [this](#12733 (comment)) discussion I created a new interface in the Doctrine Bridge which can be used to more easily implement a repository capable of returning a User. Commits ------- a8d3d12 Added UserLoaderInterface for loading users through Doctrine.

carsonbot added the Status: Needs Review label Sep 27, 2015

mtrojanowski reviewed Sep 27, 2015
View reviewed changes

stof reviewed Sep 28, 2015
View reviewed changes

mtrojanowski force-pushed the simplify-user-provider-interface branch 2 times, most recently from 36ac3e4 to e2cc24d Compare September 28, 2015 21:32

mtrojanowski reviewed Sep 28, 2015
View reviewed changes

Added UserLoaderInterface for loading users through Doctrine.

e32fcc0

mtrojanowski force-pushed the simplify-user-provider-interface branch from 65b9708 to e32fcc0 Compare September 29, 2015 20:34

xabbuh reviewed Sep 30, 2015
View reviewed changes

Change of author and formatting fix.

414fd16

xabbuh reviewed Sep 30, 2015
View reviewed changes

Added namespace to exception message.

e463cc2

stof reviewed Oct 7, 2015
View reviewed changes

fabpot reviewed Oct 11, 2015
View reviewed changes

Michal Trojanowski added 2 commits October 13, 2015 08:55

The interface should not throw UsernameNotFoundException anymore.

6bdfd0b

Removed unnecessary @see annotation.

1466957

Added new unit tests for EntityUserProvider.

1b8f219

fabpot closed this Oct 16, 2015

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

Uh oh!

Added UserLoaderInterface for loading users through Doctrine. #15947

Added UserLoaderInterface for loading users through Doctrine. #15947

Uh oh!

Conversation

mtrojanowski commented Sep 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Sep 27, 2015

Uh oh!

linaori commented Sep 28, 2015

Uh oh!

stof commented Sep 28, 2015

Uh oh!

stof commented Sep 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrojanowski commented Sep 28, 2015

Uh oh!

stof commented Sep 29, 2015

Uh oh!

mtrojanowski commented Sep 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Oct 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Oct 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Oct 13, 2015

Uh oh!

fabpot commented Oct 13, 2015

Uh oh!

mtrojanowski commented Oct 13, 2015

Uh oh!

fabpot commented Oct 16, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants