User count not correct

[psoluch]

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
[]

[psoluch]

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

[psoluch]

Screenshot of a page with incorrect count

[obenland]

@boonebgorges, what do you think?

[boonebgorges]

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]

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.

[aryamaaru]

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.

[dots]

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.

[dots]

Tested in #4

[dots]

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]

Added the phpunit test patch

[dots]

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 :)

[boonebgorges]

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.

[dots]

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 :)

[boonebgorges]

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]

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

[ptbello]

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]

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.

[ptbello]

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?

[cahara]

Is there any update on this patch?

[tharsheblows]

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'

[johnbillion]

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