Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The user's hooks haven't the proper type in the documentation of the $account param (see user.api.php
) and the type hint is missing in the user.api.php
file and in the modules which implements these hooks.
Proposed resolution
- Update the hook's implementations and include the correct type hint.
- Update the hook documentation in the
user.api.php
file.
The param account must have different typehint depending of the hook:
Hook | Typehint |
---|---|
hook_user_cancel | UserInterface |
hook_user_format_name_alter | AccountInterface |
hook_user_login | UserInterface |
hook_user_logout | AccountInterface |
Remaining tasks
Review and commit.
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#37 | 2954825-37.patch | 7.22 KB | gnuget |
#29 | 2954825-29.patch | 7.2 KB | gnuget |
#29 | 2954825-24-29-interdiff.txt | 2.43 KB | gnuget |
#24 | interdiff-14-24.txt | 2.4 KB | tresti88 |
#24 | 2954825-24.patch | 6.29 KB | tresti88 |
Comments
Comment #2
DamienGR CreditAttribution: DamienGR as a volunteer commentedComment #3
borisson_This looks like a solid documentation improvement, I checked and it is indeed the account that is passed trough the hooks.
Comment #4
BerdirSince this is just hook documentation and not like an interface where adding a type hint breaks existing implementations, we could actually also add the explicit type hint. Leaving the decision to core maintainers to add that here as well or not.
Comment #5
kala4ekMoreover, other example hooks from user.api.php have the right PHPDoc.
Comment #6
alexpottWe can add the type hint to the function too.
The correct typehint for login is UserInterface - look at user_login_finalize(). For logout it is AccountInterface.
We already have
function system_user_login(UserInterface $account) {
Also we should
update user_user_login()
anduser_user_logout()
to use the type hint.Comment #7
Prashant.cAdding Type hints to hook_user_login(), hook_user_logout and hook_user_cancel().
Comment #8
borisson_Used the FQN instead of only the class name and added a space before the accountInterface argument. Interdiff is to #7.
Comment #9
alexpottComment #10
alexpottAlso as we're adding typehints lets add them to the implementations like comment_user_cancel() for example.
Plus hook_user_cancel is UserInterface - see user_cancel()
Comment #11
rakesh.gectcrComment #13
alexpottShould have a use statement
This is not passed the user it is passed the account proxy - so needs to be the AccountInterface. See user_logout().
Comment #14
rakesh.gectcrComment #16
rakesh.gectcrComment #18
borisson_Something went wrong on the testbot,
PHP 7 & MySQL 5.5 114 pass
114 tests doesn't seem like that is the correct number of tests.Comment #19
rakesh.gectcrThat should be my bad.... I have deleted the interdiff , which I uploaded earlier with .patch extenssion....
I am uploading the patch ones again for runnning the test.
Comment #20
amietpatial CreditAttribution: amietpatial as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented@rakesh.gectcr I have applied your patch it applies cleanly but I have point that
function user_user_login(UserInterface $account)
should befunction user_user_login(AccountInterface $account)
?Comment #21
rakesh.gectcrAdding tag for the sprint weekend
Comment #22
tresti88as part of Nwdug_may18, i will update the patch
Comment #23
tresti88Attached is an updated patch and interdiff as per comments
Comment #24
tresti88Missed off changes from the previous patch in #19 (In other words please ignore comment #23)
See updated patch file
Comment #26
joachim CreditAttribution: joachim commentedCould do with an issue summary update to list all the changes that the patch should make.
Comment #27
gnugetComment #28
gnugetI just updated the summary.
I reviewed the patch and I found a problem here:
Here must be UserInterface not AccountInterface
I'm going to reroll the patch and give it a more detailed review.
Comment #29
gnugetOk.
New patch attached, I grep the core looking if all the hooks have the correct type hint:
hook_user_cancel
hook_user_login
hook_user_format_name_alter
hook_user_login
hook_user_logout
And I rerolled and fixed a couple things.
Patch and interdiff attached.
Comment #30
gnugetComment #31
gnugetComment #32
gnugetComment #33
joachim CreditAttribution: joachim commented> Here must be UserInterface not AccountInterface
The patch doesn't have this change.
hook_user_format_name_alter() is invoked here in the User entity, so $this is a UserInterface:
Comment #34
BerdirIt is also called in \Drupal\Core\Session\UserSession::getDisplayName(), where it is *not*.
Comment #35
joachim CreditAttribution: joachim commentedThanks for the clarification!
In that case this is RTBC.
But I will file a follow-up for cleaning up the docs for hook_user_format_name_alter() in light of #34.
-- done: #3000240: hook_user_format_name_alter() should document that it's invoked for both user entities and session objects.
Comment #36
alexpottUnfortunately needs a reroll.
Comment #37
gnugetDone.
Thanks!
Comment #38
John Cook CreditAttribution: John Cook at Creode commentedI've check the re-roll by @gnuget in comment 37.
All of the functions in comment 29 have been changed and no new functions have been added.
Setting back to RTBC.
Comment #39
alexpottCommitted 400413f and pushed to 8.7.x. Thanks!
Committed ac623bf and pushed to 8.6.x. Thanks!