Skip to:
Content

Opened 2 years ago

Closed 20 months ago

#6589 closed defect (bug) (fixed)

bp_setup_current_user() over zealous checking for did_action( "after_setup_theme" )

Reported by: bobbingwide Owned by: johnjamesjacoby
Milestone: 2.6 Priority: normal
Severity: normal Version: 1.7
Component: Core Keywords: has-patch commit
Cc:

Description

With WordPress-SEO, BuddyPress and WP_DEBUG true, BuddyPress produces the following _doing_it_wrong message in the Dashboard.

The current user is being initialized without using $wp->init().

In my opinion this message can be avoided by changing the test to something more closely aligned to the problem it is try to warn against... the fact that the current user may not be set.

With the current version of WordPress, 4.2 and above, the test can be changed to

global $current_user; 
if ( is_null( $current_user ) ) {
    _doing_it_wrong( ... );
}


See http://wordpress.stackexchange.com/questions/198185/what-is-valid-timing-of-using-current-user-can-and-related-functions.

bbPress has the same problem.

An alternative approach, that I did not investigate, is to not register the action hook for bp_set_current_user until 'after_setup_theme' has been fired.

Attachments (1)

6589.01.patch (854 bytes) - added by r-a-y 21 months ago.

Download all attachments as: .zip

Change History (19)

#1 @DJPaul
2 years ago

Think this is/was used to detect a number of non-trivial user roles/capabilities issues that was discovered when bbPress 2 was being built. Need to let JJJ look at this as he did the work on that, this might not be such an easy change.

#2 @DJPaul
2 years ago

  • Milestone changed from Awaiting Review to Under Consideration
  • Owner set to johnjamesjacoby
  • Status changed from new to assigned

#3 @netweb
2 years ago

  • Keywords close added; needs-patch 2nd-opinion removed

This is not a BuddyPress or bbPress issue, this is a WordPress SEO issue https://github.com/Yoast/wordpress-seo

See https://codex.bbpress.org/bbp_setup_current_user/was-called-incorrectly/

This was also supposedly fixed in WordPress SEO by Yoast a few months ago and appears to be happening again :/
https://github.com/Yoast/wordpress-seo/blob/trunk/readme.txt#L253

Release Date: April 21st, 2015

* Bugfixes:
	* Fixes a compatibility issue with BBPress caused by hooking `current_user_can` too early.

#4 @bobbingwide
2 years ago

Hi @netweb, even though Rarst has addressed this issue for WordPress SEO the problem still exists with other plugins that use current_user_can() before "after_setup_theme".
In my limited experience both BuddyPress and bbPress are working fine.

Another alternative would be not to invoke the 'bp_setup_current_user' action either by testing did_action( 'after_setup_theme') or not calling add_action( 'bp_setup_current_user') until 'after_setup_theme' has been invoked.

#5 @DJPaul
2 years ago

  • Milestone Under Consideration deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

I don't think we want to change this. All the instances we've had reported have always ended up showing problems in other plugins (minus one for the Customiser in wp-core, which we special-case).

#6 @bobbingwide
2 years ago

  • Keywords close removed
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Hi Paul, Rarst's other example was his own plugin - Toolbar Theme Switcher.
He argued in the stack exchange link he had to do it this way.

Do you want to reconsider?

#7 @r-a-y
22 months ago

  • Keywords has-patch added
  • Milestone set to Under Consideration
  • Priority changed from high to normal
  • Severity changed from minor to normal
  • Version changed from 2.3.2 to 1.7

I think we should add more info to these bp_setup_current_user() notices so plugin devs can see where their code is throwing these notices.

Attached patch does this by using Exception::getTraceAsString(), which is available in PHP 5.1+.

This is what the notice looks like with the patch:

[08-Apr-2016 00:32:38 UTC] PHP Notice:  bp_setup_current_user was called <strong>incorrectly</strong>. The current user is being initialized without using $wp->init().
===
Trace:
#6 wp-content\plugins\bp-custom.php(53): is_user_logged_in()
#7 [internal function]: BuddyPress->{closure}('')
#8 wp-includes\plugin.php(525): call_user_func_array(Object(Closure), Array)
#9 wp-settings.php(276): do_action('plugins_loaded')
#10 wp-config.php(109): require_once(...)
#11 wp-load.php(37): require_once(...)
#12 wp-blog-header.php(12): require_once(...)
#13 index.php(17): require(...)
#14 {main}
===
 Please see <a href="https://codex.wordpress.org/Debugging_in_WordPress">Debugging in WordPress</a> for more information. (This message was added in version 1.7.) in wp-includes\functions.php on line 3792

@bobbingwide - If you really wanted to, you can suppress these notices with a custom PHP error handler:
https://gist.github.com/r-a-y/bab85fc47fa0357a8100

Last edited 22 months ago by r-a-y (previous) (diff)

#9 @DJPaul
21 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Under Consideration to 2.6

new \Exception is invalid syntax in PHP 5.2. :( :)

Do we really need that here?

#10 @r-a-y
21 months ago

  • Keywords has-patch added; needs-patch removed

new \Exception is invalid syntax in PHP 5.2

Good catch! The namespacing isn't necessary.

I've refreshed 01.patch to remove the backslash.

@r-a-y
21 months ago

#11 @DJPaul
21 months ago

  • Keywords commit added

#12 @r-a-y
21 months ago

In 10709:

Core: Add debug trace to bp_setup_current_user() notice.

Previously, the bp_setup_current_user was called incorrectly notice
would not output any useful info for developers.

This commit adds a debug trace so developers are able to determine
whereabouts in the codebase this notice is originating from.

See #6589.

#13 @netweb
21 months ago

Nice, is there anything else here @r-a-y ? I've added a patch in #bbPress2932 and works great :)

#14 follow-up: @r-a-y
21 months ago

Nice, is there anything else here

@netweb - Not really. Perhaps we can alter the notice message to say something to the effect of "Please try running your current user check on the 'init' hook or later."

"The current user is being initialized without using $wp->init()" is a little vague if you're not familiar with WP internals.

#15 in reply to: ↑ 14 @netweb
21 months ago

Replying to r-a-y:

Nice, is there anything else here

@netweb - Not really. Perhaps we can alter the notice message to say something to the effect of "Please try running your current user check on the 'init' hook or later."

"The current user is being initialized without using $wp->init()" is a little vague if you're not familiar with WP internals.

Sounds good :+1

This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.


21 months ago

#17 @espellcaste
21 months ago

This is a really nice and very useful enhancement.

#18 @DJPaul
20 months ago

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

This is done. If someone cares enough to change the string, that can be done in a new ticket.

Note: See TracTickets for help on using tickets.