Skip to:
Content

Opened 3 years ago

Closed 3 years ago

Last modified 15 months ago

#6012 closed defect (bug) (fixed)

Cache: Invalid usage of `! $count`

Reported by: r-a-y Owned by: r-a-y
Milestone: 2.2 Priority: normal
Severity: normal Version: 1.2
Component: Performance Keywords: has-patch commit
Cc:

Description

In some of our object cache calls (see r2618), we do this:

if ( ! $count = wp_cache_get( 'WHATEVER', 'bp' ) {
	$count = FUNCTION_TO_FETCH_COUNT_VIA_DB();
	wp_cache_set( 'WHATEVER', $count, 'bp' );
}

If $count is zero, then the check passes due to the liberal ! condition and will go about hitting the DB again.

Attached patch fixes this.

Attachments (3)

6012.01.patch (2.1 KB) - added by r-a-y 3 years ago.
6012.unit-tests.patch (1.5 KB) - added by r-a-y 3 years ago.
6012.unit-tests.2.patch (2.4 KB) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (15)

@r-a-y
3 years ago

#1 @boonebgorges
3 years ago

Unit tests would be swell (maybe count $wpdb->num_queries before and after count requests that return 0) but this looks like a good change. +1

#2 @DJPaul
3 years ago

Yes looks fine, and is the way it should have been done originally. I am less concerned about unit tests for this than Boone probably is; either way, let's get the fix in. :)

#3 @DJPaul
3 years ago

  • Keywords commit added

#4 @DJPaul
3 years ago

Boone: do you want unit tests before we change these? I was about to commit it as we've been "doing it wrong".

#5 @r-a-y
3 years ago

I've added unit tests for groups and blogs in unit-tests.patch.

Unit tests fail without 01.patch and pass with 01.patch applied.

Note: I didn't add a test for members because there is always a member created during unit tests.

#6 @boonebgorges
3 years ago

6012.unit-tests.2.patch adds a test for the members function. Since the main purpose of the test is to check whether we're respecting the cached value, we can just force that value, even if it's not accurate. Could also do the same for the other tests, but it doesn't matter too much (those tests effectively just cast a slightly broader net). Feel free to change the group names to be consistent.

#7 @johnjamesjacoby
3 years ago

Patches look good. Agree with Paul that tests here aren't blockers.

#8 @boonebgorges
3 years ago

Agree with Paul that tests here aren't blockers.

:weeping_in_corner:

This ticket was mentioned in Slack in #buddypress by jjj. View the logs.


3 years ago

#10 @r-a-y
3 years ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

In 9231:

Check cache for count functions that return zero.

Some of our count functions were previously setting counts of zero
correctly, but the functions themselves were not referencing the cache
properly when the count returned zero. This led to unnecessary database
queries and we hate extra queries!

This commit addresses the problem and adds unit tests.

Props r-a-y, boonebgorges.

Fixes #6012.

#11 @johnjamesjacoby
3 years ago

  • Component changed from Component - Any/All to API - Cache

#12 @DJPaul
15 months ago

  • Component changed from API - Cache to Performance
Note: See TracTickets for help on using tickets.