Add required docs to user_pass_rehash() function.

Hmmm.

I'm not sure how relevant any of that paragraph is, and I can't really evaluate its correctness... It doesn't tell me how to use the token, and doesn't really give me information that helps me figure that out. Maybe it should say something like:

After creating this token and using it in a URL, you can validate its authenticity by calling such and such function, passing in the same values for the blah blah arguments you used in this function. For an example of generation and usage, see functions xyz() and abc().

That kind of information seems like it might be more useful?

Other small issues with the patch:
- Two spaces after period at least once.
- unix should be capitalized, I believe?

That's much better, thanks! I'm assuming the timestamp value is added to the URL somewhere? Also, I am not sure how to verify your claim that this is always 43 characters -- not seeing that obviously in the code? Other than that, the doc looks great...

"careful examination of the code"... hmmm... It's not clear at all, at least to me, from
http://api.drupal.org/api/drupal/modules--user--user.module/function/use...
that it's fixed length.

Nor the function it calls:
http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal...

Nor the two functions it calls:
http://us.php.net/base64_encode
http://us.php.net/hash_hmac

So is it really a fixed length, and if so, where is this happening? If you're just going on your experimental results from a few trials, let's take that statement out.

Hm. Looking over the patch again, I see that the description refers to $uid, which is not part of this function actually... and I think that descriptive paragraph might be better off without using variable names at all? Also, I noticed you need a comma before "which" in there...

How would this be for the description?

This hash is normally used to build a unique and secure URL that can be validated, so that it can be sent to the user by email for purposes such as resetting their password. In order to validate the URL, the same hash can be generated again (from the same information) and compared to the hash value from the URL. So, the URL normally contains both the time stamp and the the numeric user ID (from which you can look up the login name and hashed password). For a usage example, see user_cancel_url() and user_cancel_confirm().

Again, can you point to where in the code the 43-character limit is done? I couldn't find it, and am unwilling to have that in the doc if it might just be an accident.

There should not be a blank line between the doc header and the function declaration. That will confuse the API module.

One other small fix: the word "which" in English, as used in the @return, either needs a comma before it, or it needs to be "that". Pet grammar peeve of mine... I prefer in this case to do which -> that.

I would like to make you aware of the patch in #154221: Inconsistent Doxygen comments in user module. That patch changes almost *all* the doxygen comments in the user.module to conform to our current commenting standards.

Perhaps you can review and contribute to that patch rather than tackling one function at a time? If nothing else, that issue should get reviewed and committed first before making other more significant changes to the documentation.

(cross-post - I didn't mean to change the status)

That other patch isn't ready to go, and I don't think we should hold up all other potential work on user.module patches just because someone has decided to fix up all the doc. It will probably be a while of back and forth before that patch is ready to go.

In the case of doc-only patches, sun would IMO be mistaken in that case.

I don't advise mega-patches necessarily. They are hard to get approved, because every bit of them has to be right, and that's hard to do. But they shouldn't just be broken up for the sake of breaking them up. IMO. sun may not agree with me on that point.

Let me first address the patch in #13. I think it's pretty good, and I have no problem with it going in as-is, because it's so much better than what's currently there. If it were up to me, I would reword it slightly to improve the grammar. For example, "the user" is singular and "their password" is plural; both should be singular or both should be plural. Not a big deal. I'm not the grammar police, and in fact I don't even consider myself a very good writer, but I do know sub-optimal construction when I see it.

A bit OT, but on the topic of "mega" patches ...

Almost every function in every single module in core needs documentation work to comply with the current documentation standards. Some functions, like user_pass_rehash(), which is the subject of this thread, need more work than others. I fully support patches like #13 above - I think this is work that needs to be done and I appreciate that someone is doing it. My only issue here is with the sequence of patches. I think the bulk of the documentation cleanup can be done with some "mega" patches, then the harder work of re-doing some of the poorly documented functions can be done with patches like #13 above. However, every time a new one-function documentation patch goes in it breaks the "mega" patch, which then has to be rebased. It makes far more sense to me to clean up the little things first, like the verb tense, line wrappings, spacing between @param and @return, etc. etc., and THEN go on to improving the quality of the function descriptions.

Because almost every module and function in core needs this "little" work, that's already about 50 "mega" patches that need to be created. If I can't get just one in, it's unlikely I'll do the other 49. It was a lot of work to modify about 25% of the lines in the user.module to bring them up to standard; I'm willing to see that through to the end, but I'd rather I didn't have to keep re-rolling the patch all the time as one function after the other is addressed separately and breaks my "mega" patch.

If we *don't* do "mega" patches, but try to address each function individually, then the work will never be finished. That's not rhetoric, that's reality - there are far too many functions to deal with one at a time. I'm perfectly willing to address any specific concerns @sun may have with my patch, but "it's too big" is not a valid concern and won't carry any weight with me.

TR: you are not the first one to try this mega-patch approach. See http://drupal.org/node/855410#comment-3208874 for one example... there were a number of others at the same time.

The main problem is that you HAVE to get everything right in the mega-patch. The one you're proposing isn't there yet.

Also, you're complaining that you have to redo your mega-patch if other patches go in. But if your mega-patch gets in, many many other patches have to be redone, which is also a lot of work for a lot of other people.

So my feeling is to just leave the docs out of compliance in general, and fix up individual docs when there are other more pressing issues with them.

If there's a grammar problem in the current patch, by all means let's fix it.

Let's go with this (d7/8) -- looks good, thanks for all the iterations!

Committed to 8.x and 7.x. Thanks!

And yeah, I've been holding off on the "mega" patch because it's one of those things that's going to break a whole bunch of other patches in the queue, and it takes a really long time to review. Smaller, more focused patches are definitely safer, as a general rule. But it it's changing the exact same thing 500 times, then it makes more sense as a "mega" patch. It's a bit of a fuzzy line. But basically, try and put yourself in the shoes of someone uninvolved with the issue, and how many times they'd have to switch contexts to review the changes.