WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#16469 closed defect (bug) (invalid)

Cannot pass in ID for get_term_link() if using custom taxonomy

Reported by: sparkweb Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0.4
Component: Taxonomy Keywords: close
Focuses: Cc:

Description

Cannot pass in ID for get_term_link() if using custom taxonomy. Just throws errors. Additionally, if attempt to pass in a term name and that name legitimately includes a "/" the term link can't be found.

Attachments (1)

16469.diff (434 bytes) - added by garyc40 4 years ago.
use is_numeric() instead of is_int()

Download all attachments as: .zip

Change History (19)

comment:1 @nacin4 years ago

Version of WordPress?

comment:2 @dd324 years ago

  • Keywords reporter-feedback added

http://core.trac.wordpress.org/browser/trunk/wp-includes/taxonomy.php#L2820

Cannot pass in ID for get_term_link() if using custom taxonomy.

Are you passing an integer, or a string of the ID? Using (int)$term will retrieve a term by id, (string)$id will cause it to do a slug lookup instead, This is standard across the board for the taxonomy API. Can you confirm if this is the way you're calling get_term_link() ?

term name and that name legitimately includes a "/" the term link can't be found.

There's another ticket for this #16282, get_term_link() has never been designed to be passed a term name. Rather, a term slug. In 3.1 this is being caught a bit more, but it has always been a possibility through other versions.

comment:3 follow-up: @sparkweb4 years ago

  • Resolution set to invalid
  • Status changed from new to closed
  • Version set to 3.0.4

I was passing in $term->term_id. When I switched to (int)$term->term_id that worked. As did $term->slug which is much better than $term->name. Thank you and I'm sorry for the invalid ticket!

comment:4 @scribu4 years ago

  • Milestone Awaiting Review deleted

comment:5 in reply to: ↑ 3 @hakre4 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Replying to sparkweb:

I was passing in $term->term_id. When I switched to (int)$term->term_id that worked.

Thanks for the feedback, but I think $term->term_id should have been already casted to int, so normally for ID values there should be no need at all to cast them into integers prior further use.

Can you say where it originated from? Was the $term object returned by some API function? If yes please name it and provide some additional code so it's clear how you recieved it.

This still might be an issue with core.

comment:6 @scribu4 years ago

  • Milestone set to Awaiting Review

Might be.

FYI, you can just pass the whole object: get_term_link( $term );

Last edited 4 years ago by scribu (previous) (diff)

comment:7 @hakre4 years ago

  • Cc hanskrentel@… added

comment:8 @mikeschinkel4 years ago

  • Cc mikeschinkel@… added

+1. I have stumbled onto this problem where it fails when $term->term_id = '1'; but not when $term->term_id = 1; and I spent considerable time debugging to figure out why. It would IMO be nice it the term_id was cast to (int) inside the function.

comment:9 @scribu4 years ago

  • Keywords reporter-feedback removed

@garyc404 years ago

use is_numeric() instead of is_int()

comment:10 @garyc404 years ago

  • Keywords has-patch added

If you pass in a numeric string, is_int() still returns false, while is_numeric() returns true. So it's safer to use is_numeric() here.

I also found that is_int() is used in several other functions. Should we look into those functions as well so that confusion like this won't happen again?

comment:11 follow-ups: @dd324 years ago

If you pass in a numeric string, is_int() still returns false, while is_numeric() returns true. So it's safer to use is_numeric() here.

is_int != is_numeric in the Term API. Theres the possibility of having Numeric slugs.

The correct fix here, is to verify that ALL term objects being returned return the term_id as a integer, that'll fix most of these casting problems (ie. It may return a string '1' instead of (int)1)

If someone passes a (string)'1' in as the term_id, they're doing it wrong, OR an API call is returning the data in the incorrect format.

comment:12 in reply to: ↑ 11 @garyc404 years ago

Replying to dd32:

is_int != is_numeric in the Term API. Theres the possibility of having Numeric slugs.

The correct fix here, is to verify that ALL term objects being returned return the term_id as a integer, that'll fix most of these casting problems (ie. It may return a string '1' instead of (int)1)

If someone passes a (string)'1' in as the term_id, they're doing it wrong, OR an API call is returning the data in the incorrect format.

That makes sense. I didn't know term slug could be numeric.

comment:13 @scribu4 years ago

  • Keywords has-patch removed

Related: #14162

comment:14 in reply to: ↑ 11 ; follow-up: @mikeschinkel4 years ago

Replying to dd32:

If someone passes a (string)'1' in as the term_id, they're doing it wrong, OR an API call is returning the data in the incorrect format.

For the record, my example showing '1' was for simplicity of example; my use-cases have been being returned as a string from something else, like $_GET['term_id'].

The correct fix here, is to verify that ALL term objects being returned return the term_id as a integer, that'll fix most of these casting problems (ie. It may return a string '1' instead of (int)1)

WordPress can't force $_GET['term_id'] to return a numeric. Sure the developer can cast it as (int), but they have to know that it needs to be cast; hence the crux of the problem.

comment:15 in reply to: ↑ 14 ; follow-up: @dd324 years ago

Replying to mikeschinkel:

Replying to dd32:

If someone passes a (string)'1' in as the term_id, they're doing it wrong, OR an API call is returning the data in the incorrect format.

For the record, my example showing '1' was for simplicity of example; my use-cases have been being returned as a string from something else, like $_GET['term_id'].

The correct fix here, is to verify that ALL term objects being returned return the term_id as a integer, that'll fix most of these casting problems (ie. It may return a string '1' instead of (int)1)

WordPress can't force $_GET['term_id'] to return a numeric. Sure the developer can cast it as (int), but they have to know that it needs to be cast; hence the crux of the problem.

Well no, Because that's not a term object is it? It's your data, as a plugin developer it's your responsibility to pass the correct parameters to functions. The documentation *may* (I havn't read it) may need to stress the fact it requires a integer value to be passed, not a numeric string.

Upon reading some of the documentation:

  • get_term(): * @param int|object $term If integer, will get from database. If object will apply filters and return $term.
  • get_term_link(): * @param object|int|string $term

In both cases you need to read the documentation, and then possibly read the first few lines of the function if you're not used to "int" meaning a literal integer..

Simple fact here, Is that the function cannot change to accept a numerical string for ID lookups, nor can any of the Taxonomy API's which accept multiple data types be changed to accept numerical strings (those which only take int's seem to all cast them appropriately). so ultimately, the only change needed here is better education, through more documentation or more forceful hints as to the datatypes.

comment:16 in reply to: ↑ 15 @mikeschinkel4 years ago

Replying to dd32:

Replying to mikeschinkel:
Well no, Because that's not a term object is it? It's your data, as a plugin developer it's your responsibility to pass the correct parameters to functions. The documentation *may* (I haven't read it) may need to stress the fact it requires a integer value to be passed, not a numeric string.

Code architecture really should strive to minimize accidental misuse. To say "Yeah, it's easy to make that common mistake but it was documented otherwise" should at best be considered a short term hack and at work considered a failure of the code architect.

Taken to an extreme assume that calling wp_delete_post($args) with an empty $args array would delete all posts but that it was documented to be that way. I doubt you'd argue that deleting all posts on a misuse of that function would be acceptable; why would you argue the same for a different function just because its result is less damaging?

In both cases you need to read the documentation, and then possibly read the first few lines of the function if you're not used to "int" meaning a literal integer..

I've read the documentation, I know what it says and how to use it. That's not the issue; the issue is that it doesn't make sense to have such a distinction that invites misuse and hence unexpected bugs when few if any other functions in WordPress behave that way; most will accept numbers as string as if they were numbers.

Simple fact here, Is that the function cannot change to accept a numerical string for ID lookups, nor can any of the Taxonomy API's which accept multiple data types be changed to accept numerical strings (those which only take int's seem to all cast them appropriately).

I must be missing something obvious. Why can't is_int() be changed to 'is_numeric()` to solve this issue simply?

-Mike

Version 0, edited 4 years ago by mikeschinkel (next)

comment:17 @dd324 years ago

  • Keywords close added

I must be missing something obvious. Why can't is_int() be changed to 'is_numeric()` to solve this issue simply?

As mentioned in above comments; Because the function accepts either an ID, or a Slug. A Slug may be numeric.

comment:18 @dd324 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from reopened to closed

re-closing as invalid.

get_term_link expects either

  • An int of the term_id
  • A string which is the term slug (and a slug cannot contain a /)
  • The entire $term object

Other API functions which return the int fields as strings can be fixed elsewhere, Such as #17646

Note: See TracTickets for help on using tickets.