WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#29848 closed defect (bug) (fixed)

Caching problems when using 'alias_of' in wp_update_term() and wp_insert_term()

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

See https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/taxonomy.php?annotate=blame#L2448

When creating a new term using wp_insert_term() and passing the alias_of parameter, the term_group of the aliased term is updated (see line 2467). However, the cache for this item is not cleared.

We should use wp_update_term() here instead of a direct database query. This takes care of caching for us. It also allows us to remove these duplicate 'edit_terms' and 'edited_terms' hooks.

Patch attached.

Attachments (3)

29848.patch (3.7 KB) - added by boonebgorges 6 years ago.
29848.2.patch (9.0 KB) - added by boonebgorges 6 years ago.
29848.3.patch (9.0 KB) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (13)

@boonebgorges
6 years ago

#1 @boonebgorges
6 years ago

  • Summary changed from Cache is not flushed term A when creating a new term with A as an alias to Cache is not flushed for term A when creating a new term with A as an alias

#2 @boonebgorges
6 years ago

  • Owner set to boonebgorges
  • Severity changed from minor to normal
  • Status changed from new to accepted
  • Summary changed from Cache is not flushed for term A when creating a new term with A as an alias to Caching problems when using 'alias_of' in wp_update_term() and wp_insert_term()

There are actually a few bugs here, so let's do something more comprehensive. 29848.2.patch does the following:

  • Give the same treatment to wp_insert_term() and wp_update_term()
  • Use get_term_by( 'slug' ) instead of a direct DB query. Better because it's prettier, it'll hit the cache, it'll hit relevant filters, etc
  • Use wp_update_term() instead of a direct database query, so that we purge the cache
  • Remove the now redundant 'edit_terms' and 'edited_terms' hooks - they appear only in wp_update_term()
  • Unit tests for using 'alias_of' in both functions

#3 @boonebgorges
6 years ago

Oh, almost forgot this important bullet point:

  • Fix a PHP notice and incorrect term_group assignment when the term referenced in the 'alias_of' param does not exist. Currently, if the direct query turns up a $null alias, a new term group is created and the primary term is added to it (though not the aliased term, since there is no aliased term)

#4 follow-up: @DrewAPicture
6 years ago

The only thing I'd be concerned about with removing the duplicate hooks is that it's possible somebody could be relying on that behavior. It's pretty edge and bordering on misuse, but worth considering.

#5 in reply to: ↑ 4 ; follow-up: @boonebgorges
6 years ago

Replying to DrewAPicture:

The only thing I'd be concerned about with removing the duplicate hooks is that it's possible somebody could be relying on that behavior. It's pretty edge and bordering on misuse, but worth considering.

The hooks still fire in 29848.2.patch - they just fire in wp_update_term(). I think they were added before and after the direct db calls in order to provide parity with wp_update_term(), but using the API function means that this duplication is no longer necessary.

#6 in reply to: ↑ 5 @DrewAPicture
6 years ago

Replying to boonebgorges:

Replying to DrewAPicture:

The only thing I'd be concerned about with removing the duplicate hooks is that it's possible somebody could be relying on that behavior. It's pretty edge and bordering on misuse, but worth considering.

The hooks still fire in 29848.2.patch - they just fire in wp_update_term(). I think they were added before and after the direct db calls in order to provide parity with wp_update_term(), but using the API function means that this duplication is no longer necessary.

Sorry if I wasn't clear. What I meant was that it's possible people have workarounds in place to handle the duplicate filters.

Also, the hook doc that was converted to a comment in wp-includes/taxonomy.php should use /* to open instead of /** so it isn't mistakenly picked up as a docblock in the parser. The same goes for the new comment above this line further down:

$term_group = $wpdb->get_var("SELECT MAX(term_group) FROM $wpdb->terms") + 1;

The [syntax] for multi-line comments is simply:

/*
 * first line
 * second line
 */

#7 follow-up: @boonebgorges
6 years ago

Sorry if I wasn't clear. What I meant was that it's possible people have workarounds in place to handle the duplicate filters.

Ah, yes. Like you say, this is very edge. And I'd assume that the "handling" would mainly involves "making sure that stuff isn't run twice", which will be moot after the duplicates are removed. I'm having a hard time thinking of a legitimate way to use the double hooks, but maybe the point is that those uses wouldn't be legitimate :)

The [syntax] for multi-line comments

My bad. Fixed in 29848.3.patch - at least for the multi-line comments. The hook documentation that I moved is unchanged; it begins with /**. I don't think I converted any hook docs to comments, but I might be misunderstanding your comment.

#8 in reply to: ↑ 7 @DrewAPicture
6 years ago

Replying to boonebgorges:

The [syntax] for multi-line comments

My bad. Fixed in 29848.3.patch - at least for the multi-line comments. The hook documentation that I moved is unchanged; it begins with /**. I don't think I converted any hook docs to comments, but I might be misunderstanding your comment.

It just appeared to be a "replacement" due to the changed-lines placement in the patch: http://cl.ly/image/3Q3R0y2C1d17 :-)

#9 @boonebgorges
6 years ago

Foiled again by the Trac algorithms!

#10 @boonebgorges
6 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 29862:

Improve 'alias_of' handling in wp_insert_term() and wp_update_term().

Using get_term_by() rather than direct SQL queries to fetch the alias term
fixes a number of issues:

  • Object cache for aliased term is properly cleared after update.
  • If the aliased term is in the object cache, it's served from there, saving a database query.
  • Duplicate 'edit_terms' and 'edited_terms' hooks can be removed.
  • Fix a PHP notice when the 'alias_of' term is not found.
  • Prevent the incorrect creation of a new term group for the primary term when the 'alias_of' term is not found.

Adds unit tests for 'alias_of' functionality in both functions.

Fixes #29848.

Note: See TracTickets for help on using tickets.