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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kostajh’s picture

Status: Active » Needs review
FileSize
753 bytes
jhodgdon’s picture

Component: documentation » user.module
Status: Needs review » Needs work
Issue tags: +Needs backport to D7

Wow, 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).

jhodgdon’s picture

In 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.

kostajh’s picture

Status: Needs work » Needs review
FileSize
1.62 KB

Thanks 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.

jhodgdon’s picture

Thanks -- 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...

kostajh’s picture

Ok here's a new patch with UNIX capitalized.

kostajh’s picture

Found another function that had the $login parameter described incorrectly, this patch covers that function as well.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! 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)

+ *   - uid: The user uid number.

==> The user ID number.

b)

+ *   - login: A UNIX timestamp of the user's last login.

==> The UNIX timestamp...
(it's a specific time stamp, so needs "the" not "a")

c)

+ * @param int $timestamp
+ *   A UNIX timestamp.

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?

kostajh’s picture

Status: Needs work » Needs review
FileSize
2.84 KB

Ok, 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.

jhodgdon’s picture

That 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).

kostajh’s picture

Priority: Normal » Minor

I 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?

kostajh’s picture

Priority: Minor » Normal
FileSize
2.79 KB

Re-rolled. This is a very straightforward fix to the documentation for these functions.

markpavlitski’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good and I can't find any further incorrect references.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Thanks for the new patch and the review! I'll get it committed soon.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Component: user.module » documentation
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! 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.

Sivaji_Ganesh_Jojodae’s picture

Issue summary: View changes
Status: Patch (to be ported) » Needs review
FileSize
2.58 KB

Have attached patch for d7.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks! I'll get that committed soon.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again everyone - committed to 7.x.

Status: Fixed » Closed (fixed)

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