As noted in #113614: Add centralized token/placeholder substitution to core, Core's user notification emails and the user_mail_tokens() function should be migrated to the new token replacement system.

Notes for implementation -- user_mail_tokens() actually provides a combination of 'sane' and 'crazy-ass-dangerous' tokens, the latter including things like 'a url that deletes your account' and 'your account's current password'. Those will NOT be exposed as general tokens. Rather, the system email functions will call token_replace() for the sane ones, and supply a callback in the $options array that will insert the two 'dangerous' tokens into the resulting list of replacements.

That will ensure that those two tokens are available *just when the system is sending out its official admin notification emails*, and not at any other time.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

dww’s picture

@eaton: Cool, this will be a nice example of the token callback stuff, then. ;)

eaton’s picture

I've been out of commission for a long weekend due to a move, but my Internet is back up and I (hope?) to take a quick crack at this before DC. Wish me luck.

sun’s picture

Priority: Normal » Critical
Issue tags: +API clean-up

Introducing a new tag for feature freeze: API clean-up.

dww’s picture

Someone just bumped #121267: Support for profile fields in user email notifications with a pseudo patch for D6 -- obviously not going to fly. But, it made me wonder if this issue is actually going to fix that -- are profile fields already exposed to the D7 token API? If not, is there an issue for that, and would it be accepted in the code slush?

sun’s picture

Since converting profile to fields is in the list of code-freeze exceptions, we'd need generic token support for fields -- not sure about the state of that and whether there's already an issue that covers it.

sun’s picture

sorry, but *bump* :)

mr.baileys’s picture

Status: Active » Needs review
FileSize
17.81 KB

First stab at this

  1. I have left user_mail_tokens in place, but it now serves as the callback to supply the unsafe tokens on the call to token_replace.
  2. The existing emails used uri and uri_brief, which were based on $base_url instead of url('<front>', ...), so I added two tokens to system.tokens.inc called uri and uri-brief.
  3. By hooking into the token replacement system, the list of available tokens has just became a lot longer, so we should probably revisit the "Available variables are: "-text. Maybe by auto-generating the list, or by listing to most frequently used ones and then linking to an exhaustive list (possibly related to #514990: Add a UI for browsing tokens?)
  4. There is a bug in the current implementation which caused PHP notices in case language is passed when token_replace is called (see #590014: Tokens: undefined language when language is specified in $options.). I've included that fix in this patch too, so the testbot can take a stab...

edit:typo

sun’s picture

+++ modules/user/user.admin.inc	28 Sep 2009 14:10:21 -0000
@@ -406,7 +406,7 @@
-  $email_token_help = t('Available variables are:') . ' !username, !site, !password, !uri, !uri_brief, !mailto, !date, !login_uri, !edit_uri, !login_url, !cancel_url.';
+  $email_token_help = t('Available variables are:') . ' [user:name], [user:mail], [user:edit-url], [site:name], [site:url], [site:login-url], [user:password], [user:cancel-url], [user:one-time-login-url], [site:url], [site:uri-brief], [site:login-url].';
+++ modules/user/user.module	28 Sep 2009 14:10:23 -0000
@@ -2748,34 +2746,19 @@
-function user_mail_tokens($account, $language) {
...
-    '!login_url' => user_pass_reset_url($account),
...
-    '!login_uri' => url('user', array('absolute' => TRUE, 'language' => $language)),

1) Attention: Duplicate token names in here. The first login-url should be password-reset-url or similar.

2) Tokens should ideally be listed in the previous order here (most used => least used).

+++ modules/user/user.admin.inc	28 Sep 2009 14:10:21 -0000
@@ -765,7 +765,7 @@
-      if ($role && $role->rid != $form_state['values']['rid']) {    
+      if ($role && $role->rid != $form_state['values']['rid']) {
+++ modules/user/user.module	28 Sep 2009 14:10:23 -0000
@@ -2057,7 +2057,7 @@
- * to your theme directory, and edit it as instructed in that file's comments. 
+ * to your theme directory, and edit it as instructed in that file's comments.

Although nice, it would be good to leave out those white-space fixes in here.

+++ modules/user/user.module	28 Sep 2009 14:10:23 -0000
@@ -2114,64 +2114,62 @@
-        return t('An administrator created an account for you at !site', $variables, array('langcode' => $langcode));
+        return token_replace(t('An administrator created an account for you at [site:name]'), $variables, array('language' => $language));

I think it would make sense to

1) Not return, but assign a variable in each case here and just invoke token_replace() once. t() stays per-case though.

2) Uh oh! That said, t() still needs the language argument, because user mails are sent in the language of the user, not in the language of the current request. :)

3) Replace \n in e-mail bodies with normal newlines, like in the cancel* cases.

+++ modules/system/system.tokens.inc	28 Sep 2009 14:10:21 -0000
@@ -181,6 +183,14 @@
+        case 'uri':
+          $replacements[$original] = $base_url;

Shouldn't we call these

site:url

site:url-brief

I don't think we can assume that users know what a "URI" is. Admittedly, they may not even know what a URL is, but at least, I think that's more common.

I'm on crack. Are you, too?

mr.baileys’s picture

1) Attention: Duplicate token names in here. The first login-url should be password-reset-url or similar.
2) Tokens should ideally be listed in the previous order here (most used => least used).

Indreed, some of the tokens where listed twice (or incorrectly) in the "available variables"-text. I do think we need a different solution here (instead of just summarizing some of the tokens), as listing all available tokens is not manageable. I removed the duplicates and reordered the remaining roughly according to frequency used.

Although nice, it would be good to leave out those white-space fixes in here.

Finally figured out how to stop Eclipse from trying to be helpful. No more white-space fixes...

1) Not return, but assign a variable in each case here and just invoke token_replace() once. t() stays per-case though.
3) Replace \n in e-mail bodies with normal newlines, like in the cancel* case

Re-rolled with variable assignment, one token_replace at the end and normal newlines instead of the pre-existing \n's.

2) Uh oh! That said, t() still needs the language argument, because user mails are sent in the language of the user, not in the language of the current request. :)

Yup, thought of that while I was driving home... You beat me to it :)

New patch attached, still contains the fix from #590014: Tokens: undefined language when language is specified in $options. to make it pass the tests...

sun’s picture

Nice!

+++ modules/user/user.module	28 Sep 2009 20:31:43 -0000
@@ -2115,65 +2115,128 @@
-
+    
...
-    return strtr($admin_setting, $variables);
-  }
+    $email_text = $admin_setting;    
+  }  

ok, now you fixed your editor, but now we have plenty of trailing white-space in this patch ;)

This review is powered by Dreditor.

mr.baileys’s picture

Cleaned up my mess (auto-whitespace removal had made me lazy)... I had also forgotten to add the new [site:url-brief] token to system_token_info().

Note that the original emails used uri and uri-brief, which were both based on $base_url. Now the existing system token url is used (pointing to the front page) and url-brief is added to the system tokens, pointing to the front page but without the protocol.

sun’s picture

@eaton: Now would be a wonderful time to speak up and provide mr.baileys a technical review! :)

Status: Needs review » Needs work

The last submitted patch failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
14.69 KB
eaton’s picture

Status: Needs review » Reviewed & tested by the community

I've been out of state this week and hadn't seen the latest iteration of the patch come through, but a big thumbs up to the approach taken. The use of callback for the 'dangerous' tokens is much safer than trying to wrap 'security contexts' into the token system, and simply displaying the 'token' equivalents of the old wildcards sidesteps the currently-unsolved issue of displaying tons of tokens underneath each field. That's still a general issue, and it's a problem in the 'actions configuration' screens as well. There is still some work going on in the token-ux issue, but I think solving the 'use tokens' issue here, then solving the 'cleanly display all the tokens' problem there makes sense.

Note that the original emails used uri and uri-brief, which were both based on $base_url. Now the existing system token url is used (pointing to the front page) and url-brief is added to the system tokens, pointing to the front page but without the protocol.

Agreed - this is a good one to have, as the option was already there and the new token system doesn't punish us for providing more tokens if they're needed.

I'd say this is probably RTBC - do we have unit tests for sending out notification emails, especially in alternate languages? That's the only thing I can imagine might cause issues, though I don't see any problems.

sun’s picture

Awesome!

- Fixed missing breaks in switch statement.

- Fixed indentation of cases in switch statement.

- Fixed PHPDoc and position of user_mail_tokens().

- Fixed missing [user:name] token in description.

On a related note, I wondered why [site:login-url] is not [user:login-url]... doesn't belong into this issue though.

eaton’s picture

On a related note, I wondered why [site:login-url] is not [user:login-url]... doesn't belong into this issue though.

I can see it going either way, but originally the thought was that the "login page" was a sitewide property: it doesn't rely on a particular user to properly build the URL at all, so it isn't technically a 'user' related token.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

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

kle’s picture

The backside of the medal is, that you cannot alter the token result by hook_tokens_alter().
Our [user:one-time-login-url] must point to a different url...
My solution so far is to add a new token [user:one-time-login-url2] with the desired functionality.