Make WordPress Core

Opened 6 years ago

Last modified 3 years ago

#43869 reviewing defect (bug)

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

Reported by: flixos90's profile flixos90 Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release 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 6 years ago.

Download all attachments as: .zip

Change History (17)

@flixos90
6 years ago

#1 @flixos90
6 years 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
6 years ago

Previously: #24169

Can you explain why this is an issue for you?

#3 @flixos90
6 years 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.

#4 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 4.9.9
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

Related: #44972

#5 @tazotodua
6 years ago

as that last topic is opened by me, i will add my observation here:

the issue still exists, and i've wrote in that topic, how to reproduce.
After further check, i've found that , if wp-includes/user.php, line 2605

in front of :

wp_set_current_user( 0 );

if we add:

if(did_action('plugins_loaded'))

the problem is fixed. Will that be accepted as patch or it has negative effects (what's more, that check can be added directly in wp_set_current_user function) ?

How to overcome the problem, as many plugins call those functions before plugins_loaded hook, and our dashboard pages are broken?

Last edited 6 years ago by tazotodua (previous) (diff)

#6 @tazotodua
6 years ago

At this moment, i use this as a "temporary solution" (according to my above post):

add_action('set_current_user', function(){ if(!did_action('plugins_loaded')) $GLOBALS['current_user']=null; } );

#7 @pento
6 years ago

  • Milestone changed from 4.9.9 to 5.0.1

We can reconsider this for 5.0.1.

#8 follow-up: @flixos90
6 years ago

Worth noting that [43776] introduces a pre_determine_locale filter, which allows you to short-circuit the default logic. Over the past few months I have come to the conclusion that it seems almost impossible to fix this problem without breaking many people's code. I wonder whether we should close this as maybelater for now, as any site where it becomes a problem could use the above filter instead.

#9 in reply to: ↑ 8 @tazotodua
6 years ago

Replying to flixos90:

Worth noting ...

Hm, that's what i can't understand well. Why it cant be made so it didn't brake many people's code... Instead, it checked if it is being fired too early... or there was a constant which could be set in wp-config.php and avoid that...

shortly, there should be some way. the obvious thing is that it's very problematic and always breaks my sites if any plugin calls get_current_user or like that... very problematic, and i think the WP should solve that somehow..

#10 @pento
6 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#11 @pento
6 years ago

  • Milestone changed from 5.0.2 to 5.0.3

#12 @audrasjb
6 years ago

  • Milestone changed from 5.0.3 to 5.1

Hello,

5.0.3 is going to be released in a couple of weeks.

It doesn't appear this ticket can be handled in the next couple of weeks (still needs review/discussion). Let's address it in 5.1 which is coming in February. Feel free to ask for changing the milestone if you think this issue can be quickly resolved.

Cheers,

Jb

#13 @pento
6 years ago

  • Milestone changed from 5.1 to 5.2

Patch needs further consideration and review.

#14 @ocean90
6 years ago

  • Keywords early added
  • Milestone changed from 5.2 to 5.3

#15 @desrosj
5 years ago

  • Milestone changed from 5.3 to Future Release

This hasn't been touched during the 5.3 cycle. Because it has been punted several times already, I am going to move this to Future Release until it receives the attention it needs.

#16 @johnbillion
3 years ago

  • Keywords early removed
Note: See TracTickets for help on using tickets.