Token integration is broken

I am not a fan of putting hook implementations in separate include files, especially for core hooks (unless the module automatically looks for those include files, like Views) because it's one more thing you have to know to understand how the module works. Looking at any random module you could reasonably expect to find the hook_token_info() implementation in the .module file. The only way to know how hook_token_info() is discovered, if it is placed in another file, is to look at statuses_init(). Everywhere else where Statuses has split functionality into include files, those files are explicitly included where the files need to be used, so the flow is easy to understand.

One pattern I have used in other projects is to implement hooks like this:

// MYMODULE.module
function MYMODULE_hook_implementation() {
  module_load_include('inc', 'MYMODULE', 'includes/MYMODULE.purpose');
  return _MYMODULE_hook_implementation();
}

//includes/MYMODULE.purpose.inc
function _MYMODULE_hook_implementation() {
  // hook implementation
}

That way the "real" hook implementation is in the location you expect to find it, while the "actual" implementation is in a separate file. A good rule of thumb to determine whether it's worth doing this is whether the code you want to move into a separate file is over 50 lines long.

+++ b/statuses.module
@@ -382,6 +382,9 @@ function statuses_init() {
+  include_once $path .'/includes/statuses.tokens.inc';

Use module_load_include() instead of include_once().

+++ b/submodules/fbss_comments/fbss_comments.module
@@ -218,7 +218,7 @@ function theme_fbss_comments_items_email($variables) {
-    $output .= '<div>'. theme('username', $author) .' ';
+    $output .= '<div>'. theme('username', array('account' => $author)) .' ';

This is an unrelated change -- let's address this in a separate issue, and while we're at it, add a space between the strings and concatenation operators.

+++ b/includes/statuses.tokens.inc
@@ -0,0 +1,248 @@
+  foreach ($tokens as $name => $original) {
+    switch ($name) {
+      case 'sender-themed':

I find this logic kind of hard to follow. It would be easier to build an array of replacement values like in D6, then build the array to return by iterating over $tokens and doing something like $replacements[$original] = $values[$name];

+++ b/includes/statuses.tokens.inc
@@ -0,0 +1,248 @@
+/**
+ * Implements of hook_token_info().
+ */
+function fbss_comments_token_info() {

The comments submodule has its own Token integration. These tokens were part of the core module's Token integration because they are more relevant to statuses than status comments. They should not be implemented separately, and in fact this would break the token integration of the comments submodule.

I'm really busy this week and next week so I probably won't be able to be on IRC, though I'll have more time the following week. The patches in #5 look fine to me though.

Note: You should no longer have -raw tokens in D7 as you should be providing different token values back if $options['sanitize'] is TRUE or FALSE. Please look at the core token integrations on how they are doing that.

+++ b/statuses.tokens.incundefined
@@ -0,0 +1,222 @@
+  $tokens['statuses']['sender-themed'] = array(
+    'name' => t('Themed name of the sender'),
+    'description' => t('The themed name of the user who posted the status message.'),
+  );
+  $tokens['statuses']['sender-name'] = array(
+    'name' => t('Sender safe name'),
+    'description' => t('The safe name of the user who posted the status message.'),
+  );
+  $tokens['statuses']['sender-name-raw'] = array(
+    'name' => t('Sender raw name'),
+    'description' => t('The raw name of the user who posted the status message. WARNING: raw user input.'),
+  );
+  $tokens['statuses']['sender-uid'] = array(
+    'name' => t('Sender user ID'),
+    'description' => t('The User ID of the user who posted the status message.'),
+  );
+  $tokens['statuses']['sender-picture'] = array(
+    'name' => t('Sender Picture'),
+    'description' => t('The profile picture of the user who posted the status message.'),
+  );

This screams to me that it should be just one $tokens['statuses']['sender'] token of type 'user' so that you can use tokens like [statuses:sender:uid] instead of having to write your own tokens for all the user properties.

(Thanks Dave.)

---

See https://drupal.org/node/28984

Basically check_plain() turns HTML markup into plain text by turning e.g. < and > into their equivalent HTML entities. filter_xss() removes malicious HTML markup but leaves the rest. Statuses generally shouldn't use either one directly -- usually status messages pass through _statuses_run_filter() which typically calls check_markup() (which runs input filters over the text).

If you haven't already, you should go to https://drupal.org/user/1315174/edit/newsletter and subscribe to security announcements. If you have other questions about security there is a whole handbook in the documentation on it at https://drupal.org/writing-secure-code

+++ b/statuses.tokens.inc
@@ -0,0 +1,188 @@
+    'name' => t('Recipient type(Machine name)'),

Needs a space between "type" and "(Machine name)"

+++ b/statuses.tokens.inc
@@ -0,0 +1,188 @@
+    'name' => t('Recipient safe name'),
+    'description' => t('The safe name of the recipient of the status message.'),

Remove the word "safe" since Token should now automatically manage the difference between safe and raw tokens

+++ b/statuses.tokens.inc
@@ -0,0 +1,188 @@
+  $message_formatted = _statuses_run_filter($status->message);
+  if (variable_get('statuses_nl2br', 0)) {
+    $message_formatted = nl2br($message_formatted);
+  }
+  $edit = '';
+  $delete = '';
+  if (statuses_user_access('edit', $status)) {
+    $edit = '<span class="statuses-edit-link statuses-action-link">' . l(t('Edit'), 'statuses/' . $status->sid . '/edit') . '</span>';
+  }
+  if (statuses_user_access('delete', $status)) {
+    $delete = '<span class="statuses-delete-link statuses-action-link">' .  l(t('Delete'), 'statuses/' . $status->sid . '/delete') . '</span>';
+  }
+  $sender = user_load($status->sender);
+  $recipient = $context['handler']->load_recipient($status->recipient);
+  $url = url('statuses/'. $status->sid, array('absolute' => TRUE));

Most if not all of these variables are only used for one token, so this logic can be moved into the individual cases in the switch statement below.

+++ b/statuses.tokens.inc
@@ -0,0 +1,188 @@
+        $replacements[$original] = $sanitize ? filter_xss($context['handler']->recipient_name($recipient)) : $context['handler']->recipient_name($recipient);

Node titles, usernames, taxonomy terms, etc. should be plain text, so use check_plain() instead of filter_xss().

+++ b/statuses.tokens.inc
@@ -0,0 +1,188 @@
+        $replacements[$original] = $sanitize ? filter_xss($sender->name) : $sender->name;

Again, usernames should be plain text, so use check_plain(). Also, in the case when we're not sanitizing the username, my first thought would be to use theme('username') -- but we should be consistent with what other modules provide as the default.

+++ b/submodules/fbss_comments/fbss_comments.tokens.inc
@@ -0,0 +1,138 @@
+  $edit = '';
+  $delete = '';
+  if (fbss_comments_can('edit', $comment)) {
+    $edit = '<span class="fbss-comments-edit-delete">' .
+      l(t('Edit'), 'statuses/comment/' . $comment->cid . '/edit', array('query' => array('destination' => $_GET['q'])))
+      . '</span>';
+  }
+  if (fbss_comments_can('delete', $comment)) {
+    $delete = '<span class="fbss-comments-edit-delete">' .
+      l(t('Delete'), 'statuses/comment/' . $comment->cid . '/delete', array('query' => array('destination' => $_GET['q'])))
+      . '</span>';
+  }

As above, this logic can be moved into the switch statement

+++ b/submodules/fbss_comments/fbss_comments.tokens.inc
@@ -0,0 +1,138 @@
+        $replacements[$original] = $sanitize ? filter_xss($account->name) : $account->name;

As above, check_plain() instead of filter_xss()

If you're outputting a user name, it should actually use format_username($account) with a check_plain() on the result if $options['sanitize'] is TRUE.

Hmm. There are probably at least a few other places we should be using format_username() as well (the "user" context's recipient_name() method, the status template preprocessing...) -- let's address that as a minor follow-up to this issue. For future reference, format_username() will likely be deprecated in D8 in favor of entity_label().

Tested and it works. The "Recipient safe name" thing wasn't changed (the word "safe" should be removed) but other than that it should be ready to commit. Let's not forget to follow up with format_username() changes in a separate issue.

Sorry to reopen a closed post, just want to confirm whether the above committed changes were actually implemented into the 7.x-1.0-beta1 or dev release? As I have tried both versions and am still having problems getting many of the supplied tokens to work in Heartbeat module. For example:

When using the tokens [statuses:sender:uid] and [statuses:recipient-id] respectively, instead of getting the desired usernames the result is the string "name" (not sure where this is coming from as I have 3 users, none of which are called "name") and the linked title of the node. If I try using many of the extended Statuses tokens not listed in Heartbeat Rules (eg. [statuses:sender:name], [statuses:sender:link]), they do not output correctly. I even tried applying the patches above first to the beta and then to the dev version, but this did not fix anything either.

Also when comparing the tokens available in Heartbeat to those available in PathAuto, the list is a lot smaller. Why is this?
I've attached screen grabs of Statuses Tokens in Heartbeat compared to Statuses Tokens in PathAuto. Would appreciate an update on this.

Heartbeat is a pretty complicated module and may be doing things differently. Should be addressed in a new issue, probably in the heartbeat queue to start with.

There is actually already an issue in Heartbeat about Statuses integration for D7, it is marked as postponed still. Stalski (maintainer of Heartbeat module) discusses some of the reasons why Statuses is not yet ready for integration towards the bottom of the page. Perhaps you could shed some light in the queue regarding this integration as quite a few people are asking about it. Would be appreciated.