bp_activity_at_name_filter_updates() causes activity to be saved twice

[r-a-y]

Because bp_activity_at_name_filter_updates() resaves the activity a second time:
​http://buddypress.trac.wordpress.org/browser/tags/1.5.3.1/bp-activity/bp-activity-filters.php#L184
​http://buddypress.trac.wordpress.org/browser/tags/1.5.3.1/bp-activity/bp-activity-filters.php#L192

This can cause complications for plugins relying on the "bp_activity_after_save" as their functions will run twice.

Case in point, the Group Email subscription plugin relies on the "bp_activity_after_save" hook. So when an @mention is used, the Group Email plugin will send another copy of the email as well.

Not sure what the best solution for this is.

[DJPaul]

What about switching it to hook to 'bp_activity_before_save', and removing the save() call, with a lower priority?

[boonebgorges]

r-a-y, you've now made some modifications to the Group Email plugin that works around this BP issue. Having spent some time with workarounds, do you have ideas about a straightforward core fix? I'm pretty sure that the reason I used bp_activity_after_save is because the activity_id is not available before the save() method is run, and we need that activity id to save the mention to usermeta. See #2082, r4367

Along the lines of what DJPaul suggests, maybe split the process of identifying/linkifying mentions from the process of storing them to usermeta:

• hook into bp_activity_before_save
• look for mentions
• If none are found, do nothing and return to the save() function
• If mentions are found:
  • do the necessary link-ifying to the content
  • unhook the current function from bp_activity_before_save (to prevent infinite loops)
  • hook a second function, for recording the mention(s) to usermeta, to bp_activity_after_save
  • and run the save() function again

This workflow is pretty clunky (I am not a fan of the hook-then-unhook-to-prevent-recursion trick), but it seems like it would work with relatively little overhead.

[r-a-y]

That sounds like it could work!

[boonebgorges]

Awesome, thanks r-a-y. I can try to whip up a patch for this next week, but I'd be more than happy if you got to it first (on CUNY time :) )

[boonebgorges]

Last call for BP 1.6. I want to clear out the milestone, and this is puntable (as it's not a regression).

[r-a-y]

Returning to this issue again, I can't see a decent approach to get around the duplicate action firing.

​The workflow described by Boone will work if no @mention is found, which is better than what we currently have.

However, if we find an @mention, the activity content will need to be saved for a second time. This means we'll be firing any activity save hooks twice.

One solution is we could add a parameter to BP_Activity_Activity::save() so we could bypass any do_action() calls there. Yes, this would be rather ugly.

Any other ideas?

[boonebgorges]

What about DJPaul's original suggestion of moving bp_activity_at_name_filter_updates() to bp_activity_before_save? We wouldn't have access to $activity->id at this point, but do we need it? In bp_activity_at_name_filter(), the $activity_id seems to be used for two things: to check whether the activity is spam, and to increment mention counts. Checking for spam is easy enough; the BP_Activity_Activity object is passed to bp_activity_before_save, and so we should have access to the is_spam property.

Mention count is a little trickier, because the way we currently do mention count incrementing (in bp_activity_adjust_mention_count() is to call up the activity by id and look for mentions. But this workflow could pretty easily be changed so that all we need is the content of the activity item. In fact, the current setup requires doing the bp_activity_find_mentions() logic twice: once in bp_activity_at_name_filter() and again in the adjust_mention_count() function. We could abstract the increment-for-member functionality into its own standalone function, and then call that inside the foreach $usernames loop in bp_activity_at_name_filter(). Not only would this make it so that the activity id is not required, but it would be more efficient.

So it looks like it should be possible to move all this logic to before_save. Am I missing anything?

[r-a-y]

That would almost work except the @mention email notification needs the activity ID to build the permalink in the email content.

bp_activity_at_message_notification( $activity_id, $user_id );

Update: Have an idea... working up a patch right now! Thanks for the inspiration, Boone!

[boonebgorges]

Good call, r-a-y! Not sure what your idea is, but how about this:

• on bp_activity_before_save: look for mentions; do the preg_replace() on $activity->content to turn into links; and then, for each found username:

$bp->activity->pending_mention_notifications[] = $username;

• Then, on bp_activity_after_save,

foreach ( $bp->activity->pending_mention_notifications as $username ) {
    // do the notification for $username
}
unset( $bp->activity->pending_mention_notifications );

This seems sorta ugly on the surface, but on further reflection, it kinda makes sense: we have to parse and modify the content *before* save, and we should only be sending notifications *after* an item has been successfully created. The (very) temporary entry in the $bp global acts like a very short-term cache of the notifications that have to be sent, which is perhaps not the peak of elegance, but it's not too bad and it would work :)

[r-a-y]

Boone, your idea was basically the same as mine!

Here's a rundown of the changes in the patches:

• New: bp_activity_at_name_send_emails() - This is fired if @mentions are detected in bp_activity_at_name_filter_updates().

• Altered: bp_activity_at_name_filter_updates() - Removes the email firing logic into bp_activity_at_name_send_emails(). Linkifies @mentions before saving the activity item.

• Altered: bp_activity_at_name_filter() - Changed so the function only linkifies @mentions in content that is not an activity item.

• Altered: bp_activity_find_mentions() - Changed so it returns an associative array with the user ID as key and username as value. This logic was being duplicated a few times in other functions, so I moved it here.

• Altered: bp_activity_adjust_mention_count() - Moved mention updating into bp_activity_update_mention_count_for_user().

• New: bp_activity_update_mention_count_for_user() - Extrapolated from bp_activity_adjust_mention_count().

I've done two patches - one without global stuffing and one with. I personally don't mind saving the temporary variable, but others might not like it, which is why there are two patches!

Feedback welcome.

[boonebgorges]

Thanks for patching it, r-a-y!

I think this approach looks great. I'm in favor of with-stuffing, not only because I'm still in the Thanksgiving spirit, but also because it's far more elegant, whatever you think about global variables, to run the find_mentions() function just once.

Just so I'm clear: the returned value of bp_activity_find_mentions() is changed only in that the array keys are now user ids, right? I want to make sure that there are no backward compatibility problems with anyone using bp_activity_find_mentions() in a plugin. If it's just a matter of different indexes, I doubt there will be any problems.

[r-a-y]

I'm also in favor of stuffing in general and with the patch!

As for bp_activity_find_mentions(), you're right. The only change is in the indexes. Instead of the incremental numerical key, it's now the user ID.

Let me know if I'm missing anything before I commit the patch.

[boonebgorges]

Looks lovely to me. Thanks, r-a-y.

[r-a-y]

(In [6649]) Only fire the activity save hooks once. Fixes #3980.

Previously, bp_activity_at_name_filter_updates() would run the
BP_Activity_Activity::save() method again. So basically, all activity save
hooks would run twice; this caused problems for plugins that hooked into
any activity save hook as their code would run twice.

This commit changes the logic of how @mention activity items are linked and
sent so the activity save hooks are only run once.

bp_activity_at_name_filter_updates() now runs before an activity item is
saved and detects if @mentions are found. If @mentions exists, we add a
hook to send the emails after the activity item is saved -
bp_activity_at_name_send_emails().

Hat-tip boonebgorges for feedback.

For a full list of changes, view:
​https://buddypress.trac.wordpress.org/ticket/3980#comment:11