Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 10 years ago

#3622 closed defect (bug) (fixed)

bp_get_sitewide_activity_feed_link()

Reported by: webraket Owned by:
Milestone: 1.5.1 Priority: normal
Severity: major Version: 1.5
Component: Activity Keywords: has-patch needs-testing
Cc:

Description

bp_get_sitewide_activity_feed_link() doesn't works as expected in one of my plugins.

It returns {home_url}/feed/ where it should be {home_url}/activity/feed/

Looks like bp_get_activity_slug() or bp_get_activity_root_slug() is causing the problem, seems to return an empty string.

I also think the function should use network_home_url() instead of home_url().

function test(){
echo bp_get_sitewide_activity_feed_link();
}
add_action('init', 'test');

Attachments (2)

3622.01.patch (633 bytes) - added by boonebgorges 10 years ago.
3622.02.patch (1.5 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (21)

#1 @boonebgorges
10 years ago

  • Component changed from Core to Activity
  • Keywords has-patch reporter-feedback needs-testing added
  • Milestone changed from Awaiting Review to 1.5.1
  • Severity changed from normal to minor

Do you have the activity component disabled? Have you associated a WP page with the Activity component (Dashboard > BuddyPress > Pages)? One of these things probably explains why bp_get_activity_root_slug() is failing for you. You might also turn on WP_DEBUG to trace the problem.

In any case, you are probably right that we should not be using home_url(). network_home_url() is not right either; some people use BP on a secondary blog. We should be using bp_get_root_domain().

Could I ask you to check the following patch on your installation, after you've also checked to make sure you have an Activity WP Page?

#2 @webraket
10 years ago

Do you have the activity component disabled?
No, it's enabled.

Have you associated a WP page with the Activity component (Dashboard > BuddyPress > Pages)?
Never done this myself, but I guess those pages where created automatically after upgrading from 1.2.9.
I can see those items at dashboard->all_pages on the root/main blog.

Please note I hook to the "init" function, the plugin is network-wide enabled and executes even if no buddypress component is loaded (maybe this causes problems?)

<?php
class BP_PubSubhubbub {
    
	public function __construct(){
		
		$this->hub_urls[] = "http://pubsubhubbub.appspot.com";
		//$this->feed_urls[] = bp_get_sitewide_activity_feed_link();
		$this->feed_urls[] = network_home_url().'activity/feed/';
		
		add_action('bp_activity_sitewide_feed_head', array(&$this,'add_rss_link_tag'));
		add_action('publish_post', array(&$this,'publish_to_hub'), 999);
		
	}
	
	function add_rss_link_tag() {    
		//
	}
	
	function publish_to_hub($post_id)  {
		//
	}
    
    private function http_post($url, $post_string) {
		//
    }
	
}

function Init_BP_PubSubHubbub(){
	$oBP_PubSubHubbub = new BP_PubSubHubbub;
}
add_action('init', 'Init_BP_PubSubHubbub');
?>
Last edited 10 years ago by webraket (previous) (diff)

#3 @webraket
10 years ago

  • Keywords reporter-feedback removed

#4 follow-up: @boonebgorges
10 years ago

  • Severity changed from minor to major

Never done this myself, but I guess those pages where created automatically after upgrading from 1.2.9.

Could you double check? Dashboard > BuddyPress > Pages

Also, did you check the patch I put up?

Are you by chance running BP_ENABLE_MULTIBLOG? (If you haven't explicitly enabled it in wp-config, you're not.)

I don't think that hooking to init should cause any problems, though if you want to be safe, hook to bp_init instead.

After a bit of review, I think there is a problem with the way that bp_core_get_directory_pages() is creating slugs, which may be at the root of this problem. The logic for selecting the $posts_table_name is incorrect. I think that this problem is also at the root of a couple other tickets that we closed as 'can't reproduce' in the past few months.

Please test 3622.02.patch, which should solve the problem I've discovered, and may solve yours as well.

#5 @boonebgorges
10 years ago

  • Keywords reporter-feedback added

#6 in reply to: ↑ 4 @webraket
10 years ago

Could you double check? Dashboard > BuddyPress > Pages

You probably mean Network Admin->Dashboard->BuddyPress->Pages.
Yes they are listed (triple checked).
However I never added these myself.
I guess this was done after the upgrade from 1.2.9. (which is good)

Those (buddypress related) pages are also visible at:
dashboard->all_pages on the root/main blog
So I can change the order, rename, etc from WordPress itself. (great improvement)

Are you by chance running BP_ENABLE_MULTIBLOG
No, never added something like that in wp-config.php
I only use BuddyPress network-enabled on multisite.

hook to bp_init instead
Sorry, I don't trust bp_init at the moment.
I experienced problems yesterday on another custom plugin (activity stream ajax refresh).
Basically since 1.5 you can't add actions to wp_ajax{callback_function} from inside classes/functions hooked to bp_init.
I ended up hooking to bp_setup_globals (still confused a bit, but this is not related to this ticket) (I wish I had more time to create a seperate ticket)

I'm waiting for your patch(3622.02), and report back as soon as possible.

#7 follow-up: @boonebgorges
10 years ago

The patch 3622.02 is attached to this ticket. If you're reading this in an email, click through to it.

Sorry, I don't trust bp_init at the moment.

bp_init is itself hooked to init. http://buddypress.trac.wordpress.org/browser/trunk/bp-core/bp-core-hooks.php#L17

you can't add actions to wp_ajax{callback_function} from inside classes/functions hooked to bp_init.

Load order was changed in 1.5, so you may be right. I suggest that you load your base class at bp_loaded. This is what the new BP Skeleton Component will recommend when it comes out in the upcoming days: https://github.com/boonebgorges/buddypress-skeleton-component/blob/1.5-compat/includes/bp-example-loader.php#L364

#8 @webraket
10 years ago

The patch 3622.02 is attached to this ticket
I only see patch 3622.01 ?

For my temporary fix

$this->feed_urls[] = network_home_url().'activity/feed/';

I now use bp_get_root_domain() as you advised.

I also did some debugging with:

bp_get_root_domain() . '/' . bp_get_activity_root_slug() . '/feed/'

Hooked to init it returns http://testbp.org//feed/
Hooked to wp_footer it returns http://testbp.org/activity/feed/ (like it should)

bp_init seems to be hooked to init with priority less then 10, so that should make no difference.
It is already executed before my plugin comes into place.

I wonder why bp_get_activity_root_slug() returns a empty string at this point.
At which point is $bp->activity->root_slug currently defined in the source?

Last edited 10 years ago by webraket (previous) (diff)

#9 follow-up: @boonebgorges
10 years ago

Sorry, I'd forgotten to attach 3622.02.patch. The more I think about it, the more I think it probably won't affect your issue (though it is a necessary fix in its own regard).

$bp->activity->root_slug is defined in the activity loader class https://buddypress.trac.wordpress.org/browser/trunk/bp-activity/bp-activity-loader.php#L59; this method is hooked to bp_setup_globals, which is run at bp_init with priority 4.

bp_init seems to be hooked to init with priority less then 10, so that should make no difference.

Nope. It's loaded at 10 (the default priority).
http://buddypress.trac.wordpress.org/browser/trunk/bp-core/bp-core-hooks.php#L17

This probably means: Both bp_init (and thus bp_setup_globals) and your Init_BP_PubSubHubbub are being hooked to 'init' at priority 10. Since they have identical priorities, it's a matter of chance on who gets loaded first. It must be the case that your function is winning in this case.

I suggest defining $this->feed_urls[] inside of a method that is hooked to bp_setup_globals.

#10 in reply to: ↑ 7 @webraket
10 years ago

Replying to boonebgorges:

I suggest that you load your base class at bp_loaded. This is what the new BP Skeleton Component will recommend when it comes out in the upcoming days

bp_loaded is hooked to plugins_loaded, that seems a bit too early.

I like to construct my base class at least after wordpress loaded the current user to make use of functions like is_super_admin() etc.

I mostly hook my base classes (for BuddyPress only plugins) from bp_init, I expect this to still be fine just not for some functions anymore like wp_ajax (and probably some other functions).

Last edited 10 years ago by webraket (previous) (diff)

#11 in reply to: ↑ 9 @webraket
10 years ago

  • Keywords reporter-feedback removed

Replying to boonebgorges:

Sorry, I'd forgotten to attach 3622.02.patch. The more I think about it, the more I think it probably won't affect your issue (though it is a necessary fix in its own regard).

$bp->activity->root_slug is defined in the activity loader class https://buddypress.trac.wordpress.org/browser/trunk/bp-activity/bp-activity-loader.php#L59; this method is hooked to bp_setup_globals, which is run at bp_init with priority 4.

bp_init seems to be hooked to init with priority less then 10, so that should make no difference.

Nope. It's loaded at 10 (the default priority).
http://buddypress.trac.wordpress.org/browser/trunk/bp-core/bp-core-hooks.php#L17

This probably means: Both bp_init (and thus bp_setup_globals) and your Init_BP_PubSubHubbub are being hooked to 'init' at priority 10. Since they have identical priorities, it's a matter of chance on who gets loaded first. It must be the case that your function is winning in this case.

I suggest defining $this->feed_urls[] inside of a method that is hooked to bp_setup_globals.

You're right, I missed that default priority.
Then bp_init instead of init, does make a difference (not expected that) and should fix my problem.

But I still don't fully trust bp_init because of the wp_ajax issue. (indicates some wordpress core actions are executed before bp_init is executed) thus cannot be hooked.

I aggree bp_setup_globals is the best way to go for now.

Thanks for your great support, really fast and detailed ;)

#12 @webraket
10 years ago

Now I also know why

add_action('wp_ajax_')

isn't working any more (since 1.5) when called from bp_init with the default priority.

function bp_core_add_ajax_hook() {
	// Theme only, we already have the wp_ajax_ hook firing in wp-admin
	if ( !defined( 'WP_ADMIN' ) && isset( $_REQUEST['action'] ) )
		do_action( 'wp_ajax_' . $_REQUEST['action'] );
}
add_action( 'bp_init', 'bp_core_add_ajax_hook' );

/mu-plugins are loaded before /plugins, that's why BuddyPress loses the battle with the same priority.
Isn't it better to always use priority's lower then the default(10) in the core?

bp-core-hooks.php:
add_action( 'init','bp_init' );
add_action( 'init', 'bp_init', 9 );

bp-core-functions.php:
add_action( 'bp_init', 'bp_core_add_ajax_hook' );
add_action( 'bp_init', 'bp_core_add_ajax_hook', 9 );

#13 @boonebgorges
10 years ago

Thanks for your great support, really fast and detailed ;)

I wish more plugin developers were in here asking questions like this :)

Isn't it better to always use priority's lower then the default(10) in the core?

I would like to encourage developers whose plugins are BP-dependent to use bp_init instead, so that they can rely on our internal load order (which is predictable) instead of the load order of items attached to init, which will vary from installation to installation. If your only problem is wp_ajax_ hooks, we might consider shifting the priority on our implementation of wp_ajax_ http://buddypress.trac.wordpress.org/browser/trunk/bp-core/bp-core-functions.php#L906 - if it fired with priority 8, for instance, your plugin hooked to bp_init at 10 should be able to see it properly.

#14 @webraket
10 years ago

I agree to keep bp_init hooked to init at the default priority.
Plugins that should also work without BP can simply hook to both.

BP should however never hook to the default priority of it's own bp_init.
Or at least never when do_action() or apply_filters() is called at those points.

If a developer only tests his plugin in /plugins, it leads to unexpected results when the plugin is used in /mu-plugins.

Maybe it's even better to move all core functions currently hooked to bp_init to something like bp_init_core, then hook bp_init_core to bp_init with priority 0.
That way all priority's for bp_init are kept left for plugins.
If someone wants to hook to bp_init and do something before BP is actually loaded, they can use a negative priority I suppose.

Last edited 10 years ago by webraket (previous) (diff)

#15 @boonebgorges
10 years ago

BP should however never hook to the default priority of it's own bp_init.

I think this is mainly right. I think it's OK for core components to hook to bp_init, since they operate like plugins anyway. But I think that core functions (like bp_core_admin_menu_init and bp_core_add_ajax_hook) should probably be run sooner than that, so that plugins can reliably access them.

This conversation has gone pretty far afield. I'm going to open a separate ticket for the bp_init issue: #3625. I'll leave this ticket open for the patches above.

#16 @cnorris23
10 years ago

@webraket bp_get_sitewide_activity_feed_link() is broken, just called to early, as was more or less discussed already.

<?php
class BP_PubSubhubbub {
    
	public function __construct(){
		
		add_action('bp_setup_globals', array(&$this,'setup_globals'));
		add_action('bp_activity_sitewide_feed_head', array(&$this,'add_rss_link_tag'));
		add_action('publish_post', array(&$this,'publish_to_hub'), 999);
		
	}
	
	function setup_globals() {    
		
		$this->hub_urls[] = "http://pubsubhubbub.appspot.com";
		$this->feed_urls[] = bp_get_sitewide_activity_feed_link();
		
	}
	
	function add_rss_link_tag() {    
		//
	}
	
	function publish_to_hub($post_id)  {
		//
	}
    
    private function http_post($url, $post_string) {
		//
    }
	
}

function Init_BP_PubSubHubbub(){
	$oBP_PubSubHubbub = new BP_PubSubHubbub;
}
add_action('bp_include', 'Init_BP_PubSubHubbub');
?>

Based on the above code, give bp_get_sitewide_activity_feed_link() another try, without any of the patches from this ticket applied. My guess is that bp_get_sitewide_activity_feed_link() worked just fine, but judging from boone's patches, it's possible we inadvertently found bug.

#17 @boonebgorges
10 years ago

(In [5193]) Use bp_get_root_domain() when building link in bp_get_sitewide_activity_feed_link(). See #3622

#18 @boonebgorges
10 years ago

(In [5194]) Use bp_get_root_domain() when building link in bp_get_sitewide_activity_feed_link(). See #3622

#19 @boonebgorges
10 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Moving discussion of the 'pages' issue (the second patch on this ticket) to #3638. bp_init discussion should continue at #3625

Note: See TracTickets for help on using tickets.