Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dddave’s picture

Version: 7.0 » 7.x-dev
Category: task » feature

Can/should this be considered for D7?

SpadXIII’s picture

Title: user_pass_reset() should use tokens for system messages » user_pass_reset() should use format_username for system messages
Issue summary: View changes
Status: Active » Needs review
FileSize
3.54 KB

Small patch that uses format_username in user_pass_reset() messages instead of the $user->name directly.

+1 for bumping, or is it considered reviving yet? :)

SpadXIII’s picture

FileSize
3.11 KB

Oops, I missed a few spots, and reverted the watchdog-messages.
New patch attached!

SpadXIII’s picture

FileSize
6.62 KB

Let's try this again.. Also added format_username in the other occurances for $user->name in the user module.
Left the watchdog message as is.

SpadXIII’s picture

FileSize
6.98 KB

I also just applied this patch: https://www.drupal.org/node/1914628#comment-8288283
With the same changes to the $user->name

MrHaroldA’s picture

Status: Needs review » Needs work

Please don't mix patches/issues; if the other patch is applied, your's won't apply anymore.

SpadXIII’s picture

Sure, just use patch #4
It's only 2 lines that need changing for the other patch to give it the same treatment as this one.

SpadXIII’s picture

Status: Needs work » Needs review
FreekVR’s picture

Status: Needs review » Reviewed & tested by the community

Patch #4 works well and looks good, thanks for this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: format_username_user_pass_reset-1078894-5.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Resetting status after random failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: format_username_user_pass_reset-1078894-5.patch, failed testing.

dcam’s picture

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

Category: Feature request » Bug report
Status: Reviewed & tested by the community » Needs work

There are some patches at #1804160: Always use format_username() when displaying usernames that are older but potentially more complete. (This issue is older, but that one has patches which have been developed longer... I'll cross-link the issues but have no idea which one is a duplicate of which.)

I was going to consider committing this patch as incremental progress, but a couple things gave me pause:

       user_save($account, array('status' => 0));
-      drupal_set_message(t('%name has been disabled.', array('%name' => $account->name)));
+      drupal_set_message(t('%name has been disabled.', array('%name' => format_username($account))));
       watchdog('user', 'Blocked user: %name %email.', array('%name' => $account->name, '%email' => '<' . $account->mail . '>'), WATCHDOG_NOTICE);

Here and elsewhere, having the username displayed on the screen be potentially different than the one recorded in the watchdog logs sounds like a recipe for confusion :)

       user_delete($account->uid);
-      drupal_set_message(t('%name has been deleted.', array('%name' => $account->name)));
+      drupal_set_message(t('%name has been deleted.', array('%name' => format_username($account))));

Doing this after the account has been deleted could cause problems; see #1804160-10: Always use format_username() when displaying usernames.

David_Rothstein’s picture

Title: user_pass_reset() should use format_username for system messages » The User module should use format_username for system messages

Retitling this since the patch touches more than just that one function...

rudiedirkx’s picture

FileSize
6.65 KB

Patch of #4 against 7.41. Not as extensive as #1804160: Always use format_username() when displaying usernames, but it applies and for D7.

talhaparacha’s picture

Status: Needs work » Needs review
FileSize
6.81 KB

In reference to #16, the requirement of saving username before deleting the account has been catered. As far as watchdog-messages are concerned, I think we should keep logging the actual usernames instead of what format_username() returns since the latter could be less informative depending upon the implementation of hook_username_alter. Anyways, I've attached here patches for both scenarios.

I didn't know how to indent these edits properly while keeping the lines under 80 characters. Kindly guide me if those changes are required.

talhaparacha’s picture

The version with watchdog messages edited too...

David_Rothstein’s picture

I think we should keep logging the actual usernames instead of what format_username() returns since the latter could be less informative depending upon the implementation of hook_username_alter.

Hm, I do see the point - in some situations there is value in showing the actual username (the one that the user logs in with) to the administrator who is looking at the logs. But on the other hand, the one from format_username() is the one that is supposed to appear throughout the site's user interface, so not showing it in the logs seems wrong too.

What do others think?

joelstein’s picture

THANK YOU for this patch. This approach makes so much sense to me.

If you're using something like the Real Name module, it's very confusing to show the username stored in the database, which could be something like joel2_123 (if using Email Registration) or something obscure like "Imported User 123" in data migration scenarios.

I think it makes the most sense to show the formatted username in Watchdog, too, because it keeps things consistent and less confusing. Admins digging through the database log will also be more likely to know why a username is formatted differently than what's stored in the database, and will also likely have access to the database if they need it for troubleshooting. Just a thought.

What would it take to commit this? Is more testing needed?

joelstein’s picture

Here's the patch from #20, re-rolled against Drupal 7.50.

David_Rothstein’s picture

Looks like Drupal 8 is not totally consistent about which name it shows in the logs either (in most cases it logs the raw username, but in at least one case it logs the display name).

So maybe to move forward here and not block this issue, we could create a separate issue (for both Drupal 7 and 8) to figure out which is better for the logs... and then just try to keep things as consistent as possible with the patch in this issue.

genellann’s picture

Daluxz’s picture

I rerolled the patch for 7.84