API page: https://api.drupal.org/api/drupal/core%21modules%21user%21user.api.php/f...

> \Drupal\Core\Session\AccountInterface $account: The user object on which the operation is being performed.

That's not true -- AccountInterface is not a 'user object'.

Also, the docs should mention that this hook is invoked both for user entities and the anonymous user session object.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

gnuget’s picture

Issue tags: +Novice
izus’s picture

Status: Active » Needs review
FileSize
713 bytes

hi,
here is a patch for it
Thanks

philipnorton42’s picture

After reviewing where places where this hook is called from the $account object is either a session object (implementing \Drupal\Core\Session\AccountInterface) or a user object (implementing \Drupal\user\UserInterface). So this will not necessarily be an anonymous user object.

Updated the docs for this hook.

scott_euser’s picture

This change looks good to me, but perhaps we should clarify that ultimately either case is an AccountInterface? Or is that implied? In the end the issue is that the description says this is a user object always when it isn't so this change solves it +1 for RTBC for that reason.

philipnorton42’s picture

Good point, but I think it's important to have a distinction between AccountInterface and UserInterface as they have different functions available and as such should be documented well.

When you look at the user profile page this hook is called twice, once for the user profile on the page (which receives UserInterface) and once for the username in the toolbar (which receives AccountInterface). So an example of something that might break is using isActive() to show the status of a user in this hook. This function doesn't exist in AccountInterface and as such shouldn't be relied on in all circumstances.

scott_euser’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense! So specifying both hints to the developer appropriately that they should not expect all methods to be there (like isActive()). Changing to RTBC

Status: Reviewed & tested by the community » Needs work
philipnorton42’s picture

Status: Needs work » Reviewed & tested by the community

The tests failed due to Selenium falling over, which caused the status to go back to "Needs work". This didn't make sense as I hadn't actually changed any code so I re-ran the tests.

Rerunning the tests allowed them to pass :)
Popping it back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/user/user.api.php
    @@ -114,8 +114,9 @@ function hook_user_cancel_methods_alter(&$methods) {
    + * @param \Drupal\Core\Session\AccountInterface|\Drupal\user\UserInterface $account
    

    I think this part of the change is a bit odd. Implementations should only ever rely on Drupal\Core\Session\AccountInterface methods. I can see some benefit in making it clear that you might want to fo something like if instanceof UserInterace... /me shrugs.

  2. +++ b/core/modules/user/user.api.php
    @@ -114,8 +114,9 @@ function hook_user_cancel_methods_alter(&$methods) {
    + *   entity or a user session object.
    

    It's not a user session object. The docs from AccountInterace are
    Defines an account interface which represents the current user..

I think maybe we can do better here.

msankhala’s picture

Status: Needs work » Needs review
FileSize
1.52 KB

Here is an updated patch.

linichalexey’s picture

linichalexey’s picture

Assigned: Unassigned » linichalexey
linichalexey’s picture

Assigned: linichalexey » Unassigned
Status: Needs review » Reviewed & tested by the community

Patch serves the purpose as suggested in #10

catch’s picture

Status: Reviewed & tested by the community » Needs work

The patch doesn't resolve the feedback from #10, it's still talking about 'current user session object'. Also found a couple of other issues:

+++ b/core/modules/user/user.api.php
@@ -118,7 +119,15 @@ function hook_user_cancel_methods_alter(&$methods) {
+ *   user entity or current user session object. If the object is implementation

Should be 'an implementation'.

Also 'user entity methods'.

govind.maloo’s picture

govind.maloo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: interdiff_11_16.diff, failed testing. View results

govind.maloo’s picture

Status: Needs work » Needs review
msankhala’s picture

Status: Needs review » Reviewed & tested by the community

The patch in #16 LGTM. It fixes the problem mentioned in #15. @govind.maloo Nice work. Just a note whenever you are uploading interdiff use the .txt extension instead of .diff or .patch so that testbot do not run a test for interdiff.

alexpott’s picture

+++ b/core/modules/user/user.api.php
@@ -110,7 +110,8 @@ function hook_user_cancel_methods_alter(&$methods) {
+ * user entities and the anonymous user session object.

Looking close, I think anonymous user session object is okay. As basically this is passed the result of \Drupal\Core\Session\AccountProxy::getAccount(). Which does:

      if ($this->id) {
        // After the container is rebuilt, DrupalKernel sets the initial
        // account to the id of the logged in user. This is necessary in order
        // to refresh the user account reference here.
        $this->setAccount($this->loadUserEntity($this->id));
      }
      else {
        $this->account = new AnonymousUserSession();
      }
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
  1. +++ b/core/modules/user/user.api.php
    @@ -118,7 +119,14 @@ function hook_user_cancel_methods_alter(&$methods) {
    + *   use instanceof operator before accessing user entity method. For example:
    

    As @catch said user entity methods

  2. +++ b/core/modules/user/user.api.php
    @@ -118,7 +119,14 @@ function hook_user_cancel_methods_alter(&$methods) {
    + *   @code
    + *     if ($account instanceof UserInterface) {
    + *        // Access user entity methods.
    + *     }
    + *   @endcode
    

    The indentation here should be:

     *   @code
     *   if ($account instanceof UserInterface) {
     *      // Access user entity methods.
     *   }
     *   @endcode
    

Fixed the above on commit.

Committed and pushed b6208e5225 to 8.7.x and ad9f72e1bd to 8.6.x. Thanks!

Backported to 8.6.x as an docs update.

  • alexpott committed b6208e5 on 8.7.x
    Issue #3000240 by govind.maloo, philipnorton42, msankhala, izus,...

  • alexpott committed ad9f72e on 8.6.x
    Issue #3000240 by govind.maloo, philipnorton42, msankhala, izus,...
govind.maloo’s picture

Status: Fixed » Closed (fixed)

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