API page: http://api.drupal.org/api/drupal/modules%21user%21user.module/function/u...
In user.module, function user_pass_rehash has defined the $login parameter like so:
* @param $login
* The user account login name.
*
But throughout Drupal core's use of user_pass_rehash()
, $account->login
is passed for the $login
param. $account->login
is a timestamp of the user's last login, not the user account login name as suggested by user_pass_rehash()
. Or am I missing something?
Here's a patch to update the documentation for the function.
Comment | File | Size | Author |
---|---|---|---|
#16 | user-pass-rehash-docs-1665446-16.patch | 2.58 KB | Sivaji_Ganesh_Jojodae |
#12 | user-pass-rehash-docs-1665446-12.patch | 2.79 KB | kostajh |
#9 | user-pass-rehash-docs-1665446-9.patch | 2.84 KB | kostajh |
#7 | user-pass-rehash-docs-1665446-7.patch | 2.68 KB | kostajh |
#6 | user-pass-rehash-docs-1665446-6.patch | 1.85 KB | kostajh |
Comments
Comment #1
kostajh CreditAttribution: kostajh commentedComment #2
jhodgdonWow, good catch! I totally agree with you on your assessment -- $account->login is always passed to this function, and $account->login is coming from the database {users}.login, which is the timestamp of the last login.
I think the documentation patch could make this clearer though -- maybe say "the timestamp of the last login" rather than "The user account login timestamp" (which I don't think really says which timestamp you are talking about)?
Also, I'm moving this to the user.module component, at least temporarily, because I think the maintainers of the user.module component should weigh in on whether this was the intended use for the function (to use the last login time in the hash, as the code is doing, vs. the login name, as the documentation says).
Comment #3
jhodgdonIn other words, I'm not sure whether this is a code bug or a documentation bug, and I hope that the user.module maintainers will weigh in on that.
Comment #4
kostajh CreditAttribution: kostajh commentedThanks for the feedback and review! In this new patch I changed the language describing the $login parameter, I hope it is clearer now. The new patch also provides correct information on the $login parameter for two other functions in user.module. It does seem like a documentation bug to me, not a code issue.
Comment #5
jhodgdonThanks -- I think this patch looks good, with the exception of the capitalization of "unix" (apparently it should be UNIX: http://en.wikipedia.org/wiki/Unix).
I'd still like the user.module maintainers to weigh in on whether this is a code bug or not...
Comment #6
kostajh CreditAttribution: kostajh commentedOk here's a new patch with UNIX capitalized.
Comment #7
kostajh CreditAttribution: kostajh commentedFound another function that had the $login parameter described incorrectly, this patch covers that function as well.
Comment #8
jhodgdonThanks! This is looking pretty good, although someone should independently double-check all of the functions in user.module that have a $login parameter and make sure they are all documented correctly (it's possible some of them might use $login for the login name, but I think $name is the usual parameter name... anyway we need to double check them).
I also have three minor suggestions:
a)
==> The user ID number.
b)
==> The UNIX timestamp...
(it's a specific time stamp, so needs "the" not "a")
c)
I think this should say something about how this is used... is it just any UNIX timestamp, or is it used for expiration of the login link or something?
Comment #9
kostajh CreditAttribution: kostajh commentedOk, I made the requested changes.
I think this should say something about how this is used... is it just any UNIX timestamp, or is it used for expiration of the login link or something?
I feel like this is explained in the function documentation for
user_pass_rehash()
but I did add a note saying that this is typically REQUEST_TIME. At least, that is what user.module passes into the function.Comment #10
jhodgdonThat looks good to me, as far as formatting, grammar, and clarity. Thanks!
Someone other than kostajh still needs to verify that:
- all functions in this file with $login parameter that need fixes are in the patch.
- the fixes for the functions in the patch are all correct (they all use $login as the timestamp, not the login name).
Comment #11
kostajh CreditAttribution: kostajh commentedI know this is a minor issue but it's also quite simple to review. And the incorrect documentation is frustrating to anyone who is trying to track down issues with the affected functions. Anyone want to take a look?
Comment #12
kostajh CreditAttribution: kostajh commentedRe-rolled. This is a very straightforward fix to the documentation for these functions.
Comment #13
markpavlitski CreditAttribution: markpavlitski commentedThe patch looks good and I can't find any further incorrect references.
Comment #14
jhodgdonThanks for the new patch and the review! I'll get it committed soon.
Comment #15
jhodgdonThanks! I finally committed this to 8.x, sorry for the delay. Time for a port of the patch -- it does not apply to 7.x as it is.
Comment #16
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedHave attached patch for d7.
Comment #17
jhodgdonLooks good, thanks! I'll get that committed soon.
Comment #18
jhodgdonThanks again everyone - committed to 7.x.