Undefined index: link_attributes in theme_username()

bugfolder commented 1 year ago

Description of the bug

I have been seeing a cascade of three errors appearing in the dblog of my site, appearing at cron time. In chronological order:

Notice: Undefined index: link_attributes in theme_username() (line 83 of /mysite/core/modules/user/user.theme.inc).
Warning: array_merge_recursive(): Expected parameter 1 to be an array, null given in theme_username() (line 83 of /mysite/core/modules/user/user.theme.inc).
TypeError: Argument 1 passed to backdrop_attributes() must be of the type array, null given, called in /mysite/core/includes/common.inc on line 2913 in backdrop_attributes() (line 2833 of /mysite/core/includes/common.inc).

The underlying problem is that theme_username() expects that variables['link_attributes'] will always exist, and it is possible that it doesn't.

Steps To Reproduce

To reproduce the behavior, add this code snippet to something being rendered. (This isn't necessarily what was happening to generate my errors, but it forces the error.)

  $account = user_load(0);
  $build[] = array(
    '#type' => 'help',
    '#markup' => theme('username', array('account' => $account, 'link_path' => '#')),
  );

Actual behavior

This will create the error (and, depending on PHP version, crash Backdrop.)

Expected behavior

There should be no error.

Additional information

The solution is in template_preprocess_username() add another branch to the if/elseif sequence after line 353 to set $variables['link_attributes'] if was not previously set.

Also seems like link_attributes should be documented in the docblock along with the other $variables keys.

A PR will follow.

argiepiano commented 1 year ago

The problem is that user 0 (anonymous) is not really a user, and therefore has no username or profile, right? If you load any other "real" user, the error doesn't happen - template_preprocess_username takes care of initializing $variables['link_attributes'].

When would this situation (trying to theme a username for anonymous) happen in normal operation of core?

bugfolder commented 1 year ago

I don't actually know the specific cause, because it's happening during cron calls on a real site, so I can't easily instrument it. (And why theming of a username is happening during cron is another good question.) But this is just bulletproofing the code (and of course, documenting an undocumented key).

argiepiano commented 1 year ago

I'm all for bulletproofing, but sometimes it's a good idea to know where the bullets are coming from - maybe that's where the problem is. Do you think you can try the following?

Enable Devel
Insert the following in line 72 of theme_username() (right after the function declaration)

  if (empty($variables['link_attributes'])) {
    dpm(ddebug_backtrace(TRUE));
    return '';
  }

Run cron from the Home icon in admin bar and see the backtrace being printed. This will give us a clue of why this is happening, and may help fix that... or be more secure about the bulletproofing.

My guess is that this may be happening because any of these is somehow called during cron: uc_file_remove_user, uc_file_remove_user_file_by_id, uc_roles_delete, or uc_roles_revoke

jenlampton commented 1 year ago

Code LGTM.

bugfolder commented 1 year ago

The cron error is sporadic, so not immediately reproducible (as requested above), but it happened again, so I'll put a backtrace into the watchdog log where I can find it after it happens the next time. Stay tuned. Might take a few days.

bugfolder commented 1 year ago

On further analysis, I don't think we have to wait. From other logs, I can see that the train of errors happens immediately after an email of a role revocation from uc_roles module, so (as @argiepiano guessed) it's almost certainly coming from uc_roles_revoke() (or one of the related functions).

And indeed, throughout Ubercart, there are calls like this:

      backdrop_set_message(t('!user has had the %role role revoked.', array(
        '!user' => theme('username', array(
          'account' => $account,
          'name' => check_plain(user_format_name($account)),
          'link_path' => 'user/' . $account->uid,
        )),
        '%role' => $role_name,
      )));

Note that this is setting the link_path property, which theme_username() handles like this:

function theme_username($variables) {
  if (isset($variables['link_path'])) {
    ...
    $variables['link_options']['attributes'] = array_merge_recursive($variables['link_attributes'], $variables['attributes']);
    ...

Since Ubercart didn't set link_attributes, that leads to the error/crash.

So the issue could be fixed by adding 'link_attributes' => array(), to the various Ubercart calls to theme('username', ...). Or just passing the account to theme('username', (array('account' => $account)) in Ubercart and letting theme_username() handle the name and link path.

But I would suggest that the bulletproofing (and documentation!) of the submitted PR here is still a good thing to do.

bugfolder commented 1 year ago

And to confirm the above, another way to reproduce the error is to use the code that's in Ubercart:

  $account = user_load(1);
  $text = theme('username', array(
    'account' => $account,
    'name' => check_plain(user_format_name($account)),
    'link_path' => 'user/' . $account->uid,
  ));
  $build[] = array(
    '#type' => 'help',
    '#markup' => $text,
  );

Note, though, that the crash only happens if the user executing this code does not have the "View user profiles" permission (due to some logic in template_preprocess_username()). That's why it was only happening during cron calls, because those were being run as the anonymous user, who didn't have that permission.

backdrop / backdrop-issues