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

+            }
+
+            if ($this->repository instanceof UserProviderInterface) {
+                @trigger_error('Implementing loadUserByUsername from Symfony\Component\Security\Core\User\UserProviderInterface is deprecated since version 2.7 and will be removed in 3.0. Use the Symfony\Bridge\Doctrine\Security\User\UserLoaderInterface instead.', E_USER_DEPRECATED);


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

+ * method is asked to load the UserInterface object for the given username
+ * (via loadUserByUsername) so that the rest of the process can continue.
+ *
+ * Internally, a user provider can load users from any source (databases,


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

@@ -54,8 +54,15 @@ public function loadUserByUsername($username)
        if (null !== $this->property) {
            $user = $this->repository->findOneBy(array($this->property => $username));
        } else {
-            if (!$this->repository instanceof UserProviderInterface) {
-                throw new \InvalidArgumentException(sprintf('The Doctrine repository "%s" must implement UserProviderInterface.', get_class($this->repository)));
+            if (!$this->repository instanceof UserLoaderInterface) {


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

+                    throw new \InvalidArgumentException(sprintf(
+                        'The Doctrine repository "%s" must implement UserLoaderInterface.',
+                        get_class($this->repository)
+                    ));


this should be kept on one line

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

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

-                throw new \InvalidArgumentException(sprintf('The Doctrine repository "%s" must implement UserProviderInterface.', get_class($this->repository)));
+            if (!$this->repository instanceof UserLoaderInterface) {
+                if (!$this->repository instanceof UserProviderInterface) {
+                    throw new \InvalidArgumentException(sprintf('The Doctrine repository "%s" must implement UserLoaderInterface.', get_class($this->repository)));


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

+     *
+     * @see UsernameNotFoundException
+     *
+     * @throws UsernameNotFoundException if the user is not found


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

+     *
+     * @param string $username The username
+     *
+     * @return UserInterface


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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added UserLoaderInterface for loading users through Doctrine. #15947

Added UserLoaderInterface for loading users through Doctrine. #15947

mtrojanowski commented Sep 27, 2015

mtrojanowski Sep 27, 2015

stof Sep 28, 2015

weaverryan commented Sep 27, 2015

linaori commented Sep 28, 2015

stof commented Sep 28, 2015

stof commented Sep 28, 2015

stof Sep 28, 2015

mtrojanowski Sep 28, 2015

mtrojanowski Sep 28, 2015

mtrojanowski commented Sep 28, 2015

stof commented Sep 29, 2015

mtrojanowski commented Sep 29, 2015

xabbuh Sep 30, 2015

mtrojanowski Sep 30, 2015

xabbuh Sep 30, 2015

mtrojanowski Sep 30, 2015

fabpot commented Oct 7, 2015

stof Oct 7, 2015

fabpot Oct 7, 2015

fabpot Oct 11, 2015

fabpot commented Oct 11, 2015

fabpot Oct 11, 2015

mtrojanowski Oct 13, 2015

fabpot commented Oct 13, 2015

fabpot commented Oct 13, 2015

mtrojanowski commented Oct 13, 2015

fabpot commented Oct 16, 2015

Added UserLoaderInterface for loading users through Doctrine. #15947

Added UserLoaderInterface for loading users through Doctrine. #15947

Conversation

mtrojanowski commented Sep 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Sep 27, 2015

linaori commented Sep 28, 2015

stof commented Sep 28, 2015

stof commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrojanowski commented Sep 28, 2015

stof commented Sep 29, 2015

mtrojanowski commented Sep 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 13, 2015

fabpot commented Oct 13, 2015

mtrojanowski commented Oct 13, 2015

fabpot commented Oct 16, 2015