Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

It's not so much that the D6 doc was removed (it has different parameters), but that there is no documentation for the params and return value. It would also be helpful if it had a @see linking to the _user_mail_text() function that actually uses this callback, I think?

pillarsdotnet’s picture

Title: Regression: user_mail_tokens() is missing documentation. » Regression: d6 user_mail_tokens() has documented parameters and return value, but d7 and d8 versions do not.
pillarsdotnet’s picture

Status: Active » Needs review
FileSize
1.11 KB

Patch to add docs. Really this should have been required as a follow-up to #554088: Move user_mail_tokens() and emails to token system

aspilicious’s picture

and the $options ?

pillarsdotnet’s picture

Documented the unused parameter.

pillarsdotnet’s picture

Title: Regression: d6 user_mail_tokens() has documented parameters and return value, but d7 and d8 versions do not. » Regression: user_mail_tokens() rewritten by #554088 needs docs.
jhodgdon’s picture

Status: Needs review » Needs work

Thanks! This is pretty good, but has a few problems:
- We don't put the & in the @params area, so that needs to be removed.
- Maybe add a sentence to the top (a new paragraph of description, that is), saying when/why this function is called?
- Please talk about array elements vs. array keys vs. object properteis correctly, in the $data parameter, and it's also probably best to say it's an associative array. I would word it:

An associative array of token replacement values. If the 'user' element exists, it contains a user account object with the following properties:

pillarsdotnet’s picture

Corrected.

pillarsdotnet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Regression, -Needs backport to D7

The last submitted patch, user_mail_tokens-docs-1166388-8.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
Issue tags: +Regression, +Needs backport to D7
jhodgdon’s picture

Status: Needs review » Needs work

Better... Still a couple of problems:

a) An writable ==> a writeable... actually, I would leave out "writeable". It's obvious from the function signature that it's passed by reference. We don't put that in elsewhere in Drupal docs...

b) "values. If " has two spaces after the period. SHould only have one.

c) "it must contain user account object" needs "a" before user account, or use the wording I suggested above.

d) From my earlier review: Maybe add a sentence to the top (a new paragraph of description, that is), saying when/why this function is called? [I guess "maybe" means "please do this, or explain why you think it is unnecessary"]

pillarsdotnet’s picture

a) -- Changed, but why do we deliberately avoid documenting the difference between reference and non-reference parameters? If we have to read the code to understand how to use it, why include a doc header at all?

b) -- Sorry; after years and years of picky English teachers, my fingers automatically hit the spacebar twice after each sentence.

c) -- changed.

d) -- I'm really not sure why this function exists, and at this point I honestly forget why I cared in the first place. Just trying to clean up my issues queue; that's all. The d6 version of the function lacks such a descriptive paragraph, as well, so I can't use it for reference.

jhodgdon’s picture

Well, when I saw "writeable", I just thought it was inconsistent with the other Drupal doc, which I didn't write... And I had to think about what it was talking about. If you want the information in there, you could say "passed by reference I guess?

I know what you mean about the space bar. I learned to type on a typewriter, when that was the standard... but have had to learn not to do it now. :)

Suggested extra paragraph for description:

This function is used by the token_replace() call at the end of _user_mail_text() to set up some additional tokens that can be used in user email messages (generated by user_mail()).

If you put this in, I think you can take out all the @see lines at the end of this function, because all three functions will then be referenced in context.

Thoughts?

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

I think it's enough to specify "variable", which is what the reference requires.

Added paragraph, slightly modified; removed @see lines.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I'm fine with this version, thanks!

7.x/8.x please...

webchick’s picture

Status: Reviewed & tested by the community » Needs review

One question/clarification:
+ * - pass: The account login password.

Is that actually the password? Or the password hash?

pillarsdotnet’s picture

Honestly, all I know from inspection of the code is that it must be non-blank.

jhodgdon’s picture

Status: Needs review » Needs work

I took a look through the code... what a mess.

So. First off, we are not advertising any more (or using, in the default messages) that you can even use user:pass as a token in email messages to the user. See user_admin_settings
http://api.drupal.org/api/drupal/modules--user--user.admin.inc/function/...

 $email_token_help = t('Available variables are: [site:name], [site:url], [user:name], [user:mail], [site:login-url], [site:url-brief], [user:edit-url], [user:one-time-login-url], [user:cancel-url].');

Next, let's look at what happens when a user registers (user_register_submit) as one example of sending these pesky email messages that the admin might have redefined:
http://api.drupal.org/api/drupal/modules--user--user.module/function/use...
Note that comments in [] are added by me; other stuff comes straight from the function:

  // [user_save() changes $account->pass from plain text password to hashed password.]
  $account = user_save($account, $edit);
  ...
   // Add plain text password into user account to generate mail tokens.
  $account->password = $pass;
  ...
  // [this eventually calls drupal_mail(), which calls user_mail(), which calls _user_mail_text()]
  _user_mail_notify('register_no_approval_required', $account);

   ...
  // [from _user_mail_text(): Here, $variables has been set to array('user' => $account)]
   token_replace($text, $variables, array('language' => $language, 'callback' => 'user_mail_tokens', 'sanitize' => FALSE));

So token_replace() takes any object variable in $account and uses it as a possible token. So both [user:password] (plain text) and [user:pass] (hashed) would be supported (if it's called from user_register_submit() anyway), along with anything else that might have been added to the $account object.

BUT, duh, I guess this is all a bit extraneous, because user_mail_tokens() itself does not use anything in $account except what user_pass_reset_url() and user_cancel_url() use. Which is $account['pass'] and $account['login']. And from the above, 'pass' is the hashed password.

Long story short... ['pass'] is hashed.

And it would be nice if user_pass_reset_url() and user_cancel_url() documented their parameters too:
#1196530: User reset URL functions need actual docs

jhodgdon’s picture

Incidentally, since we document $options as "unused parameter...", I think we could/should do the same with $replacements. Either that or document both of them properly, which seems silly.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

I don't understand. Since the $replacements parameter is actually modified by this function, would documenting it as an "Unused parameter" be accurate?

If there is no code in Drupal core that uses the [user:pass] token, should we remove it from this function?

Changed the docs of the $data parameter to reflect that $data['pass'] is the hashed password.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I'm an idiot (regarding $replacements). Ignore that.

No, we shouldn't change the behavior of this function, just the documentation. Removing the ability to use the password/pass tokens would be a completely separate issue.

Patch looks fine, 8.x/7.x please...

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool, thanks for checking into that.

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

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