Help plugins to generate activities for their post type(s)

[imath]

In dev-chats, we've discussed about the opportunity to split #3460 starting with a first step to help plugins in managing activities in case their post type(s) is (are) published / updated / trashed / deleted.

This ticket is not covering eventual comments made on the post type(s) as it could be a second step.

I think we need to deal with two kinds of plugin :

• BuddyPress plugins that are registering a post type in their custom BP_Component
• Any other plugins.

So first, i'm suggesting to create a new Activity class BP_Activity_Tracking to listen to post types so that the BuddyPress blogs component and custom components will be able to use it to generate their activities.

You can have a look at the BuddyPress Skeleton Component (branch 1.7) patch (bsc.1.7.patch), i've used to test it, i've added some comments in the BP_Example_Component->register_post_types() method that explains how to use it.

Then i thought for the other plugins, that as we were doing so in #3460, their post types should be managed if the blogs component is active. So if it's the case, a plugin could simply use a filter to attach its post type(s) to the one managed by the blogs component (post).

You can have a look at the function bp_custom_set_activity_tracking() in the bp-custom.php file for an example of use.

[DJPaul]

There are a couple of minor stylistic tweaks I'd want to see adjusted, and some improvements to output escaping, but I have a bigger question about the ideas here, plus a bunch of random thoughts:

• I think the proposed changes to BP_Component->register_post_types are redundant and over-complicate things, and don't really help with the core aim of the change.
• What I think we should do is simply allow post types to register themselves into the activity stream, e.g.:

add_post_type_support( 'post', 'buddypress-activity' )

and we keep the existing hooks to transition_post_status and add checks with post_type_supports. I think there are enough labels already set in WP's register_post_type that we can re-use for BuddyPress; if people want to change whatever we construct, we can add a filter.

• We can then just fetch all public post types, and check if that support is set, and then do the bp_activity_set_action() stuff.
• I like the idea to move all the existing tracking code out of the Blogs component and into Activity. That'd help us be in a position where we can discuss if we want to keep the Site Tracking component in the future, but I digress.
• Why is this built in a class? Just fake namespacing? I think we can probably just use functions if/when this patch is trimmed down.

[DJPaul]

I think this is going to need time for more discussion (and then to update the patch) than we have before 2.1 beta, so moving to the 2.2 milestone.

[imath]

Replying to DJPaul:

add_post_type_support( 'post', 'buddypress-activity' )

Thanks a lot for your great feedback. You're completely right. I'll work on a new patch following your advices.

[imath]

​5669.02.patch is trying to follow DJPaul's recommandations. Once applyed you can add any post types to activity tracking using add_post_type_support(), for instance for the Page post type :

add_post_type_support( 'page', 'buddypress-activity' );

As explained in the main description of the ticket. It's a first step so i've only took care of published post types. If it's ok, i'll look at comments about post types.

[boonebgorges]

imath - I like the direction this is going - add_post_type_support() is a great way to do this.

Small concern: why collect this data on 'bp_init' with priority 7? I'm guessing that plugins will register post support when registering post types, which usually happens at 'init:10' or later. Collecting info at bp_init:7 (which is init:10) is going to create race conditions. Any reason we can't do it later, like 'bp_ready' or something like that?

Is it possible to stop using bp_blogs_record_post()? In other words, we would add_post_type_support( 'post', 'buddypress-activity' ). It'd be nice to centralize our own code.

[imath]

Thanks for your feedback boonebgorges.

I thought maybe we need to have the info of post types to track before 'bp_register_activity_actions' which hooks 'bp_init' with priority 8. I agree with you i'd like this to be at a later priority or on another hook, but in the patches i've worked on #3460 i think it wasn't ok. So i'll double check to see if it's working.

You are right: it would be great to stop using bp_blogs_record_post() and centralize our own code. I was a bit afraid to change too much things in the Blogs component, but my initial idea was to check if blog has a 'blog_public' option set to 1 and if so force the 'post' post type to have 'buddypress-activity' support.

I'll also look at this point and work on another patch.

[boonebgorges]

I was a bit afraid to change too much things in the Blogs component

needs-unit-tests ;)

[imath]

:) Absolutely, i'll include them on the next patch.

[imath]

In ​5669.03.patch i've edited the way the tracking data was collected as suggested boonebgorges. To avoid "race conditions" i've created a new action bp_fully_loaded which hooks to wp_loaded. Unfortunately i cannot rely on bp_ready as it's too late for the activity administration screen (dropdown list of activity actions).

wp_loaded exists since WP 3.0 and is fired once WordPress, plugins and themes are fully loaded.

I'm now using add_post_type_support( 'post', 'buddypress-activity' ) also for the Blogs component. It means i've deprecated some functions and the Site Tracking feature for posts mainly rely on the new activity functions. I took many precautions to keep on using filters and actions that was used in the deprecated functions so that it should not have an impact on people using them. I haven't change anything about comments tracking. I think it could happen in a future step.

As i'm no more hooking bp_init before bp_register_activity_actions, i've also needed to introduce a new parameter to bp_activity_set_action(): 'position'. By default it's set to 0. It helps me to be sure the "Posts" activity action will be listed before the "Comments" one. To do so, i've introduced a new function that sort the actions for a given component bp_activity_sort_component_actions()

Finally i've added some unit tests and found a bug in the way the Blogs component is dealing with password protected posts. If you publish a post as public, an activity is generated. If you update this post by setting a password the activity is updated. I think it should be deleted.

Ran all the tests in regular and multisite mode and everything is ok. "Generating" post types activities is really expected by users, it would really be great if we could do this first step for the 2.2 milestone.

[boonebgorges]

Thanks, imath! I think this is looking pretty good. A couple smallish suggestions, and one larger one surrounding i18n.

• test_bp_activity_catch_transition_post_type_status_publish_to_publish() doesn't really test that no new activity was posted. Maybe after the initial post, delete the activity item, then assertFalse() for the second assertion?
• You will probably want to clear out (or reset) the buddypress()->activity->track array between tests. You do in some cases, but not in others.
• A possible alternative to your new 'bp_fully_loaded' hook is to have a wrapper function like bp_activity_get_trackable_types(), and then to use that function whenever you need to get information about these types. Internally, that function could dynamically assemble the required data (like currently happens in bp_activity_tracking_register_post_types()), and could store it in the buddypress() global if you wanted to, but in any case by the time it's called, all post types should have had a chance to register themselves. This kind of lazy-loading seems more elegant to me than introducing yet another bp_ subhook (especially since it'll save the overhead of setting up the data if it's unneeded), but I'm glad to leave the decision up to you.
• You'll probably want a hook at the end of bp_activity_tracking_register_post_types() (or a filter at the end of bp_activity_get_trackable_types(), if you decide to adopt the suggestion). It's likely that plugin authors will want to modify your default values for stuff like 'singular' and 'activity_comment', and there should be a centralized way for them to do it.
• I don't feel very confident about the localizability of bp_activity_format_activity_action_custom_post_type_post(). Using sprintf() like this works in English and French, but I'm not confident it will work in other languages. What do you think about using the sprintf() as a fallback, with a different "preferred" method of storing a translatable full sentence in, say, $post_type['labels']['bp_activity_new_post'] = __( '%s posted a new foo' ) or something like that? Obviously there would be a lot of details to work out, but I think we need to come up with a recommended way to register full sentences for the activity action formatter.

[imath]

Thanks for you feedback boonebgorges. The i18n is giving me headaches !! I'll explain why after.

First "new hook vs lazy-loading". Although I agree that the your lazy loading suggestion is very interesting, if you and the team don't mind, i think i'd rather use the hook as, in the end, i find it less risky.
I can see how to use a function like bp_activity_get_trackable_post_type( "post_type_to_check" ) when doing the transition post status stuff, as we actually need to get the tracking info.
But i'm less sure about the other usage of this function that is to populate another global buddypress()->activity->actions so that in dropdowns it's possible to filter the activity stream by post types. I mean,

1. for instance in bp_activity_show_filters(), it seems strange to me to call a function which would return nothing used directly.
2. imho it would introduce some risks themes wouldn't get all the actions if they don't use the specific function before trying to get the activity types using the global buddypress()->activity->actions or the function bp_activity_get_types()

So have a preference for the hook and if it's ok with everybody, i'll add a hook at the end of the function bp_activity_tracking_register_post_types()

I'll correct the two points you mentioned about the unit tests.

Translation is giving me headaches!!

But before all this, i think we need to find the best way to deal with translations. I'll first explain what i do in the patch. Unlike the 'new_blog_post' action string, i chose not to fetch the title of the post to avoid a blogmeta. So the string looks like this :

__( [user_link] posted a new [post type link], on the site [blog_link] )

So as you can see there are 3 links. First and last are the same than in the 'new_blog_post' action but [post type link] is like this :
<a href="linktothepost">$post_type->labels->singular_name</a>

So having a $post_type['labels']['bp_activity_new_post'] = __( '%s posted a new foo' ) is not very helpful in this case because foo is not a link. It could be $post_type['labels']['bp_activity_new_post'] = __( '%s posted a new %s' ), but it might confuse the plugin author :(

I think we'll never find the perfect solution. What we're trying to do in the first place is to find a "generic" action. So it is necessarily problematic.

Let's take an english example.

imath posted a page > good
imath posted a post > good
imath posted a idea > not good

So the word "new" is an interesting way to solve this issue in english.

Let's look at french with the word new :
imath a posté un nouvel article > good
imath a posté un nouvel page > not good, it should be: imath a posté une nouvelle page
imath a posté un nouvel livre > not good it should be : imath a posté un nouveau livre

A way to solve this in french would be to get rid of the new word and use something like un(e), e.g.:
imath a posté un(e) livre

So IMHO, i don't think sprintf is problematic as on the contrary it avoids wrong order comparing to concatenated strings.
see ​http://codex.wordpress.org/I18n_for_WordPress_Developers#Best_Practices

Finally, using the post type labels argument might not be the best way in my opinion.

1. I think lots of people will use add_post_type_support( 'book', 'buddypress-activity' ) without being the plugin author, because it's easy.
2. As a plugin author i think i would use the filter i've used in the Blogs loader class to define my own formatting function because anyway i'd need to set the component_id parameter to my component (by default it's the activity component)

Argh. I'm disappointed and i lack ideas to solve this issue :(

[boonebgorges]

for instance in bp_activity_show_filters(), it seems strange to me to call a function which would return nothing used directly.

It wouldn't return nothing, it would return the types. It's only that those types would be generated on the fly, the first time that the function is called on a given page load.

imho it would introduce some risks themes wouldn't get all the actions if they don't use the specific function before trying to get the activity types using the global buddypress()->activity->actions or the function bp_activity_get_types()

This is true of your setup too - everything has to run by bp_fully_loaded. My proposal would actually be *more* lenient, since the types wouldn't be generated until needed - which will generally be during 'wp' or later.

But I don't feel strongly about any of this.

Translation is giving me headaches!!

Yeah, but we have to get this right, or we can't do it :)

I think you're overthinking this. We can't predict what every language will do, which is why we have to provide full sentences. And while generally we try to keep markup out of translatable strings, it's not a hard and fast rule for cases like this. So the proposed 'labels' string will look like this:

'bp_activity_new_post' => __( '<a href="%1$s">%2$s</a> posted a new <a href="%3$s">foo</a>' )

Finally, using the post type labels argument might not be the best way in my opinion.

I take your points. I propose that we support use of the 'labels' array, so that people registering support with register_post_type() will be able to use it. But we can also provide a wrapper like bp_activity_register_tracking_action_format() (maybe you can think of a better name?) which would allow people to register this label for an existing post type. Or maybe it could be a more general function for updating any aspect of the 'trackable' types.

[imath]

Thanks a lot for your advices :) You're right i'm overthinking the translation thing.

In ​5669.04.pacth

• I think each tests are now resetting the tracking global.
• I've edited test_bp_activity_catch_transition_post_type_status_publish_to_publish() the way you suggested
• i've added an action at the end of bp_activity_tracking_register_post_types()

It's now possible to define custom action strings when registering the post type, for instance :

$labels = array(
	'name'                    => 'bars',
	'singular_name'           => 'bar',
        'bp_activity_new_post'    => __( '%1$s shared a new <a href="%2$s">bar</a>', 'bar-domain' ),
	'bp_activity_new_post_ms' => __( '%1$s shared a new <a href="%2$s">bar</a>, on the site %3$s', 'bar_domain' ),
);

register_post_type( 'bars', array(
	'labels'   => $labels,
	'public'   => true,
	'supports' => array( 'buddypress-activity' ),
) );

It's also possible to change any tracking params of the tracked post type (as soon as it's after 'bp_fully_loaded' )

function page_supports_tracking() {
	add_post_type_support( 'page', 'buddypress-activity' );
}
add_action( 'bp_init', 'page_supports_tracking' );

function page_tracking_args() {
	bp_activity_set_post_type_tracking_args( 'page', array(
		'component_id' => buddypress()->blogs->id,
	) );
}
add_action( 'bp_ready', 'page_tracking_args' );

Finally, I've added tests for the custom strings and the bp_activity_set_post_type_tracking_args() function.

[DJPaul]

I am not a huge fan of the name of the new core action, "bp_fully_loaded". It is interesting that we have not needed anything hooked to wp_loaded so far, and unfortunate we have bp_loaded (plugins_loaded) already. I wonder if JJJ has an opinion on naming or how to proceed as these core actions are his architecture?

[boonebgorges]

Unfortunately i cannot rely on bp_ready as it's too late for the activity administration screen (dropdown list of activity actions).

I am not a huge fan of the name of the new core action, "bp_fully_loaded".

I changed 'bp_activity_tracking_register_post_types' to 'bp_ready' and the admin dropdown seems to be populating fine. And to be honest, if it's broken, we should be fixing the dropdown rather than creating a new action - we should not be generating markup before 'bp_ready'. Let's try to get rid of this new action.

Some nitpicky feedback as I look through the patch more closely:

• Why the clever call_user_func_array() stuff in bp_activity_tracking_register_post_types()? Can't we just call bp_set_activity_action() procedurally?

• do_action( 'bp_activity_tracking_register_post_type' ); should be a filter. We want to discourage plugin authors from reaching into the $bp global. Let them modify the track array before we stash it.

• We should sort by 'position' at the time of registration, not at the time of output. That way we only have to do it once, and the code is cleaner on output.

• bp_activity_set_post_type_tracking_args() is great. This, plus support in register_post_type(), provides a ton of flexibility to plugin authors. If you're going to use bp_parse_args() there, be sure to pass a third param so it can be filtered. (Or just use array_merge().)

• 'admin_filter' needs the same treatment as 'bp_activity_new_post' and 'bp_activity_new_post_ms'. We can't sprintf() like this.

• In bp_activity_post_type_publish(), I think your filter on 'hide_sitewide' is incorrect (and do we need a filter here at all? It all runs through bp_parse_args() later)

• I'm a bit confused by your filter backward compatibility in bp_activity_post_type_publish(). I suggest: remove the individual filters from $activity_args, and only run the legacy hooks if 'component=blogs'. That filter can be hardcoded - no need to assemble dynamically from the component name. Then have a single filter on all the args, or none at all; see my comment above.

[imath]

Replying to boonebgorges:

Unfortunately i cannot rely on bp_ready as it's too late for the activity administration screen (dropdown list of activity actions).

I am not a huge fan of the name of the new core action, "bp_fully_loaded".

I changed 'bp_activity_tracking_register_post_types' to 'bp_ready' and the admin dropdown seems to be populating fine. And to be honest, if it's broken, we should be fixing the dropdown rather than creating a new action - we should not be generating markup before 'bp_ready'. Let's try to get rid of this new action.

Actually i was wrong. "wp" so "bp_ready" are not fired at all into the activity administration. It seems that "wp" hook in the WordPress administration is only fired when viewing wp-admin/edit.php. As a screenshot master, here are some proofs :

Activity Administration index page

Activity Administration single item page

As the dropdowns wasn't populating in the administration, i gave up with 'bp_ready'. I've just done some testing and it appears this hook is not fired when submitting a post. So we cannot use it at all.

I understand the new hook, which at first wasn't bothering you, is now very annoying.

So to avoid making things wrong another time, could we find an agreement on the way you and the team want to deal with this :

1. Use the lazy method you recommended
2. Create a new hook on 'wp_loaded' (the annoying 'bp_fully_loaded')
3. add_action( 'init', 'bp_activity_tracking_register_post_types', PHP_INT_MAX );

My concern about the lazy method is link to the way activity actions are get. In core, and surely in plugins and themes playing with them, we are getting the actions this way buddypress()->activity->actions. And i haven't found any wrapper function to get all actions.
The only way i see is creating a wrapper function and replace all occurrences of buddypress()->activity->actions in the code, but then i think there's a risk plugins and themes will keep on directly using buddypress()->activity->actions. But if you and the team are thinking it's the way, then i'll use it.

So 1, 2, 3 or another option ?

[boonebgorges]

Gah, this issue is very minor and annoying :) Under no circumstances should we do anything like (3). Another option is to just hook the damn thing in two places:

add_action( 'bp_ready', 'foo' );
add_action( 'admin_init', 'foo' );

This is not beautiful, but it's better IMO than adding another arbitrary action. We can come back to the issue at a later date. (I still think my lazoloading idea is more elegant, but I don't care enough to argue for it anymore.)

The only way i see is creating a wrapper function and replace all occurrences of buddypress()->activity->actions in the code

Regardless of what happens with the hooks, we should do this anyway. If plugin authors reach directly into the global when we have provided an easy-to-use wrapper, then it's their own fault if something breaks in the future. It was an oversight not to do it in the first place.

[imath]

Replying to boonebgorges:

I still think my lazoloading idea is more elegant.

Since the beginning of our discussions on this ticket, you've convinced me. The buddypress()->activity->actions thing was bothering me. So after a short night thinking about it, i thought let's try to do it to have an alternative to the way the patch was built so far.

So i've changed some functions and tried to build the tracking feature the lazyloading way. So in ​5669.05.patch, i'm not using hook(s) to register a buddypress()->activity->track global anymore.

I now have a function i'm using just before publishing, updating or removing the activity : bp_activity_get_post_type_tracking_args( $post_type = '' )

There's also bp_activity_get_post_types_tracking_args() which loops through the registered post types and uses the function i've mentioned above to build all "post types tracking arguments".

This function is used at 2 places :

• to populate the dropdown lists
• inside the format_callback function

About the dropdowns in front and admin they are now using a wrapper function to buddypress()->activity->actions :
bp_activity_get_actions() first fetches all post types to eventually add the actions about the post types and sort each component actions by their "position" order.

About the format callback, as this function is used for each activity of the type listed in the stream, i thought maybe it could be interesting to use bp_activity_get_post_types_tracking_args() only once, so that's the only place where i've kept buddypress()->activity->track.

About the 7 points of your "nitpicky feedback", i've included them all and your point about the 'admin_filter' made me thought maybe we could leave all tracking args being customizable from the post types register arguments.

So it's now possible to define all the tracking arguments using for instance :

$labels = array(
	'name'                    => 'bars',
	'singular_name'           => 'bar',
        'bp_activity_new_post'    => __( '%1$s shared a new <a href="%2$s">bar</a>', 'bar-domain' ),
	'bp_activity_new_post_ms' => __( '%1$s shared a new <a href="%2$s">bar</a>, on the site %3$s', 'bar_domain' ),
);

register_post_type( 'bars', array(
	'labels'   => $labels,
	'public'   => true,
	'supports' => array( 'buddypress-activity' ),
	'bp_activity => array( 'component_id' => 'my_custom_component', 'action_id' => 'my_custom_action' .....), 
) );

we still can do a simple :

add_post_type_support( 'page', 'buddypress-activity' );

In this case BuddyPress will use generic strings, format callback etc...

Finally, we can also set one or all arguments using bp_activity_set_post_type_tracking_args()

	add_post_type_support( 'page', 'buddypress-activity' );

	bp_activity_set_post_type_tracking_args( 'page', array(
		'component_id'             => buddypress()->blogs->id,
		'action_id'                => 'mabelle_page',
		'bp_activity_admin_filter' => 'Nouvelle page publiée',
		'bp_activity_front_filter' => 'Nouvelle page',
		'contexts'                 => array( 'member', 'activity' ),
		'activity_commment'        => false,
		'bp_activity_new_post'     => '%1$s shared a new <a href="%2$s">test page</a>',
		'bp_activity_new_post_ms'  => '%1$s shared a new <a href="%2$s">test page</a>, on the site %3$s',
		'position'                 => 100,
	) );

I've edited the unit tests, ran it and checked on a multisite config, everything seems to work fine.

I need extra eyes :) Thanks again for your help.

[imath]

Lazy Loading version of the patch

[boonebgorges]

In 9191:

Introduce bp_sort_by_key().

A companion for bp_alpha_sort_by_key(), this function allows arrays of
objects or associative arrays to be sorted according to one of their keys/
properties - a kinda-sorta version of wp_list_pluck() for sorting. Supports
numeric as well as alphabetical sorts.

bp_alpha_sort_by_key() is converted to a wrapper of the new function.

Fixes #6047. See #5669.

[boonebgorges]

5669.2.patch​ updates the latest in a couple ways:

• I knew we already had a utility function for sorting, so I expanded it [9191] and implemented it for 'position' sorting.
• Changed the fallback values for the post type strings. We can't do sprintf( $string, $singular_name ) - this just won't be grammatical in many languages. I settled on "[author] wrote an item", but other suggestions are welcome.
• Cleaned up some code standards, documentation, etc.

I'm feeling pretty good about this. imath, have another look. If no one has objections, I think we should go with it.

[imath]

Replying to boonebgorges:

imath, have another look. If no one has objections, I think we should go with it.

Great !!

I'll give it another look tomorrow, thanks a lot for the improvements :)

[imath]

Actually, i was too excited to wait till tomorrow!

So i had another look on it and found that i've missed something while building the lazy loading version patch. I forgot to edit the checks i was doing in bp_activity_can_comment() and i've misspelled the 'activity_comment' tracking arguments (i've used activity_commment - 3 m)

And i've also added the src/bp-core/deprecated/2.2.php and tests/phpunit/testcases/activity/actions.php that disappeared in a patch manipulation.

I've played the tests on regular & multisite configs, and tested everything again, and everything was ok :)

so ​5669.3.patch should be ready to go!

Finally a big +1 to:

I think we should go with it.

[boonebgorges]

YOLO

[imath]

In 9194:

Post Type Activity Tracking feature

So far the tracking feature was requiring the Blogs component to be active. In 2.2 we are centralizing the majority of the tracking code into the Activity component. A new set of functions and hooks has been created to catch public post types supporting the feature 'buddypress-activity' and to automatically generate an activity when a new item is publicly published.

It's now possible to add this support to any public post type using one unique line of code, eg: add_post_type_support( 'page', 'buddypress-activity' ). In this case BuddyPress will use generic activity attributes.
Each activity attribute of the supported post type can be customized using a specific function (eg: set custom strings to describe the post type activity action).

When registering a post type in WordPress it's also possible to set the 'buddypress-activity' feature using the support parameter of the second argument of the register_post_type() function. Custom activity action strings can be defined within the labels parameter and activity attributes can be set using the new parameter 'bp_activity'.

When the Blogs component is active, the 'post' post type is automatically supporting the 'buddypress-activity' feature. The conditional logic (eg: blog_public option set to 1 ...) that occurs before a new activity is posted and the comments tracking remain unchanged.

props boonebgorges, DJPaul

Fixes #5669

[tw2113]

imath I'm curious about the usage of bp_activity_delete_by_item_id() in bp-activity-functions since PHPStorm is reporting that it's a deprecated function and that users should instead use bp_activity_delete(). Thoughts?

[tw2113]

In 9384:

Add hooks docs for newly added hooks in 2.2.0.

See #5669.

[imath]

Replying to tw2113:

imath I'm curious about the usage of bp_activity_delete_by_item_id()

As i was working on blogs component functions, i guess i took what was in place in bp_blogs_delete_activity() to avoid any regression. There are others place it's used such as friends_delete_activity(), bp_groups_delete_group_delete_all_activity() and in xprofile_delete_activity().

I think, there's a problem here. If it was really deprecated this function would be in bp-core/deprecated/1.2.php and use in it _deprecated_function(). IMHO, there's a possibility plugin are using it, so if we are to deprecate it i wonder if it would be fair play to do so in 1.2..