Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Defer customer initialization until 'wp' action hook #497

Closed
garyc40 opened this Issue · 19 comments
@garyc40
Owner

Right now we're initializing current user by calling get_current_user_id() as soon as WPEC is loaded (via plugins_loaded hook). This is not a recommended practice, as WP core initializes the current user just before the 'wp' hook.

As a result, we should re-schedule all the function calls related to customer meta (including the cart) until 'wp' is fired. By then the current user object has already been initialized by WP.

@garyc40 garyc40 referenced this issue from a commit in garyc40/WP-e-Commerce
@garyc40 garyc40 Fix: Make sure customer data is properly initialized after WordPress …
…core has

finished initializing current user object.

Fixed #497.
d05cbd3
@garyc40
Owner

Some more context to this issue:

bbpress starts complaining that user object is being initialized before wp() has finished initializing.

Please test!

@jjeaton

+1, this causes all bbPress admin screens and menus to be inaccessible until WPEC is deactivated.

Clean install of:
WP 3.5.2
bbPress 2.3.1
WPEC 3.8.11.1

Backtrace of Notice:

[27-Jun-2013 14:18:53] PHP Notice:  bbp_setup_current_user was called <strong>incorrectly</strong>. The current user is being initialized without using $wp->init(). Please see <a href="http://codex.wordpress.org/Debugging_in_WordPress">Debugging in WordPress</a> for more information. (This message was added in version 2.3.) in /Users/josh/Sites/www/wp-includes/functions.php on line 2962
[27-Jun-2013 14:18:53] PHP Stack trace:
[27-Jun-2013 14:18:53] PHP   1. {main}() /Users/josh/Sites/www/wp-admin/admin-ajax.php:0
[27-Jun-2013 14:18:53] PHP   2. require_once() /Users/josh/Sites/www/wp-admin/admin-ajax.php:20
[27-Jun-2013 14:18:53] PHP   3. require_once() /Users/josh/Sites/www/wp-load.php:29
[27-Jun-2013 14:18:53] PHP   4. require_once() /Users/josh/Sites/www/wp-config.php:71
[27-Jun-2013 14:18:53] PHP   5. do_action() /Users/josh/Sites/www/wp-settings.php:209
[27-Jun-2013 14:18:53] PHP   6. call_user_func_array() /Users/josh/Sites/www/wp-includes/plugin.php:406
[27-Jun-2013 14:18:53] PHP   7. WP_eCommerce->init() /Users/josh/Sites/www/wp-includes/plugin.php:0
[27-Jun-2013 14:18:53] PHP   8. WP_eCommerce->load() /Users/josh/Sites/www/wp-content/plugins/wp-e-commerce/wp-shopping-cart.php:52
[27-Jun-2013 14:18:53] PHP   9. _wpsc_action_create_customer_id() /Users/josh/Sites/www/wp-content/plugins/wp-e-commerce/wp-shopping-cart.php:190
[27-Jun-2013 14:18:53] PHP  10. wpsc_get_current_customer_id() /Users/josh/Sites/www/wp-content/plugins/wp-e-commerce/wpsc-core/wpsc-functions.php:1811
[27-Jun-2013 14:18:53] PHP  11. is_user_logged_in() /Users/josh/Sites/www/wp-content/plugins/wp-e-commerce/wpsc-core/wpsc-functions.php:1659
[27-Jun-2013 14:18:53] PHP  12. wp_get_current_user() /Users/josh/Sites/www/wp-includes/pluggable.php:727
[27-Jun-2013 14:18:53] PHP  13. get_currentuserinfo() /Users/josh/Sites/www/wp-includes/pluggable.php:54
[27-Jun-2013 14:18:53] PHP  14. wp_set_current_user() /Users/josh/Sites/www/wp-includes/pluggable.php:107
[27-Jun-2013 14:18:53] PHP  15. do_action() /Users/josh/Sites/www/wp-includes/pluggable.php:37
[27-Jun-2013 14:18:53] PHP  16. call_user_func_array() /Users/josh/Sites/www/wp-includes/plugin.php:406
[27-Jun-2013 14:18:53] PHP  17. bbp_setup_current_user() /Users/josh/Sites/www/wp-includes/plugin.php:0
[27-Jun-2013 14:18:53] PHP  18. _doing_it_wrong() /Users/josh/Sites/www/wp-content/plugins/bbpress/includes/core/sub-actions.php:147
[27-Jun-2013 14:18:53] PHP  19. trigger_error() /Users/josh/Sites/www/wp-includes/functions.php:2962
@JustinSainton JustinSainton closed this in #498
@JustinSainton

There have been a few reports that this breaks AJAX - the proposed resolution (c5f6c7f#commitcomment-3548089) is to run on wp_loaded instead of wp.

Does that work for you, @garyc40? Just want to make sure it actually fixes the bbPress compat issues without causing other issues to crop up.

@JustinSainton JustinSainton reopened this
@jjeaton

For bbPress, I believe it just needs to run on init or later, so wp_loaded should be fine. (I think)

Here's the code that throws the Notice in bbpress:

/**
 * Setup the currently logged-in user
 *
 * @since bbPress (r2695)
 * @uses did_action() To make sure the user isn't loaded out of order
 * @uses do_action() Calls 'bbp_setup_current_user'
 */
function bbp_setup_current_user() {

    // If the current user is being setup before the "init" action has fired,
    // strange (and difficult to debug) role/capability issues will occur.
    if ( ! did_action( 'after_setup_theme' ) ) {
        _doing_it_wrong( __FUNCTION__, __( 'The current user is being initialized without using $wp->init().', 'bbpress' ), '2.3' );
    }

    do_action( 'bbp_setup_current_user' );
}
@JustinSainton

Interesting. There may be an unrelated issue going on then - plugins_loaded (the original hook) was failing here, I guess because it's before init.

But the proposed change in c5f6c7f moves it to wp, which is after init - which apparently breaks some of the AJAX functionality. wp_loaded, also after init but before wp, seems to resolve both issues.

@garyc40
Owner

To be honest I don't know why bbpress requires this to be done after init, but in theory, WordPress intializes user data just before wp:
https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp.php#L472
https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp.php#L546

If our AJAX hooks are hooked into wp_ajax_{$action} or wp_ajax_nopriv_{$action} then everything should be fine.

I guess we need to do an audit and move things that is related to customer data (cart, customer meta etc.) from init hook to the new hook wpsc_setup_customer.

@garyc40
Owner

Hmm, I also found this:
https://github.com/WordPress/WordPress/blob/master/wp-settings.php#L299

So yeah, init is probably fine.

@JeffPyeBrook JeffPyeBrook referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@JeffPyeBrook JeffPyeBrook referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@JeffPyeBrook JeffPyeBrook referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@JeffPyeBrook JeffPyeBrook referenced this issue from a commit in JeffPyeBrook/WP-e-Commerce
@garyc40 garyc40 Fix: Make sure customer data is properly initialized after WordPress …
…core has

finished initializing current user object.

Fixed #497.
1a47cb5
@JeffPyeBrook JeffPyeBrook referenced this issue from a commit in JeffPyeBrook/WP-e-Commerce
@JustinSainton JustinSainton Changing _wpsc_action_setup_customer to run on init instead of wp or …
…wp_loaded. See conversations at wp-e-commerce@c5f6c7f and wp-e-commerce#497.  Fixes #497.
a82221a
@JeffPyeBrook JeffPyeBrook referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@JeffPyeBrook JeffPyeBrook referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@visser

This is still an issue. New install of WP e-Commerce 3.8.13 and latest bbPress.

@JustinSainton

Reopening for investigation

@JustinSainton JustinSainton reopened this
@JeffPyeBrook
Collaborator

If someone who can reproduce the issue wants to try a little experiment, put a call to 'wpsc_get_current_customer_id' as the last line in 'customer.php'.

I have this in my local and production copy of customer.php to work around an initialization issue with a plugin I am working on. Issue is weird amd is somehow related to do actions/filters firing in the order that they are registered. Was wondering if it is the same as has been seen with this problem? Probably won't have an effect, but who knows.

@JustinSainton

FWIW, when I activate bbPress (and it's the only plugin being activated, no others are activated) on a fresh MS instance, I get this error:

Notice: bbp_setup_current_user was called incorrectly. The current user is being initialized without using $wp->init(). Please see Debugging in WordPress for more information. (This message was added in version 2.3.) in /home/zaowsai3/public_html/wp-includes/functions.php on line 3049

Related: BB Trac #4830, BB Trac #2309,BB Trac #4957 and WP Core Trac #24169

@JustinSainton

Screenshot for proof: http://d.pr/i/BBbc

@JustinSainton

Screenshot showing the same exact error, except with WPeC activated: http://d.pr/i/a0FG

@JustinSainton

I'll hold off on closing this until we get confirmation from others, but this seems like either a core WP or bbPress bug, not particularly a WPeC bug. Not sure if @johnjamesjacoby is active on GitHub much, but I'll ping him for input just in case.

@JustinSainton

@JeffPyeBrook Assigning this to you. Can you confirm that all of the new customer meta APIs hook in later than 'wp'?

@JustinSainton

For what it's worth, I no longer get the notices in bbPress (as of 2.5.3) and bbPress and WPeC seem to play fine together. I can access all admin screens of both plugins without any issues.

Not sure if @JeffPyeBrook or @jjeaton want to check in to confirm, but closing for now, as it works for me.

@johnjamesjacoby

Hey everyone! The last missing bit about why this is a problem for bbPress/BuddyPress (and most other robust plugins) here: https://core.trac.wordpress.org/ticket/23016

The logged in user, their roles, and their individual capabilities, can accidentally be jump-started in WordPress without core complaining. Core does not complain because everything within the scope of core itself can compensate for the out-of-order loading. (This isn't entirely true, but let's assume it is. See: http://core.trac.wordpress.org/ticket/24169)

Conversely to core, any plugin that loads the user early gets the raw user object before any other plugin has loaded and/or has added actions or filters to set_current_user which is the only action plugins can somewhat reliably hook into to manipulate the logged in user (even WordPress core hooks kses_init() in there.)

The callstack is somewhat complicated:

  • wp_get_current_user()
  • get_currentuserinfo()
  • wp_set_current_user()
  • do_action( 'set_current_user' )

wp_get_current_user() is called 35 individual times in core, each time (except for once above) it's after the init action has fired. It happens here because user specific API's need to be loaded first, like WP_Roles and Gettext_Translations. Specifically, loading the user prematurely can result in role/capability and user language issues. Imagine if a plugin restricted user access based on roles.

Two scenarios:

  • If WPEC loads the user early, and this plugin can't filter out roles in time, WPEC accidentally exposed site information to a user that should never have seen it.
  • A site is multilingual, and installing WPEC forces all languages to English because GetText isn't loaded yet.

These aren't core problems, so core doesn't care to alert anyone when this happens. I'm not a fan of this decision or behavior, and I've done my diligence in reporting and patching, but the reason it sits "unfixed" is likely because of the large amount of plugins accidentally doing it wrong, and the small amount of plugins that it actually effects (BuddyPress and bbPress being two of them.)

Hope this helps. Sorry I didn't chime in sooner. Thanks for taking the time to update WPEC to play nicely.

@JeffPyeBrook
Collaborator

Thanks, that's very helpful.

Based on this information we should be in really good shape with the upcoming release. The only early 'touch' we will have to the current user is a single get user meta call looking for for a saved wpec visitor id. The call happens in 'init' hook processing and only requires that is_user_logged_in and wp_get_current_user are available.

@johnjamesjacoby

Sounds like you are in good shape. Nice work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.