WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 8 months ago

#36949 new enhancement

Term exists should use get_terms internally

Reported by: spacedmonkey Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.3
Component: Taxonomy Keywords: has-patch
Focuses: performance Cc:

Description

More than WP_Term_Query has been merged, it makes sense to converge to use it in all internal functions to query the terms table. Using WP_Term_Query / get_terms will also add caching to a functions that do not currently have it.

Attachments (3)

36949.patch (3.7 KB) - added by spacedmonkey 19 months ago.
36949.2.patch (5.2 KB) - added by spacedmonkey 8 months ago.
36949.3.patch (5.2 KB) - added by boonebgorges 8 months ago.

Download all attachments as: .zip

Change History (8)

#1 @spacedmonkey
19 months ago

  • Keywords has-patch needs-unit-tests added

#2 @boonebgorges
19 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to Future Release

Thanks. This is a good idea in theory, though I'm not sure how it will pan out in practice. First, there are errors in the patch that are causing invalid SQL in some cases. Second, relying on the cache in term_exists() is likely to surface invalidation bugs in places where WordPress uses term_exists() internally to avoid duplicates. There needs to be a review of these places. A good place to start is the dozens of failing tests when running phpunit with the patch applied :)

#3 @spacedmonkey
8 months ago

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

Updated the patch so it now passes all but three of the unit tests. All the taxonomy tests pass. The existing tests for term_exists are pretty solid, so not sure how many more tests are needed.

#4 @boonebgorges
8 months ago

36949.3.patch includes some formatting cleanup.

I looked into one of the failures - Tests_Import_Import::test_small_import(). What's happening here is that the importer suspends cache validation, such that parent-term lookups fail during bulk term import. So here's a real-life instance of the kind of case I hypothesized above. I assume the other failures are similar. We have a couple options available.

  1. Don't suspend cache invalidation for bulk operations.
  2. Don't use term_exists() during bulk operations. (We'd use some version that skips the cache.)
  3. Don't change the behavior of term_exists()

It's fairly easy to make the internal changes necessary for 1 and 2, but both are going to cause serious problems for third-party libraries that suspend cache invalidation for imports and other bulk operations.

I think term_exists() is not a great function anyway, and I wouldn't mind if we moved away from its use in core. If we phased out our own internal uses, we could replace them with functions that leverage the cache, and also demonstrate the proper way to handle bulk operations (perhaps those would continue to use term_exists()?).

#5 @boonebgorges
8 months ago

Thinking about this a little more. The problem with imports etc is that cache invalidation is being skipped, but the cache is still being accessed for lookups. What we ought to have is something like wp_suspend_cache_access() (bad name, but you get the idea), which would prevent the cache from being accessed for lookups in places like term_exists() and get_term_by().

Note: See TracTickets for help on using tickets.