Make WordPress Core

Opened 6 years ago

Last modified 2 weeks ago

#49799 new defect (bug)

get_the_terms() object term cache check too strict?

Reported by: dingo_d Owned by:
Priority: normal Milestone: Awaiting Review
Component: Taxonomy Version: 2.3
Severity: normal Keywords: reporter-feedback has-patch has-unit-tests
Cc: Focuses:

Description

I ran into a weird issue when doing integration tests on one of a client's project.

I was setting up custom post and custom taxonomy from the factory (for posts I used the default one, and for custom taxonomy I have created my own factory that extends WP_UnitTest_Factory_For_Term), set everything up, but the part of the plugins code that used get_the_terms returned nothing. So after racking my brains, I tried wp_get_post_terms, and lo and behold, this worked (the test posts had assigned terms for them, I checked before invoking my function I needed to test using get_the_terms).

So something was up with caching. I started to dig and it turned out that

$terms = get_object_term_cache( $post->ID, $taxonomy );

inside wp-includes/category-template.php

returned an empty array.

Now, since the line 1246 says

if ( false === $terms ) {

The empty array, while being a falsy value, it's not a false value (https://3v4l.org/RVuG7), so the above check will return false and the returned value from the get_the_terms function will be false.

So when I 'loosened' the condition in my test suite WP, the $terms = wp_get_object_terms( $post->ID, $taxonomy );
works, and I get my terms inside the tests.

If the condition is loosened, nothing seems to be lost in terms of checking the emptiness of the terms from the term object cache (if it's full, it's false all the times https://3v4l.org/34UQk).

Now, on the face value, this could be the oddity of the test suite (which in my case is set up using wpcli scaffold command), but I'm thinking that changing this requirement cannot hurt (from what I can tell) in the long run.

Attachments (1)

49799.diff (546 bytes ) - added by dingo_d 6 years ago.
Patch with the fix

Download all attachments as: .zip

Change History (9)

This ticket was mentioned in PR #211 on WordPress/wordpress-develop by dingo-d.


6 years ago
#1

Loosened the condition to check the terms fetched fro mthe object term cache. The cache can return an empty array, which is a falsy value, so the get_the_terms will return false value.

Trac ticket: https://core.trac.wordpress.org/ticket/49799

@dingo_d
6 years ago

Patch with the fix

dingo-d commented on PR #211:


6 years ago
#2

Also, maybe a good thing to note is that I'm assigning the terms in my tests using wp_set_object_terms(); function. The same thing happened when I tried wp_set_post_terms(), so it could be something related to that (test env could mess with the object caching?)

This ticket was mentioned in Slack in #themereview by dingo_d. View the logs.


6 years ago

#4 follow-up: @joyously
6 years ago

Is this another fallout from #48965 (similar to #49853)?

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


6 years ago

#6 in reply to: ↑ 4 @SergeyBiryukov
6 years ago

  • Keywords reporter-feedback needs-unit-tests added; dev-feedback 2nd-opinion has-patch removed
  • Version 5.42.3

Thanks for the ticket!

Replying to joyously:

Is this another fallout from #48965 (similar to #49853)?

Doesn't seem like that. This line was added in [5598] / #4189 for WordPress 2.3, moved to the newly created get_the_terms() function in [7520] / #6357, and has not changed much since then.

$terms = get_object_term_cache( $post_id, $taxonomy );

if ( false === $terms ) {
	$terms = wp_get_object_terms( $post_id, $taxonomy, $args );
	...
}

This pattern is not specific to get_the_terms(), it's used in quite a few other places in core:

  • get_attachment_fields_to_edit()
  • get_compat_media_markup()
  • get_terms_to_edit()
  • get_inline_data()
  • wp_queue_posts_for_term_meta_lazyload()
  • get_the_taxonomies()
  • is_object_in_term()

So while there indeed might be an issue here, this doesn't appear to be a regression in 5.4.

The false === ... check is specifically used to ensure the cache is empty. Making the check less strict would go against the coding standards and doesn't seem like the correct way to fix this.

Instead, a check for an empty array should be added if necessary. Before making any changes though, it would be helpful to have some unit tests to reproduce the issue.

This ticket was mentioned in PR #13226 on WordPress/wordpress-develop by @sainathpoojary.


2 weeks ago
#7

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

The ticket reports that get_the_terms() returns false instead of the correct terms when get_object_term_cache() returns an empty array.

Investigation shows the false === $terms check is correct. It distinguishes a cache miss from a valid "no terms" state ([]). When terms are assigned, wp_set_object_terms() properly clears the cache so get_the_terms() will query the database on the next call. The reported bug is likely a test environment configuration issue.

As requested, this PR adds unit tests to verify the expected cache behavior:

  1. Cache primed with [] before terms assigned → terms assigned → returns correct terms.
  2. Cache primed with [] for a post with no terms → returns false without a DB query.
  3. Cache primed after term assignment → returns correct terms from cache.

All tests pass on current trunk with no source change needed.

Trac ticket: https://core.trac.wordpress.org/ticket/49799

## Use of AI Tools

AI assistance: Yes
Tool(s): Github Copilot
Used for: Investigating the cache flow and generating initial unit tests. Implementation and testing were reviewed and edited by me.

#8 @sainathpoojary
2 weeks ago

I investigated this issue and found the false === $terms check is correct. It intentionally distinguishes a cache miss (false) from a cached "no terms" state ([]).
wp_set_object_terms() correctly clears the cache using wp_cache_delete(), so get_the_terms() will always do a fresh DB query after terms are assigned. This appears to be a test environment issue rather than a Core bug.
As requested in comment 6, I've added unit tests covering the cache behavior:

  1. Cache primed with [] before terms are assigned → terms assigned → get_the_terms() returns the correct terms.
  2. Cache primed with [] for a post with no terms → get_the_terms() returns false without a DB query.
  3. Cache primed after term assignment → get_the_terms() serves the correct terms from cache.

All tests pass on current trunk with no source change needed.

Note: See TracTickets for help on using tickets.