initial commit #37
initial commit #37
Conversation
There was a problem hiding this comment.
Thanks, @padmasreegade, this looks like it's off to a strong start. See below for a few minor suggestions.
| * | ||
| * @param array $data Key-value pairs representing entity properties. | ||
| */ | ||
| public function exchangeArray(array $data): void; |
There was a problem hiding this comment.
I would actually make this a separate interface, like VuFind\Db\Entity\ExchangeArrayInterface and then you could implement it via VuFind\Db\Entity\ExchangeArrayTrait. Then UserEntityInterface could extend ExchangeArrayInterface. This way, we could "mix in" this function to other entities as needed, but we keep the session serialization functionality distinct from all of the other more user-specific functionality.
| @@ -228,7 +254,7 @@ public function updateUserEmail( | |||
| */ | |||
| public function addUserDataToSession(UserEntityInterface $user): void | |||
| { | |||
| if ($user instanceof UserRow) { | |||
| if ($user) { | |||
There was a problem hiding this comment.
I think you can completely get rid of the if/else here, since $user will never be false.
| return $this->getDbTable('User')->createRow(); | ||
| $dql = 'SELECT u ' | ||
| . 'FROM ' . $this->getEntityClass(UserEntityInterface::class) . ' u ' | ||
| . 'WHERE u.password != \'\' ' |
There was a problem hiding this comment.
This might be a little easier to read:
| . 'WHERE u.password != \'\' ' | |
| . "WHERE u.password != '' " |
|
The other challenging thing that you're going to run into, though, is in \VuFind\Auth\Manager, which is currently designed to convert Doctrine entities into Laminas rows. It does this because the authentication system requires objects that implement \LmcRbacMvc\Identity\IdentityInterface, and currently, the entity objects do not, but the Laminas rows do. The easiest short-term solution would be to implement \LmcRbacMvc\Identity\IdentityInterface in the User entity. I think it might be a better idea in the long run to separate these things out, so the identity is perhaps a wrapper around the entity, or something like that -- but we can worry about making it tidier after we have the basic functionality working. :-) |
draft version