WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

#29785 closed defect (bug) (fixed)

User count not correct

Reported by: psoluch Owned by: boonebgorges
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-unit-tests has-patch needs-testing
Focuses: administration Cc:
PR Number:

Description

Are you using either the latest version of WordPress, or the latest development version?

SVN trunk version

What's the problem?
Count of users is not correct on the Users list in WP Admin.

Reason?
The count_users() function in wp-includes/user.php uses the user_meta table to count the number of users with specific role. If (for some reason) the entry in users table get deleted and the corresponding user_meta entries don't - the count will not be correct.

How did it happen?
Not sure... The user had to be deleted manually from the DB or a custom function did that. It's a rather rare case but might happen.

What steps should be taken to consistently reproduce the problem?

  1. Delete manually a user from a users table (leave the users_meta entries)
  2. Go to admin and check the number of users in the group of the previously deleted user

How it can be fixed
A joint query on the users and user_meta tables. This not only fixes the problem, but is the more consistent way of doing things - the query used for the list of users uses a joint query too.

Performance implications
There might be some performance implications - the joint query will be slower, but not by much - it's done on a index column (user_id). It shouldn't have an impact, but someone with more knowledge on the matter should comment.

Patch
[]

Attachments (10)

user_count.patch (722 bytes) - added by psoluch 5 years ago.
Creates a joint query for users and user_meta tables in count_users() function
Users_‹_MyStory_me_—_WordPress.jpg (145.6 KB) - added by psoluch 5 years ago.
Screenshot of a page with incorrect count
user_count_normalsite_multisite.patch (697 bytes) - added by dots 3 years ago.
Modified the select user query to fetch the proper user count in simple and the multisite network. Tested in simple and multisite network and it's working fine.
user_count.2.patch (741 bytes) - added by dots 3 years ago.
Modified the select user query to fetch the proper user count and based on the blogged prefix select table in simple and the multisite network. Tested in simple and multisite network and it's working fine.
user_count_normalsite_multisite.2.patch (727 bytes) - added by dots 3 years ago.
Tested in #4
phpunit_test_countUsers.patch (1.2 KB) - added by dots 3 years ago.
Added the phpunit test patch
29785.diff (796 bytes) - added by boonebgorges 3 years ago.
29785.2.diff (796 bytes) - added by boonebgorges 3 years ago.
29785.3.diff (2.4 KB) - added by ptbello 3 years ago.
added fix and test also for when 'memory' == $strategy
29785.4.diff (2.0 KB) - added by johnbillion 3 years ago.

Download all attachments as: .zip

Change History (31)

@psoluch
5 years ago

Creates a joint query for users and user_meta tables in count_users() function

@psoluch
5 years ago

Screenshot of a page with incorrect count

#1 @OriginalEXE
5 years ago

  • Keywords has-patch 2nd-opinion added

#2 @obenland
4 years ago

  • Keywords dev-feedback added; 2nd-opinion removed

@boonebgorges, what do you think?

#3 @wonderboymusic
4 years ago

  • Owner set to boonebgorges
  • Status changed from new to assigned

#4 @boonebgorges
3 years ago

  • Focuses performance added
  • Keywords needs-patch needs-unit-tests good-first-bug added; has-patch dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

Performance is one concern here, though I guess accuracy is more important if the performance difference is not huge.

A simple join against the user table is probably not enough. Deleted and spam users should not show up in the count. Neither should non-activated users. We may need a different query for is_multisite() to handle these cases. Unit tests would be helpful.

@dots
3 years ago

Modified the select user query to fetch the proper user count in simple and the multisite network. Tested in simple and multisite network and it's working fine.

#5 @aryamaaru
3 years ago

Hi,
Actually I have tried by using right join instead of join in the query and i have also used sub-query but performance wise use of join is perfect.And patch working fine.

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


3 years ago

@dots
3 years ago

Modified the select user query to fetch the proper user count and based on the blogged prefix select table in simple and the multisite network. Tested in simple and multisite network and it's working fine.

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


3 years ago

#8 @dots
3 years ago

  • Keywords has-unit-tests has-patch added; needs-patch needs-unit-tests removed

I have modified the query and tested all the cases mentioned by @boonebgorges in #4 and I have made the PHP unit test and below are the results from the PHP unit test

C:\wamp\www\wordpress-core>phpunit tests/phpunit/tests/user/countUsers.php
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 4.1.6 by Sebastian Bergmann.

Configuration read from C:\wamp\www\wordpress-core\phpunit.xml.dist

..←[36;1mS←[0m←[36;1mS←[0m..

You should really fix these slow tests (>150ms)...
 1. 598ms to run Tests_User_CountUsers:test_count_users_is_accurate with data se
t #0
 2. 373ms to run Tests_User_CountUsers:test_count_users_is_accurate with data se
t #1
 3. 187ms to run Tests_User_CountUsers:test_count_users_is_accurate_with_multipl
e_roles with data set #0


Time: 41.02 seconds, Memory: 30.50MB

←[30;43mOK, but incomplete, skipped, or risky tests!←[0m
←[30;43mTests: 6, Assertions: 10, Skipped: 2.       ←[0m

C:\wamp\www\wordpress-core>

@dots
3 years ago

Added the phpunit test patch

#9 @dots
3 years ago

Hi All,

I have added the current test patch which will test the result of this bug. But I am getting the results in Failure. If anyone helps me what exactly I have to do in this file in PHPUnit testing.

Below are the results which I got.

C:\wamp\www\wordpress-core>phpunit tests/phpunit/tests/user/countUsers.php
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
Not running external-oembed tests. To execute these, use --group external-oembed
.
PHPUnit 4.1.6 by Sebastian Bergmann.

Configuration read from C:\wamp\www\wordpress-core\phpunit.xml.dist

←[41;37mF←[0m←[41;37mF←[0m←[36;1mS←[0m←[36;1mS←[0m..

You should really fix these slow tests (>150ms)...
 1. 994ms to run Tests_User_CountUsers:test_count_users_is_accurate with data se
t #0
 2. 523ms to run Tests_User_CountUsers:test_count_users_is_accurate with data se
t #1


Time: 13.92 seconds, Memory: 27.75MB

There were 2 failures:

1) Tests_User_CountUsers::test_count_users_is_accurate with data set #0 ('time')

Failed asserting that 9 matches expected 8.

C:\wamp\www\wordpress-core\tests\phpunit\tests\user\countUsers.php:48

2) Tests_User_CountUsers::test_count_users_is_accurate with data set #1 ('memory
')
Failed asserting that 9 matches expected 8.

C:\wamp\www\wordpress-core\tests\phpunit\tests\user\countUsers.php:48
←[37;41m                                                 ←[0m
←[37;41mFAILURES!                                        ←[0m
←[37;41mTests: 6, Assertions: 8, Failures: 2, Skipped: 2.←[0m

C:\wamp\www\wordpress-core>

Thanks in Advance :)

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


3 years ago

@boonebgorges
3 years ago

@boonebgorges
3 years ago

#11 @boonebgorges
3 years ago

Hi @dots - Thanks for working on this ticket!

Unless there's a reason to believe that the existing tests are *incorrect*, it would be best to add some new tests for the specific bug we're trying to fix. 29785.2.diff is an example of what such a test might look like.

Regarding your proposed fix: Be sure to add a corresponding fix for the 'time' $strategy.

#12 @dots
3 years ago

Hi @boonebgorges - Thanks for the updates.

I have made the changes into the test file as per your 29758.2.diff and now I am getting below result in unit testing.

C:\wamp\www\wordpress-core>phpunit tests/phpunit/tests/user/countUsers.php
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
Not running external-oembed tests. To execute these, use --group external-oembed
.
PHPUnit 4.1.6 by Sebastian Bergmann.

Configuration read from C:\wamp\www\wordpress-core\phpunit.xml.dist

←[41;37mF←[0m←[41;37mF←[0m←[41;37mF←[0m←[36;1mS←[0m←[36;1mS←[0m..

You should really fix these slow tests (>150ms)...
 1. 405ms to run Tests_User_CountUsers:test_count_users_is_accurate with data se
t #0
 2. 400ms to run Tests_User_CountUsers:test_count_users_is_accurate with data se
t #1


Time: 6.41 seconds, Memory: 32.25MB

There were 3 failures:

1) Tests_User_CountUsers::test_count_users_is_accurate with data set #0 ('time')

Failed asserting that 9 matches expected 8.

C:\wamp\www\wordpress-core\tests\phpunit\tests\user\countUsers.php:48

2) Tests_User_CountUsers::test_count_users_is_accurate with data set #1 ('memory
')
Failed asserting that 9 matches expected 8.

C:\wamp\www\wordpress-core\tests\phpunit\tests\user\countUsers.php:48

3) Tests_User_CountUsers::test_should_not_count_users_who_are_not_in_posts_table

Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 1
+    0 => 2
     1 => Array (
         'none' => 0
         'administrator' => 1
+        'editor' => 1
     )
 )

C:\wamp\www\wordpress-core\tests\phpunit\includes\testcase.php:423
C:\wamp\www\wordpress-core\tests\phpunit\tests\user\countUsers.php:77
←[37;41m                                                 ←[0m
←[37;41mFAILURES!                                        ←[0m
←[37;41mTests: 7, Assertions: 9, Failures: 3, Skipped: 2.←[0m

C:\wamp\www\wordpress-core>

But still, I am confused here how exactly do we have to do unit testing on this issue.


Regarding your proposed fix: Be sure to add a corresponding fix for the 'time' $strategy.

Can you please explain more about your sentence?

I appreciate your help on this issue. Thanks in Advance :)

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


3 years ago

#14 @boonebgorges
3 years ago

  • Keywords needs-patch added; has-patch removed

Hi @dots -

But still, I am confused here how exactly do we have to do unit testing on this issue.

The tests are failing because the code is incorrect. That means that the tests are working properly :)

Regarding your proposed fix: Be sure to add a corresponding fix for the 'time' $strategy.

Can you please explain more about your sentence?

count_users() does two different kinds of query, depending on the value of the $strategy parameter. See https://core.trac.wordpress.org/browser/tags/4.6.1/src/wp-includes/user.php?marks=845,876#L837 Your proposed fix only addresses the "else" part. This also explains why the tests above are failing, because they use the default value for $strategy, which is 'time'.

@ptbello
3 years ago

added fix and test also for when 'memory' == $strategy

#15 @ptbello
3 years ago

  • Keywords has-patch added; needs-patch removed

Hi @boonebgorges, I added the query fix also for when 'memory' == $strategy as you suggested.
29785.3.diff also contains the tests for both 'time' and 'memory'.
(thanks @johnbillion)

@johnbillion
3 years ago

#16 follow-up: @johnbillion
3 years ago

  • Keywords needs-testing added

29785.4.diff improves the tests by implementing the existing data_count_users_strategies data provider.

This patch looks good, but as @psoluch mentioned it will need performance testing (before and after) with a large number of users in place so the effect on performance can be seen. The wp_users.ID field is indexed, and by its nature the wp_users table contains much fewer rows than wp_usermeta so its affect should only be slight.

#17 in reply to: ↑ 16 @ptbello
3 years ago

Replying to johnbillion:

it will need performance testing (before and after) with a large number of users in place so the effect on performance can be seen.

How do we move forward - is there a standard procedure for performance testing?

Last edited 3 years ago by ptbello (previous) (diff)

#18 @cahara
3 years ago

Is there any update on this patch?

#19 @tharsheblows
3 years ago

I'm not sure on performance testing but here is cheap and cheerful look at the MYSQL query performance for 184k users and user meta table with 3.2 million rows (on average over 17 meta entries per user <sob>).

count_users( 'memory' ) is fine with patch.

#no patch: 0.0813s  
SELECT meta_value FROM wp_usermeta WHERE meta_key = 'wp_capabilities'

#with patch: 0.0803s 
SELECT meta_value FROM wp_usermeta JOIN wp_users ON user_id = ID WHERE meta_key = 'wp_capabilities'

count_users( 'time' ) has two factors which increase query time:

  • number of users
  • number of roles

The patch increases the query time substantially for me (~0.6s) when using count_users( 'time' ) with the standard WP roles only and more with an increased number of roles.

count_users() is a problem for me already, increasing the query time would hurt.

Here's what I ran, as always please let me know any errors:

#default WP roles without patch: 1.497s 
SELECT 
COUNT(NULLIF(`meta_value` LIKE '%\"administrator\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"editor\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"author\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"contributor\"%', false)),
COUNT(NULLIF(`meta_value` LIKE '%\"subscriber\"%', false)), 
COUNT(NULLIF(`meta_value` = 'a:0:{}', false)), 
COUNT(*) 
FROM wp_usermeta 
WHERE meta_key = 'wp_capabilities'

#default WP roles with patch: 2.100s 
SELECT 
COUNT(NULLIF(`meta_value` LIKE '%\"administrator\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"editor\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"author\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"contributor\"%', false)),
COUNT(NULLIF(`meta_value` LIKE '%\"subscriber\"%', false)), 
COUNT(NULLIF(`meta_value` = 'a:0:{}', false)), 
COUNT(*) 
FROM wp_usermeta 
JOIN wp_users 
ON user_id = ID 
WHERE meta_key = 'wp_capabilities'

#including bbPress roles without patch: 2.289s
SELECT 
COUNT(NULLIF(`meta_value` LIKE '%\"administrator\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"editor\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"author\"%', false)),
COUNT(NULLIF(`meta_value` LIKE '%\"contributor\"%', false)),
COUNT(NULLIF(`meta_value` LIKE '%\"subscriber\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"bbp\\_keymaster\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"bbp\\_moderator\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"bbp\\_participant\"%',false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"bbp\\_spectator\"%', false)),
COUNT(NULLIF(`meta_value` LIKE '%\"bbp\\_blocked\"%', false)), 
COUNT(NULLIF(`meta_value` = 'a:0:{}', false)), 
COUNT(*) 
FROM wp_usermeta 
WHERE meta_key = 'wp_capabilities'

#including bbPress roles with patch: 2.945s 
SELECT 
COUNT(NULLIF(`meta_value` LIKE '%\"administrator\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"editor\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"author\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"contributor\"%', false)),
COUNT(NULLIF(`meta_value` LIKE '%\"subscriber\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"bbp\\_keymaster\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"bbp\\_spectator\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"bbp\\_blocked\"%', false)),
COUNT(NULLIF(`meta_value` LIKE '%\"bbp\\_moderator\"%', false)), 
COUNT(NULLIF(`meta_value` LIKE '%\"bbp\\_participant\"%', false)), 
COUNT(NULLIF(`meta_value` = 'a:0:{}', false)), 
COUNT(*) 
FROM wp_usermeta 
JOIN wp_users 
ON user_id = ID 
WHERE meta_key ='wp_capabilities'

#20 @johnbillion
3 years ago

  • Focuses administration added; performance removed
  • Keywords good-first-bug removed
  • Milestone changed from Future Release to 4.8

#21 @johnbillion
3 years ago

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

In 40560:

Users: Ensure user counts remain accurate if users are added to or removed from the users table without corresponding usermeta entries being added or removed.

This has a slight performance impact on sites with a large number of users when the time strategy is used for counting users. Hopefully this impact will be negated by enhancements proposed in #38741.

Props psoluch, dots, boonebgorges, ptbello, tharsheblows

Fixes #29785

Note: See TracTickets for help on using tickets.