Make WordPress Core

Opened 13 years ago

Closed 10 years ago

Last modified 10 years ago

#17646 closed defect (bug) (fixed)

wp_get_object_terms should return arrays of integers for IDs and tt_IDs

Reported by: simonwheatley's profile simonwheatley Owned by: wonderboymusic's profile wonderboymusic
Milestone: 3.8 Priority: normal
Severity: normal Version: 3.2
Component: Taxonomy Keywords: has-patch early
Focuses: Cc:

Description

Currently when you use wp_get_object_terms to request an array of IDs or tt_IDs, the array returned contains strings representing the IDs, not integers. This then creates issues if you send the values back into wp_set_object_terms as it creates terms named as per those strings, rather than associating the term_ids as expected.

Code to prove the issue:

$term_ids = wp_get_object_terms( get_the_ID(), 'category', array( 'fields' => 'tt_ids' ) );
var_dump( $term_ids );

Results in:

array
  0 => string '1' (length=1)
  1 => string '5' (length=1)

I believe that the array should have all values cast to integers before they are returned. The attached patch does this by mapping a created function to utilise (int) for the desired result.

Attachments (6)

cast_to_int.diff (693 bytes) - added by simonwheatley 13 years ago.
Cast the values of the array to int if the fields are ids or tt_ids
17646.diff (3.1 KB) - added by dd32 13 years ago.
17646.2.diff (2.7 KB) - added by dd32 13 years ago.
patch refresh; Exclude cruft from around the changes. Fix typo in previous patch
17646.sw.2.diff (2.6 KB) - added by simonwheatley 12 years ago.
Cleaned up DD32's patch so it works on the current trunk
17646.3.diff (2.7 KB) - added by kovshenin 11 years ago.
17646.tests.diff (1.1 KB) - added by kovshenin 11 years ago.

Download all attachments as: .zip

Change History (29)

@simonwheatley
13 years ago

Cast the values of the array to int if the fields are ids or tt_ids

#1 @nacin
13 years ago

Sounds like a very good idea. Also probably a duplicate ticket somewhere. Also, we can use absint rather than an anonymous function (which, also, we're no longer allowing in core).

#2 @scribu
13 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

A good idea indeed.

#3 @dd32
13 years ago

IMO, sanitize_term_field() should be updated to absint() the term_id field. wp_get_object_terms() should call the sanitization functions. This would also take care of tickets such as #17652

#4 @dd32
13 years ago

IMO, sanitize_term_field() should be updated to absint() the term_id field.

Completely missed the fact that it already (int)'s the term_id value

@dd32
13 years ago

#5 @dd32
13 years ago

attachment 17646.diff added

  • Not a clean patch, Just a diff showing my thinking
  • Utilises sanitize_term() / sanitize_term_field() for sanitizing values, ie. int'ing fields
  • causes get_term(), get_term_by(), wp_get_object_terms() and most other high level taxonomy functions to return int'd values as expected (no point fixing one function and leaving the rest) - There are other tickets for these I believe.

Other functions that don't int the results:

  • get_objects_in_term() - object_id
  • class WP_Tax_Query - Doesn't int term_ids, or sanitize them in any way, just returns the table results into WP_Query
  • term_exists() - term_id, term_taxonomy_id as strings
  • wp_set_object_terms() term_taxonomy_ids as strings

Example output:

$res = get_term(1, 'category');
var_dump($res);

object(stdClass)[648]
  public 'term_id' => int 1
  public 'name' => string 'Uncategorized' (length=13)
  public 'slug' => string 'uncategorized' (length=13)
  public 'term_group' => int 0
  public 'term_taxonomy_id' => int 1
  public 'taxonomy' => string 'category' (length=8)
  public 'description' => string '' (length=0)
  public 'parent' => int 0
  public 'count' => int 6
  public 'filter' => string 'raw' (length=3)

$term_ids = wp_get_object_terms( 1, 'category', array( 'fields' => 'ids' ) );
var_dump( $term_ids );

array
  0 => int 1

#6 @dd32
13 years ago

I should mention, that the patch reverts [6321] (part of #5325) as can be seen by the commented out code, sanitize_term_field was written to short-circuit itself after int'ing resulting in that addition not helping as much as it would've for posts, also resulting in string fields rather than int fields in more places.

#7 @scribu
13 years ago

  • Keywords 3.3-early added
  • Milestone changed from Awaiting Review to Future Release

Leveraging sanitize_term_field() makes sense to me.

We should cover all the functions in the taxonomy API in the next release.

@dd32
13 years ago

patch refresh; Exclude cruft from around the changes. Fix typo in previous patch

#8 @jkudish
13 years ago

  • Keywords has-patch added; needs-patch removed

removed needs-patch and added has-patch

@simonwheatley
12 years ago

Cleaned up DD32's patch so it works on the current trunk

#9 @simonwheatley
12 years ago

Tested cleaned patch against trunk, integers are returned as expected for tt_ids and ids.

#10 @kurtpayne
11 years ago

  • Cc kurtpayne added
  • Milestone changed from Future Release to 3.6

Patch looks good for the original report. The object_id field is still coming through as a string. Test code:

var_dump( wp_get_object_terms( get_the_ID(), 'category', array( 'fields' => 'all' ) ) );
var_dump( wp_get_object_terms( get_the_ID(), 'category', array( 'fields' => 'all_with_object_id' ) ) );
var_dump( wp_get_object_terms( get_the_ID(), 'category', array( 'fields' => 'id' ) ) );
var_dump( wp_get_object_terms( get_the_ID(), 'category', array( 'fields' => 'tt_ids' ) ) );
Version 0, edited 11 years ago by kurtpayne (next)

#11 @kovshenin
11 years ago

#23894 was marked as a duplicate.

@kovshenin
11 years ago

#12 @kovshenin
11 years ago

Updated in 17646.3.diff, used the same $int_fields approach we use in other places, added object_id to the integer fields.

#13 @kovshenin
11 years ago

17646.tests.diff tests the integer fields for all_with_object_id and ids.

#14 @DrewAPicture
11 years ago

17646.3.diff works as expected for me using @kurtpayne's test dumps. Didn't run the unit tests.

#15 @nacin
11 years ago

  • Keywords punt added

I have a feeling that (despite this being a very good idea) this could cause problems along the lines of #22324. I don't think this should necessarily stop us, but now that we have unit tests and a patch, this sounds like something that should be landed on the first day of a cycle, not deep into beta.

#16 @SergeyBiryukov
11 years ago

  • Keywords 3.7-early added; 3.3-early punt removed
  • Milestone changed from 3.6 to Future Release

#17 @ryanduff
11 years ago

  • Cc ryan@… added

#18 @Mamaduka
11 years ago

WP_Query also returns stings representing post IDs, when fields is set to ids.

#19 @ericmann
11 years ago

#17652 was marked as a duplicate.

#20 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#21 @nacin
10 years ago

  • Keywords early added; 3.7-early removed
  • Milestone changed from 3.7 to 3.8

I'm holding this until early 3.8, day one.

#22 @wonderboymusic
10 years ago

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

In 26010:

Cast proper fields to int when returning from wp_get_object_terms(). Add term_taxonomy_id and object_id to the list in sanitize_term() and sanitize_term_field().

Fixes #17646. Adds unit tests.
Props simonwheatley, dd32, kovshenin.

Note: See TracTickets for help on using tickets.