Make WordPress Core

Opened 11 years ago

Closed 10 years ago

#25711 closed defect (bug) (fixed)

The "{$taxonomy}_children" cache can become stale

Reported by: dd32's profile dd32 Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.8
Component: Taxonomy Keywords:
Focuses: Cc:


When you add multiple hierarchical terms on a single page load, "{$taxonomy}_children" can become stale as only the first call to clean_term_cache() will invalidate that cache.

On subsequent calls, it, along with the all_ids and get taxonomy caches will not be cleared, which leads to anything which relies upon hierarchical checks to fail.
Many things rely upon the hierarchical cache, retrieving children of terms being the primary one.

This can be traced to the static $cleaned added in [9102].

I've attached a basic unit test that covers "{$taxonomy}_children" via _get_term_hierarchy() in 25711.diff

Attachments (1)

25711.diff (1.8 KB) - added by dd32 11 years ago.

Download all attachments as: .zip

Change History (7)

11 years ago

#1 @nacin
11 years ago

Related ticket, referring to {$taxonomy}_relationships stale cache: #24189.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

10 years ago

#3 @wonderboymusic
10 years ago

  • Milestone changed from Awaiting Review to 3.9
  • Resolution set to fixed
  • Status changed from new to closed

Fixed via [27102].

#4 @bpetty
10 years ago

  • Keywords needs-unit-tests added
  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm not sure exactly what values should be tested for here, so I'm not going to attempt a patch, but this does need some eyes on the unit tests again at least.

1) Tests_Term_Cache::test_category_children_cache
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    75 => Array (...)


Here's the assertion:

$this->assertEquals( array( $term_id1 => array( $term_id1_child ) ), $hierarchy );

I've only see this fail on PHP 5.2, however, what I'd like to specifically point out is that this is comparing the result fetched from the factory, which should contain row IDs, and likely a bunch more data that is both irrelevant to this test, and is also volatile (can easily change between runs just like the ID does depending on previous tests run - try running full tests compared to just --group taxonomy).

This really needs to be rewritten with more more specific values used for assertion checks. Try to avoid using arrays, they can frequently fail when just out of order (but still remain functional and correct). In fact, don't even use the variable returned from the factory. Use an entirely explicit static array. We still need to know *exactly* what the test should be returning in case the test is failing (like it is here).

Last edited 10 years ago by bpetty (previous) (diff)

#6 @SergeyBiryukov
10 years ago

  • Keywords needs-unit-tests removed
  • Resolution set to fixed
  • Status changed from reopened to closed

The changes suggested in 25711.diff were committed in [27163] and [27103].

Unit test failure is not directly related to those commits, see ticket:14485:95. Let's fix it in #14485.

Note: See TracTickets for help on using tickets.