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.
Comment | File | Size | Author |
---|---|---|---|
#17 | drupal.user-mail-tokens.patch | 14.43 KB | sun |
#15 | 554088-tokens-for-users-4.patch | 14.69 KB | mr.baileys |
#12 | 554088-tokens-for-users-3.patch | 16.91 KB | mr.baileys |
#10 | 554088-tokens-for-users-2.patch | 17.13 KB | mr.baileys |
#8 | 554088-tokens-for-users.patch | 17.81 KB | mr.baileys |
Comments
Comment #1
sunsubscribing
Comment #2
dww@eaton: Cool, this will be a nice example of the token callback stuff, then. ;)
Comment #3
eaton CreditAttribution: eaton commentedI'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.
Comment #4
sunIntroducing a new tag for feature freeze: API clean-up.
Comment #5
dwwSomeone 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?
Comment #6
sunSince 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.
Comment #7
sunsorry, but *bump* :)
Comment #8
mr.baileysFirst stab at this
user_mail_tokens
in place, but it now serves as the callback to supply the unsafe tokens on the call totoken_replace
.$base_url
instead ofurl('<front>', ...)
, so I added two tokens tosystem.tokens.inc
calleduri
anduri-brief
.edit:typo
Comment #9
sun1) 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).
Although nice, it would be good to leave out those white-space fixes in here.
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.
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?
Comment #10
mr.baileysIndreed, 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.
Finally figured out how to stop Eclipse from trying to be helpful. No more white-space fixes...
Re-rolled with variable assignment, one token_replace at the end and normal newlines instead of the pre-existing \n's.
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...
Comment #11
sunNice!
ok, now you fixed your editor, but now we have plenty of trailing white-space in this patch ;)
This review is powered by Dreditor.
Comment #12
mr.baileysCleaned 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
anduri-brief
, which were both based on$base_url
. Now the existing system tokenurl
is used (pointing to the front page) andurl-brief
is added to the system tokens, pointing to the front page but without the protocol.Comment #13
sun@eaton: Now would be a wonderful time to speak up and provide mr.baileys a technical review! :)
Comment #15
mr.baileysRerolled after #590014: Tokens: undefined language when language is specified in $options. landed.
Comment #16
eaton CreditAttribution: eaton commentedI'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.
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.
Comment #17
sunAwesome!
- 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.
Comment #18
eaton CreditAttribution: eaton commentedI 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.
Comment #19
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #21
kle CreditAttribution: kle commentedThe 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.