Make WordPress Core

Opened 18 months ago

Closed 18 months ago

Last modified 17 months ago

#56952 closed defect (bug) (fixed)

cache_users() not defined when calling get_user without field parameter or using all_with_meta or all

Reported by: carazo's profile carazo Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: 6.1.1 Priority: normal
Severity: major Version: 6.1
Component: Users Keywords: has-patch has-testing-info commit fixed-major i18n-change
Focuses: performance Cc:

Description

Good morning,

I have found that in WordPress 6.1 when I call get_users without the field parameter (or using fields = all or fields = all_with_meta) I get a fatal error.

I have been reading the code and it raise here:

Line 843 in file wp-includes/class-wp-user-query.php: cache_users( $this->results );

It seems this function is not defined there.

Could you have a look?

Thanks!

Attachments (1)

56952.diff (641 bytes) - added by oakesjosh 18 months ago.
Proposed patch

Download all attachments as: .zip

Change History (29)

#1 @carazo
18 months ago

  • Component changed from General to Users

#2 @subrataemfluence
18 months ago

  • Keywords needs-patch removed
  • Resolution set to worksforme
  • Status changed from new to closed

Hi @carazo ,

I could not reproduce the issue, it works fine for me even when I invoke get_users() without passing any argument.

Regarding Line 843 in file wp-includes/class-wp-user-query.php: cache_users( $this->results );

The said function cache_users() is located in wp-includes/pluggable.php at Line 125.

<?php
if ( ! function_exists( 'cache_users' ) ) :
        /**
         * Retrieves info for user lists to prevent multiple queries by get_userdata().
         *
         * @since 3.0.0
         *
         * @global wpdb $wpdb WordPress database abstraction object.
         *
         * @param int[] $user_ids User ID numbers list
         */
        function cache_users( $user_ids ) {
                global $wpdb;

                update_meta_cache( 'user', $user_ids );

                $clean = _get_non_cached_ids( $user_ids, 'users' );

                if ( empty( $clean ) ) {
                        return;
                }

                $list = implode( ',', $clean );

                $users = $wpdb->get_results( "SELECT * FROM $wpdb->users WHERE ID IN ($list)" );

                foreach ( $users as $user ) {
                        update_user_caches( $user );
                }
        }
endif;

I am not sure about you setup, but if possible, please deactivate all plugins and switch back to WordPress default theme. If the issue still persists, please feel free to reopen the ticket.

#3 @carazo
18 months ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

@subrataemfluence,

I was calling this function inside this plugin in version 0.5, here: https://plugins.trac.wordpress.org/browser/woo-products-restricted-users/tags/0.5/class-wpru-metabox.php

In line 95, the error appeared there.

#4 @oakesjosh
18 months ago

I am also experiencing cache_users not being defined when calling get_user on 6.1.

Under some conditions, my plugin calls get_users when initialized on the front end. In this context the plugin has loaded, but pluggable.php has not yet. Calling it without manually including wp-includes/pluggable.php results in a fatal error.

I believe the issue is with the context of the call and not the content

#5 @TimothyBlynJacobs
18 months ago

  • Milestone changed from Awaiting Review to 6.1.1
  • Owner set to TimothyBlynJacobs
  • Status changed from reopened to accepted

Moving to 6.1.1, I don't think this should be unsupported usage.

@oakesjosh
18 months ago

Proposed patch

#6 @oakesjosh
18 months ago

  • Keywords has-patch added

I've attached a patch that would:

  • Prevent fatal errors if get_users() is called before pluggable.php is loaded
  • Not prematurely load pluggable.php so plugins can still overwrite functions

#7 @TimothyBlynJacobs
18 months ago

  • Keywords has-testing-info added

Thanks for the patch @oakesjosh!

The alternative would be to manually load pluggable.php instead of skipping the cache call. However, that would end up preventing any plugins loaded after the offending plugin from overriding any pluggable functions.

While not caching these requests isn't ideal, I think it's better than potentially breaking pluggable.php. We could consider adding a _doing_it_wrong here with an indication to wait until plugins_loaded before making any queries as well.

Cc: @peterwilsoncc, @spacedmonkey.

I don't think this is unit testable, since pluggable.php will have already been loaded by the test bootstrap. However, to test manually, create a plugin with the following code.

<?php
/*
 * Plugin Name: Test Plugin
 */

if ( isset( $_GET['test_query'] ) ) {
        $users = get_users();

        wp_die( sprintf( '%d users', count( $users ) ), 'Queried users', array( 'response' => 200 ) );
}

If you visit https://yoursite.com?test_query, you should see the number of users successfully queried on a WP Die screen. Otherwise, you'll see a fatal error.

#9 @spacedmonkey
18 months ago

@TimothyBlynJacobs Added a simple PR to fix this issue.

#10 @spacedmonkey
18 months ago

It is worth noting that cache_users has been called in this class as early as 3.1.

#11 @obenland
18 months ago

Introduced in #55594

obenland commented on PR #3557:


18 months ago
#12

Would this break if a plugin that defines cache_users() without a function_exists() check was loaded after the plugin that called get_users()?

@TimothyBlynJacobs commented on PR #3557:


18 months ago
#13

Yes, though I think that is slightly less of a concern since that can always happen if another plugin overrides a pluggable function.

I don't think this approach works because it will prevent any other plugins that are loaded after the offending plugin from overriding any other pluggable functions.

#14 @obenland
18 months ago

#56988 was marked as a duplicate.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


18 months ago

#16 follow-up: @TimothyBlynJacobs
18 months ago

@dd32, @SergeyBiryukov I’d love your opinions about implementing a doing it wrong notice here. I think plugins ideally wouldn’t be doing user queries before plugins_loaded even prior to the change in 6.1. For instance, plugins also wouldn’t get an opportunity to register necessary filters.

However, I could see MU-Plugins needing to make early queries anyways. So as much as I think it’s “correct” for plugins to wait, I’m not sure we can get away with the notice here. It may just have to be a documentation note.

#17 in reply to: ↑ 16 @SergeyBiryukov
18 months ago

Replying to TimothyBlynJacobs:

I’d love your opinions about implementing a doing it wrong notice here. I think plugins ideally wouldn’t be doing user queries before plugins_loaded even prior to the change in 6.1. For instance, plugins also wouldn’t get an opportunity to register necessary filters.

However, I could see MU-Plugins needing to make early queries anyways.

Hmm, MU-plugins is a good point. Still, I strongly agree with not running anything before plugins_loaded.

Only a few things happen between muplugins_loaded and plugins_loaded:

  • Cookie and SSL constants are defined.
  • Initial post types and taxonomies are created.
  • Default theme directory root is registered.
  • Recovery mode is initialized (for single sites).
  • Active plugins are loaded.
  • Pluggable functions are loaded.

So I would say that even for a MU-plugin, it should be recommended to wait until plugins_loaded before runnning any queries.

Last edited 18 months ago by SergeyBiryukov (previous) (diff)

@spacedmonkey commented on PR #3557:


18 months ago
#18

Looks good to commit.

#19 @spacedmonkey
18 months ago

  • Keywords commit added

The PR #3557 is good to commit IMO.

@TimothyBlynJacobs commented on PR #3557:


18 months ago
#20

Will commit shortly.

#21 @spacedmonkey
18 months ago

  • Focuses performance added

#22 @TimothyBlynJacobs
18 months ago

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

In 54766:

Query: Don't attempt caching if running a WP_User_Query before plugins_loaded.

In #55594 user meta caching was enabled by default when making a WP_User_Query. Previously, this was only enabled if a developer specifically queried for 'all_with_meta'
fields. User meta caching is implemented using a pluggable function, cache_users. If a plugin runs a WP_User_Query before pluggable functions have been defined, this
will now cause a fatal error.

In this commit, a function_exists check is introduced to avoid calling cache_users if it's not defined. Additionally, a _doing_it_wrong notice is issued if the
WP_User_Query::query method is called before the 'plugins_loaded' hook.

Props carazo, subrataemfluence, oakesjosh, spacedmonkey, obenland, SergeyBiryukov, peterwilsoncc.
Fixes #56952.

#23 @TimothyBlynJacobs
18 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#24 @peterwilsoncc
18 months ago

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

In 54773:

Query: Don't attempt caching if running a WP_User_Query before plugins_loaded.

In #55594 user meta caching was enabled by default when making a WP_User_Query. Previously, this was only enabled if a developer specifically queried for 'all_with_meta'
fields. User meta caching is implemented using a pluggable function, cache_users. If a plugin runs a WP_User_Query before pluggable functions have been defined, this
will now cause a fatal error.

In this commit, a function_exists check is introduced to avoid calling cache_users if it's not defined. Additionally, a _doing_it_wrong notice is issued if the
WP_User_Query::query method is called before the 'plugins_loaded' hook.

Props carazo, subrataemfluence, oakesjosh, spacedmonkey, obenland, SergeyBiryukov, peterwilsoncc, TimothyBlynJacobs.
Merges [54766] to the 6.1 branch.
Fixes #56952.

#25 @azurseisme
18 months ago

I have the bug but after applying the patch, I still have:

[Wed Nov 09 22:35:15.833980 2022] [php7:error] [pid 933177] [client 2a01:e0a:824:3ae0:ade:a428:3e00:8342:40936] PHP Fatal error:  Uncaught Error: Call to undefined function wp_cache_get() in /home/Web/Azurseisme/volcanspro/wp-includes/option.php:165\nStack trace:\n#0 /home/Web/Azurseisme/volcanspro/wp-includes/l10n.php(63): get_option()\n#1 /home/Web/Azurseisme/volcanspro/wp-includes/l10n.php(139): get_locale()\n#2 /home/Web/Azurseisme/volcanspro/wp-includes/l10n.php(766): determine_locale()\n#3 /home/Web/Azurseisme/volcanspro/wp-includes/load.php(1395): load_textdomain()\n#4 /home/Web/Azurseisme/volcanspro/wp-includes/class-wpdb.php(1198): wp_load_translations_early()\n#5 /home/Web/Azurseisme/volcanspro/wp-includes/class-wpdb.php(1858): wpdb->select()\n#6 /home/Web/Azurseisme/volcanspro/wp-includes/class-wpdb.php(734): wpdb->db_connect()\n#7 /home/Web/Azurseisme/volcanspro/wp-includes/load.php(562): wpdb->__construct()\n#8 /home/Web/Azurseisme/volcanspro/wp-settings.php(124): require_wp_db()\n#9 /home/Web/Azurseisme/volcanspro/wp-config.php(89): require_once('/home/Web/Azurs...')\n#10 /home/Web/Azurseisme/volcanspro/wp-load.php(50): require_once(' in /home/Web/Azurseisme/volcanspro/wp-includes/option.php on line 165, referer: https://volcanspro.azurseisme.com/les-galapagos/
[Wed Nov 09 22:35:15.842924 2022] [php7:error] [pid 933177] [client 2a01:e0a:824:3ae0:ade:a428:3e00:8342:40936] PHP Fatal error:  Uncaught Error: Call to a member function set() on null in /home/Web/Azurseisme/volcanspro/wp-includes/l10n.php:784\nStack trace:\n#0 /home/Web/Azurseisme/volcanspro/wp-includes/l10n.php(872): load_textdomain()\n#1 /home/Web/Azurseisme/volcanspro/wp-includes/class-wp-fatal-error-handler.php(47): load_default_textdomain()\n#2 [internal function]: WP_Fatal_Error_Handler->handle()\n#3 {main}\n  thrown in /home/Web/Azurseisme/volcanspro/wp-includes/l10n.php on line 784, referer: https://volcanspro.azurseisme.com/les-galapagos/

#26 @TimothyBlynJacobs
18 months ago

Based on that error trace, that looks to be a separate issue @azurseisme. Would you mind opening a new ticket for it?

#27 @azurseisme
18 months ago

For sure, #57051
thanks,

#28 @desrosj
17 months ago

  • Keywords i18n-change added
Note: See TracTickets for help on using tickets.