WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 2 months ago

#43869 new defect (bug)

Do not initialize current user too early in `get_user_locale()`

Reported by: flixos90 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: has-patch has-unit-tests 2nd-opinion
Focuses: Cc:

Description

The current user in WordPress is typically initialized right before the init action, in wp-settings.php. While WordPress has measures in place to initialize the current user on request, this is rather unexpected and we should try to avoid it.

There is one function in core that causes the current user to be initialized in such an unexpected way, namely the get_user_locale() function. It is at least called by load_default_textdomain() (after the setup_theme hook), but most times even earlier, when plugins call load_plugin_textdomain() (usually on plugins_loaded).

Of course it's necessary to load the user-specific locale that early, so it's not an option to set it up later. However, it shouldn't be necessary for get_user_locale() to set the current user.

  • First of all, it doesn't need to access a full user object. Initializing a user object is a heavy process (unless the current user), and all we need to do here is look up a meta value for the locale, for which a user's ID is sufficient.
  • I suggest to do the following: If no $user_id is passed, call get_current_user_id() if we're at a point in the bootstrap flow where the current user is already set up. Otherwise, we can use the filter determine_current_user which is internally used by _wp_get_current_user() and returns the current user's ID on success. We might wanna consider centralizing that logic in a utility function.

Attachments (1)

43869.diff (2.2 KB) - added by flixos90 2 months ago.

Download all attachments as: .zip

Change History (4)

@flixos90
2 months ago

#1 @flixos90
2 months ago

  • Keywords has-patch has-unit-tests 2nd-opinion added; needs-patch needs-unit-tests removed

43869.diff gets rid of the unnecessary usage of user objects in get_user_locale() and also no longer sets the current user if the function is called too early in the bootstrap process. A patch to verify that behavior is also provided.

#2 @ocean90
2 months ago

Previously: #24169

Can you explain why this is an issue for you?

#3 @flixos90
2 months ago

@ocean90

I generally think initializing the current user early violates expectations resulting from the typical flow of the bootstrap process and the order of things happening in wp-settings.php.

The current user is generally initialized right before the init hook and the process of initializing fires a set_current_user hook. You should be able to expect that, when using this hook, you're just before init (or later) in the flow. Since a call to load_plugin_textdomain() (usually called early on plugins_loaded) will currently set the current user, but many plugins initialize their entire functionality on plugins_loaded too, making it hard and sometimes impossible to use the set_current_user hook under correct assumptions.

The problem I personally ran into was the following: I was filtering user_has_cap in a plugin and relying on plugin functionality that is loaded on plugins_loaded. However, since the current user was initialized before, the kses_init() function was executed (which is hooked into set_current_user by core). That function checks whether current_user_can( 'unfiltered_html' ), causing the user_has_cap filter to trigger unexpectedly early, where some of the plugin functionality was not yet available.

Back to an approach for fixing this: If the general consensus is that setting the current user that early is not unexpected or if my initial patch is considered a little too risky/clunky, I'd like to at least have no unnecessary user_has_cap filter fire as early. We can ensure this by executing the line add_action( 'set_current_user', 'kses_init' ) on init, to ensure it only fires afterwards. It shouldn't be necessary to initialize KSES behavior before that anyway. In case a process changes the current user, the behavior is still reinitialized correctly, since we still have the filter in place. But before init, it's pretty useless anyway.

Note: See TracTickets for help on using tickets.