Make WordPress Core

Opened 7 years ago

Last modified 7 years ago

#39926 new defect (bug)

wp_get_object_terms should return WP_Error on wrong fields argument or use a sane default

Reported by: bjornw's profile BjornW Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch dev-feedback
Focuses: Cc:

Description

wp_get_object_terms( $object_ids, $taxonomies, $args ) accepts object_ids, taxonomies and as last option extra arguments as an array. One of the extra arguments in the $args array is the fields option.

I used the field value 'term_id' (erroneously) assuming this would return the term_ids. However this did not work. wp_get_object_terms returned an empty array and in my error log I noticed this SQL error message:

WordPress database error You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'FROM wp_terms AS t  INNER JOIN wp_term_taxonomy AS tt ON t.term_id = tt.term_id ' at line 1 for query SELECT   FROM wp_terms AS t  INNER JOIN wp_term_taxonomy AS tt ON t.term_id = tt.term_id INNER JOIN wp_term_relationships AS tr ON tr.term_taxonomy_id = tt.term_taxonomy_id WHERE tt.taxonomy IN ('product') AND tr.object_id IN (449, 427) ORDER BY t.name ASC

After consulting the source of the wp_get_object_terms I noticed that 'term_id' is not a valid fields value. However I would have expected wp_get_object_terms() to return a WP_Error object instead of creating erroneous SQL code.

The following are valid field option values:

  • all - Default : all matching term's objects will be returned
  • ids : term's ids will be returned
  • names : term's names will be returned
  • slugs : term's slugs will be returned
  • all_with_object_id : all matching term's objects will be returned
  • tt_ids : term's taxonomy's ids will be returned

See wp_get_object_terms() docs on the Codex

Proposed solution

Due to this switch statement invalid or an empty fields value will result in an empty SELECT SQL query in this codeblock

My proposal is to add a default clause to the switch statement which defaults to using the 'all' case value.

This seems to adhere to the functionality as well as the spirit of the rest of the WP_Term_Query class code. As far as I know it should be backwards compatible as well.

Attachments (2)

fix-39926.diff (466 bytes) - added by BjornW 7 years ago.
Add default to switch to prevent empty SQL SELECT statements
fix-39926-2.diff (497 bytes) - added by BjornW 7 years ago.
Set the fields variable to 'all' for further use

Download all attachments as: .zip

Change History (7)

@BjornW
7 years ago

Add default to switch to prevent empty SQL SELECT statements

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


7 years ago

#2 follow-up: @dd32
7 years ago

I'm actually OK with the SQL error being generated in this case - it's a developer error which they'll otherwise get no notification of, garbage in = garbage out type thing.

The alternative here would be for the default in the switch statement to simply return array(); but that would then hide the source of the error and cause more debugging.
Having it default to all doesn't seem that great at first, mostly because it's not what the developer has requested - and they'd get no feedback about why it's happening, however, they'd get back a object which should be pretty obviously not what they requested in that scenario, and should highlight to the developer they've passed something invalid.

If fix-39926.diff is the route taken, the default: should be added to the all branch of the switch (not duplicating it by itself), and it could probably set $args['fields'] to 'all' for later use?

#3 in reply to: ↑ 2 @BjornW
7 years ago

  • Keywords has-patch dev-feedback added

Replying to dd32:

I'm actually OK with the SQL error being generated in this case - it's a developer error which they'll otherwise get no notification of, garbage in = garbage out type thing.

I agree with the sentiment of 'garbage in, garbage out'. My initial approach towards solving this was to send an explicit error back instead of letting the code continue executing and generating invalid SQL. However a default fall-back seems more elegant and still conveys the message towards the developer that something is not right. Happy to hear you agree on this approach :)

If fix-39926.diff is the route taken, the default: should be added to the all branch of the switch (not duplicating it by itself), and it could probably set $args['fields'] to 'all' for later use?

I see the appeal of not duplicating lines of code, however mixing explicit valid field values with a default 'catch all' does not seem right to me. The 'default' in the switch is now explicitly used as a catch all, as you'd expect it to do. That's why I opted for adding a default to the switch instead of combining it. What do you think about this reasoning?

I've updated my patch to include settings the fields value to 'all' to make sure any further use of the variable will continue as intended.

ps: Thanks for your quick review & feedback!

@BjornW
7 years ago

Set the fields variable to 'all' for further use

#4 @dd32
7 years ago

I see the appeal of not duplicating lines of code, however mixing explicit valid field values with a default 'catch all' does not seem right to me. The 'default' in the switch is now explicitly used as a catch all, as you'd expect it to do. That's why I opted for adding a default to the switch instead of combining it. What do you think about this reasoning?

I still disagree that a duplicated block is the correct way there, perhaps a more elegant way would be:

switch( field ) { 
   default:
       // default to 'all'
       field = 'all';
       // explicit fall through
  case 'all':
       // ....
}

duplicating the branch doesn't help future maintainers who change the all branch and miss the default - nor does it convey to the reader that it's actually the all case that it's defaulting to.
Although I suggested that it should maybe update $args['fields'] I don't necessarily believe it should - in some cases it may be needed, I don't know if this is one of those places (I haven't looked close enough)

#5 @swissspidy
7 years ago

  • Version trunk deleted
Note: See TracTickets for help on using tickets.