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.

Files: 
CommentFileSizeAuthor
#16 user-pass-rehash-docs-1665446-16.patch2.58 KBsivaji
PASSED: [[SimpleTest]]: [MySQL] 40,437 pass(es).
[ View ]
#12 user-pass-rehash-docs-1665446-12.patch2.79 KBkostajh
PASSED: [[SimpleTest]]: [MySQL] 55,620 pass(es).
[ View ]
#9 user-pass-rehash-docs-1665446-9.patch2.84 KBkostajh
PASSED: [[SimpleTest]]: [MySQL] 40,339 pass(es).
[ View ]
#7 user-pass-rehash-docs-1665446-7.patch2.68 KBkostajh
PASSED: [[SimpleTest]]: [MySQL] 40,739 pass(es).
[ View ]
#6 user-pass-rehash-docs-1665446-6.patch1.85 KBkostajh
PASSED: [[SimpleTest]]: [MySQL] 40,739 pass(es).
[ View ]
#4 user-pass-rehash-docs-1665446-4.patch1.62 KBkostajh
PASSED: [[SimpleTest]]: [MySQL] 37,039 pass(es).
[ View ]
#1 user-pass-rehash-docs-1665446-1.patch753 byteskostajh
PASSED: [[SimpleTest]]: [MySQL] 37,006 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new753 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,006 pass(es).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new1.62 KB
PASSED: [[SimpleTest]]: [MySQL] 37,039 pass(es).
[ View ]

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.

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

StatusFileSize
new1.85 KB
PASSED: [[SimpleTest]]: [MySQL] 40,739 pass(es).
[ View ]

Ok here's a new patch with UNIX capitalized.

StatusFileSize
new2.68 KB
PASSED: [[SimpleTest]]: [MySQL] 40,739 pass(es).
[ View ]

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

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?

Status:Needs work» Needs review
StatusFileSize
new2.84 KB
PASSED: [[SimpleTest]]: [MySQL] 40,339 pass(es).
[ View ]

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.

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

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?

Priority:Minor» Normal
StatusFileSize
new2.79 KB
PASSED: [[SimpleTest]]: [MySQL] 55,620 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

Assigned:Unassigned» jhodgdon

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

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.

Issue summary:View changes
Status:Patch (to be ported)» Needs review
StatusFileSize
new2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 40,437 pass(es).
[ View ]

Have attached patch for d7.

Status:Needs review» Reviewed & tested by the community

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

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.