[Security] [Doctrine] Added BaseUserProviderInterface #12733
Conversation
You can now implement BaseUserProviderInterface without the need of implementing `supportsClass` function.
| @@ -54,7 +54,7 @@ public function loadUserByUsername($username) | |||
| if (null !== $this->property) { | |||
| $user = $this->repository->findOneBy(array($this->property => $username)); | |||
| } else { | |||
| if (!$this->repository instanceof UserProviderInterface) { | |||
| if (!$this->repository instanceof BaseUserProviderInterface) { | |||
| throw new \InvalidArgumentException(sprintf('The Doctrine repository "%s" must implement UserProviderInterface.', get_class($this->repository))); | |||
There was a problem hiding this comment.
Souldn't the message of exception be changed as well (Classname) ?
There was a problem hiding this comment.
Of course you're right. Fixed that.
* Removed unneeded `use` statements. * Fixed error message to reference the new interface * Changed `refreshUser` to compare to the new interface
| @@ -77,7 +77,7 @@ public function refreshUser(UserInterface $user) | |||
| throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', get_class($user))); | |||
| } | |||
|
|
|||
| if ($this->repository instanceof UserProviderInterface) { | |||
| if ($this->repository instanceof BaseUserProviderInterface) { | |||
There was a problem hiding this comment.
Is it really needed to move the refreshUser() method to the base interface too? Usually you would have users that have an identifier which can be passed to the repository's find() method, wouldn't you?
|
Hey @mtrojanowski! So the use-case for this So, I'm going to propose an evolution, that I think will be less intrusive (e.g. it won't touch the very central UserProviderInterface in any way, even a BC-way). What if we: A) Introduce a new interface inside of the Doctrine Bridge - something like B) In EntityUserProvider, we basically have take the existing What do you think? P.S. Nice avatar ;) |
|
I'm with @weaverryan here. Your BaseUserProviderInterface makes no sense in the context of the Security component. So a separate interface in the bridge to be used by the bridge to allow simpler implementations for Doctrine repositories would be better. Can you work on this @mtrojanowski ? |
|
@stof, sure. I'll try to propose a new solution according to your ideas. |
|
Closing as there is no activity. @mtrojanowski Feel free to open a new PR implementing @weaverryan suggestions. |
…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.
You can now implement BaseUserProviderInterface without the need of implementing
supportsClassfunction.