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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienGR created an issue. See original summary.

DamienGR’s picture

Status: Active » Needs review
FileSize
772 bytes
borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This looks like a solid documentation improvement, I checked and it is indeed the account that is passed trough the hooks.

Berdir’s picture

Since 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.

kala4ek’s picture

Assigned: DamienGR » Unassigned

Moreover, other example hooks from user.api.php have the right PHPDoc.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/user.api.php
@@ -130,7 +130,7 @@ function hook_user_format_name_alter(&$name, $account) {
 function hook_user_login($account) {

@@ -144,7 +144,7 @@ function hook_user_login($account) {
+ * @param \Drupal\Core\Session\AccountInterface $account

We 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() and user_user_logout() to use the type hint.

Prashant.c’s picture

Adding Type hints to hook_user_login(), hook_user_logout and hook_user_cancel().

borisson_’s picture

Used the FQN instead of only the class name and added a space before the accountInterface argument. Interdiff is to #7.

alexpott’s picture

Status: Needs review » Needs work

The correct typehint for login is UserInterface - look at user_login_finalize(). For logout it is AccountInterface.

alexpott’s picture

Also 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()

rakesh.gectcr’s picture

Status: Needs review » Needs work

The last submitted patch, 11: interdiff-2954825-8-11.patch, failed testing. View results

alexpott’s picture

  1. +++ b/core/modules/system/tests/modules/session_test/session_test.module
    @@ -8,7 +8,7 @@
    -function session_test_user_login($account) {
    +function session_test_user_login(\Drupal\user\UserInterface $account) {
    

    Should have a use statement

  2. +++ b/core/modules/user/user.api.php
    @@ -144,10 +144,10 @@ function hook_user_login($account) {
    -function hook_user_logout($account) {
    +function hook_user_logout(\Drupal\user\UserInterface $account) {
    

    This is not passed the user it is passed the account proxy - so needs to be the AccountInterface. See user_logout().

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
5.74 KB
73 bytes

Status: Needs review » Needs work

The last submitted patch, 14: interdiff-2954825-11-14.patch, failed testing. View results

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
2.07 KB

The last submitted patch, 11: 2954825-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

borisson_’s picture

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.

rakesh.gectcr’s picture

That 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.

amietpatial’s picture

FileSize
158.24 KB
140.92 KB

@rakesh.gectcr I have applied your patch it applies cleanly but I have point that function user_user_login(UserInterface $account) should be function user_user_login(AccountInterface $account)?

-function hook_user_login($account) {
+function hook_user_login(\Drupal\user\UserInterface $account) {
   $config = \Drupal::config('system.date');
-function hook_user_login($account) {
+function hook_user_login(\Drupal\user\AccountInterface $account) {
   $config = \Drupal::config('system.date');
rakesh.gectcr’s picture

Issue tags: +Nwdug_may18

Adding tag for the sprint weekend

tresti88’s picture

as part of Nwdug_may18, i will update the patch

tresti88’s picture

Attached is an updated patch and interdiff as per comments

tresti88’s picture

Missed off changes from the previous patch in #19 (In other words please ignore comment #23)

See updated patch file

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

Could do with an issue summary update to list all the changes that the patch should make.

gnuget’s picture

Issue tags: +Novice
gnuget’s picture

Title: hook_user_login/logout $account parameter should be of type AccountInterface » Update the documentation and add the correct type hints in the user's hooks
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Novice, -Needs issue summary update

I just updated the summary.

I reviewed the patch and I found a problem here:

+++ b/core/modules/user/user.api.php
@@ -130,10 +133,10 @@ function hook_user_format_name_alter(&$name, $account) {
-function hook_user_login($account) {
+function hook_user_login(AccountInterface $account) {

Here must be UserInterface not AccountInterface

I'm going to reroll the patch and give it a more detailed review.

gnuget’s picture

Status: Needs work » Needs review
FileSize
2.43 KB
7.2 KB

Ok.

New patch attached, I grep the core looking if all the hooks have the correct type hint:

hook_user_cancel

grep -EHrin "function [a-z_]+_user_cancel\(.*\)" .
./modules/comment/comment.module:522:function comment_user_cancel($edit, UserInterface $account, $method) {
./modules/user/user.api.php:42:function hook_user_cancel($edit, UserInterface $account, $method) {
./modules/history/history.module:163:function history_user_cancel($edit, UserInterface $account, $method) {
./modules/node/node.module:717:function node_user_cancel($edit, UserInterface $account, $method) {

hook_user_login

 grep -EHrin "function [a-z_]+_user_login\(.*\)" .
./modules/user/user.module:572:function user_user_login(UserInterface $account) {
./modules/user/user.api.php:139:function hook_user_login(UserInterface $account) {
./modules/system/tests/modules/session_test/session_test.module:13:function session_test_user_login(UserInterface $account) {
./modules/system/system.module:834:function system_user_login(UserInterface $account) {

hook_user_format_name_alter

grep -EHrin "function [a-z_]+_user_format_name_alter\(.*\)" .
./modules/user/tests/modules/user_hooks_test/user_hooks_test.module:14:function user_hooks_test_user_format_name_alter(&$name, AccountInterface $account) {
./modules/user/user.api.php:126:function hook_user_format_name_alter(&$name, AccountInterface $account) {

hook_user_login

grep -EHrin "function [a-z_]+_user_login\(.*\)" .
./modules/user/user.module:572:function user_user_login(UserInterface $account) {
./modules/user/user.api.php:139:function hook_user_login(UserInterface $account) {
./modules/system/tests/modules/session_test/session_test.module:13:function session_test_user_login(UserInterface $account) {
./modules/system/system.module:834:function system_user_login(UserInterface $account) {

hook_user_logout

grep -EHrin "function [a-z_]+_user_logout\(.*\)" .
./modules/user/user.module:581:function user_user_logout(AccountInterface $account) {
./modules/user/user.api.php:160:function hook_user_logout(AccountInterface $account) {

And I rerolled and fixed a couple things.

Patch and interdiff attached.

gnuget’s picture

Issue summary: View changes
gnuget’s picture

Issue summary: View changes
gnuget’s picture

Title: Update the documentation and add the correct type hints in the user's hooks » Update the user.api.php documentation and add the correct type hints in the user's hooks implementations
joachim’s picture

Status: Needs review » Needs work

> 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:

  public function getDisplayName() {
    $name = $this->getAccountName() ?: \Drupal::config('user.settings')->get('anonymous');
    \Drupal::moduleHandler()->alter('user_format_name', $name, $this);
    return $name;
  }
Berdir’s picture

It is also called in \Drupal\Core\Session\UserSession::getDisplayName(), where it is *not*.

joachim’s picture

Status: Needs work » Reviewed & tested by the community

Thanks 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Unfortunately needs a reroll.

gnuget’s picture

Status: Needs work » Needs review
FileSize
7.22 KB

Done.

Thanks!

John Cook’s picture

Status: Needs review » Reviewed & tested by the community

I'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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 400413f and pushed to 8.7.x. Thanks!
Committed ac623bf and pushed to 8.6.x. Thanks!

  • alexpott committed 400413f on 8.7.x
    Issue #2954825 by rakesh.gectcr, tresti88, gnuget, borisson_, DamienGR,...

  • alexpott committed ac623bf on 8.6.x
    Issue #2954825 by rakesh.gectcr, tresti88, gnuget, borisson_, DamienGR,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.