-
-
Notifications
You must be signed in to change notification settings - Fork 422
[make:user] user entities implement PasswordAuthenticatedUserInterface #849
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.
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
[make:user] user entities implement PasswordAuthenticatedUserInterface #849
Conversation
6efb0b6
to
2e10b20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks a lot @jrushlow!
$passwordUserInterfaceName = PasswordAuthenticatedUserInterface::class; | ||
} | ||
|
||
$interfaceClassNameDetails = new ClassNameDetails($passwordUserInterfaceName, 'Symfony\Component\Security\Core\User'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need ClassNameDetails
here? Can't we just use the normal class strings?
You might be doing this due to the logic in the template to figure out which location the use
statement should be. I'd prefer passing in a second flag to know that - e.g. is_using_password_authenticated_interface
. That should simplify the template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the full class name for the use
statement and the shortName for the type hint in upgradePassword()
.
We currently already have a $with_password_upgrade
var available in the templates - we could add a $is_using_password_authenticated_interface
var as well, but i think that would make things even messier in the template when we need to determine the sort order of the use statements. Ultimately what we really need is a use statement sorter to solve the messy part - but I'm open to ideas.
Also to consider, #824 introduces the concept of a TemplateClassDetails
object the returns formatted strings for the particular use case. e.g.
$userDetails = new TemplateClassDetails(User::class, true); // second arg === $isTyped
$userDetails->getUseStatement();
// use App\Entity\User;
$userDetails->getConstructorArg();
// User $user
$userDetails->getMethodArg(); // similar to the above but spacing is slightly different
// User $user
$userDetails->getVariable();
// $user
$userDetails->getProperty();
// $this->user
// All of the above add appropriate spacing, casing, trailing commas, new lines, etc..
While 824 is not ready yet, ideally if we merge that in with, a lot of stuff in this PR will be refactored to leverage the new helper and cleanup the templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok then!
Ultimately what we really need is a use statement sorter to solve the messy part
👍
src/Security/UserClassBuilder.php
Outdated
$this->addGetSalt($manipulator, $userClassConfig); | ||
if (06000 < Kernel::VERSION_ID) { | ||
$this->addGetPassword($manipulator, $userClassConfig); | ||
$this->addGetSalt($manipulator, $userClassConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In both getPassword()
and getSalt()
, when using 5.3 or 5.4, the messaging should change. For getSalt()
it should have some comment about how the method is deprecated - and you can remove it once using 6.0. For getPassword()
, the same (unless they are implementing the password interface because they actually need it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the doc blocks for both methods to This method can be removed in Symfony 6.0 - is not needed for apps that do not check user passwords.
unless the a user is checking passwords.
For getSalt()
, once symfony/symfony-docs#15065 is completed, I figure we can refactor the docBlocks to point to that doc (#854) vs pointing to an interface that may not exist / have a lengthy explanation.
43f49ad
to
f9d9d03
Compare
Thanks Jesse! |
Starting in Symfony 5.3 - PR symfony/symfony#40267 introduces the new
PasswordAuthenticatedUserInterface
&aimed at decoupling passwords from theLegacyPasswordAuthenticatedUserInterface
UserInterface
.User
entity implementsPasswordAuthenticatedUserInterface
if it exists.UserRepository::upgradePassword()
method.Authentication related changes are handled in #824
// cc @chalasr