Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#32876 closed defect (bug) (fixed)

Wrong data type for term taxonomy ID variable

Reported by: ipm-frommen's profile ipm-frommen Owned by: boonebgorges's profile boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version: 2.3
Component: Taxonomy Keywords: has-patch commit
Focuses: Cc:


In wp_update_term(), there is a variable $tt_id that will be passed to looots of functions, and every single function is expecting an int here. Due to $wpdb->get_var(), however, the variable is of type string.

I'm pretty sure there are several other similar incidents, though.

Attachments (2)

32876.patch (1016 bytes) - added by ipm-frommen 9 years ago.
32876.2.patch (1.9 KB) - added by boonebgorges 9 years ago.

Download all attachments as: .zip

Change History (9)

9 years ago

#1 @ipm-frommen
9 years ago

  • Keywords has-patch added

#2 @johnbillion
9 years ago

  • Keywords needs-testing 2nd-opinion added
  • Version set to 2.3

There is a chance of some breakage here, mainly around plugins which might expect the term_taxonomy_id element in the return value of wp_update_term() to be a numerical string (ie. it might break some code which is subsequently performing a strict check).

#3 @ipm-frommen
9 years ago

@johnbillion that's right. If someone expects the value to be a numerical string and performs strict checks, this might lead to unexpected results. However, as the value/type is told (as per PHPDoc) to be an int (all around the core where it is being used, that is), I would consider casting the value to int the right choice (and not changing all the PHPDoc blocks).

And by the way, a not-working strict check is exactly what made me create this ticket. ;)

#4 @ipm-frommen
9 years ago

What's the status here?

In my opinion, you just have to decide which way to go. Either change the PHPDoc comment to indicate $ttid being a string (which is the wrong decision, if you ask me), or apply the patch (and thus cast the string to an int).

#5 @boonebgorges
9 years ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Awaiting Review to 4.4

I've just looked through a couple dozen plugins in the plugin repository that look at the return value of wp_update_term() ($ ack "\= *wp_update_term"). I found zero plugins that would be affected by the change. In fact, I found zero plugins that looked at the 'term_taxonomy_id' value *at all* - the vast majority of plugins are checking is_wp_error(), and a few look at 'term_id'. I say we make the change. 32876.2.patch contains a unit test. johnbillion, you have strong feelings about it?

#6 @johnbillion
9 years ago

  • Keywords commit added; needs-testing removed

Okay, I'm happy with that change in that case.

#7 @boonebgorges
9 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 33652:

wp_update_term() should return a true integer for 'term_taxonomy_id'.

Props ipm-frommen.
Fixes #32876.

Note: See TracTickets for help on using tickets.