Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
System messages displayed in user_pass_reset() don't use tokens to display the username. Now that tokens are built in to D7 should this code be updated? I am using hook_tokens_alter() to change the way usernames are displayed so if would be useful to update this code for consistency.
Comment | File | Size | Author |
---|---|---|---|
#26 | drupal-format_username-1078894-26.patch | 9.81 KB | Daluxz |
#25 | drupal-format_username-1078894-25.patch | 9.86 KB | genellann |
#23 | drupal-format_username-1078894-23.patch | 9.88 KB | joelstein |
#20 | the_user_module_should-1078894-20.patch | 10.48 KB | talhaparacha |
#19 | the_user_module_should-1078894-19.patch | 6.81 KB | talhaparacha |
Comments
Comment #1
dddave CreditAttribution: dddave commentedCan/should this be considered for D7?
Comment #2
SpadXIII CreditAttribution: SpadXIII commentedSmall 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? :)
Comment #3
SpadXIII CreditAttribution: SpadXIII commentedOops, I missed a few spots, and reverted the watchdog-messages.
New patch attached!
Comment #4
SpadXIII CreditAttribution: SpadXIII commentedLet'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.
Comment #5
SpadXIII CreditAttribution: SpadXIII commentedI also just applied this patch: https://www.drupal.org/node/1914628#comment-8288283
With the same changes to the $user->name
Comment #6
MrHaroldA CreditAttribution: MrHaroldA commentedPlease don't mix patches/issues; if the other patch is applied, your's won't apply anymore.
Comment #7
SpadXIII CreditAttribution: SpadXIII commentedSure, 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.
Comment #8
SpadXIII CreditAttribution: SpadXIII commentedComment #9
FreekVR CreditAttribution: FreekVR commentedPatch #4 works well and looks good, thanks for this.
Comment #12
dcam CreditAttribution: dcam commentedResetting status after random failure.
Comment #15
dcam CreditAttribution: dcam commentedComment #16
David_Rothstein CreditAttribution: David_Rothstein commentedThere 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:
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 :)
Doing this after the account has been deleted could cause problems; see #1804160-10: Always use format_username() when displaying usernames.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedRetitling this since the patch touches more than just that one function...
Comment #18
rudiedirkx CreditAttribution: rudiedirkx commentedPatch of #4 against 7.41. Not as extensive as #1804160: Always use format_username() when displaying usernames, but it applies and for D7.
Comment #19
talhaparacha CreditAttribution: talhaparacha as a volunteer commentedIn 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.
Comment #20
talhaparacha CreditAttribution: talhaparacha as a volunteer commentedThe version with watchdog messages edited too...
Comment #21
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedHm, 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?
Comment #22
joelstein CreditAttribution: joelstein commentedTHANK 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?
Comment #23
joelstein CreditAttribution: joelstein commentedHere's the patch from #20, re-rolled against Drupal 7.50.
Comment #24
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedLooks 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.
Comment #25
genellann CreditAttribution: genellann at On Fire Media commentedNew patch for 7.78
Comment #26
Daluxz CreditAttribution: Daluxz at iO commentedI rerolled the patch for 7.84