WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#16521 closed defect (bug) (fixed)

Inconsistent return values in get_category_link and get_tag_link

Reported by: nacin Owned by:
Milestone: 3.1 Priority: normal
Severity: critical Version:
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

In 3.0, we had very inconsistent parameters and return values in get_category_link and get_tag_link.

get_tag_link() could return WP_Error if the term didn't exist, despite not being documented in the phpdoc. But, it also could handle objects as $tag_id being passed in, despite being undocumented.

get_category_link() could return WP_Error if the term didn't exist, despite not being documented. But, it could handle objects as $category_id being passed in -- only when using pretty permalinks -- despite being undocumented.

get_term_link() did not document that it could return WP_Error. But it could also take object|int|string as a $term, and it would work.

For some time, we just pasted over the issues: [16430], [17341] (doc fixes), #16385 (wontfix), #16282 (don't even know), . But I keep seeing sporadic reports in the forums of these functions blowing up themes. That's not good.

As template functions, they should be avoiding WP_Error to begin with. Let's prevent get_category_link and get_term_link from returning WP_Error, and let's allow object|string|int for $category(_id) and $tag(_id).

Attachments (3)

16521.diff (1.3 KB) - added by nacin 4 years ago.
16521.2.diff (2.0 KB) - added by nacin 4 years ago.
16521.3.diff (2.0 KB) - added by ryan 4 years ago.

Download all attachments as: .zip

Change History (19)

@nacin4 years ago

comment:1 @nacin4 years ago

Initial fix just touches get_category_link() and get_tag_link(). get_term_link() still functions the same and returns WP_Error.

If that's worth changing, second patch coming up.

@nacin4 years ago

comment:2 @nacin4 years ago

  • Keywords has-patch dev-feedback added

comment:3 @ryan4 years ago

.2.diff works for me.

comment:4 follow-ups: @westi4 years ago

Template tags should never return WP_Error - I think that should be a rule

comment:5 in reply to: ↑ 4 @markjaquith4 years ago

Replying to westi:

Template tags should never return WP_Error - I think that should be a rule

That seems prudent. WordPress themes are popular partly because of simplicity. Handling error objects isn't simple.

comment:6 @ryan4 years ago

Tested all three functions with empty string, invalid id, invalid slug, invalid object as well as with valid id, slug, and object. Looks good.

comment:7 @ryan4 years ago

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

(In [17437]) Return empty strings instead of WP_Errro from get_category_link(), get_term_link(), and get_tag_link() when passed an invalid term. Clarify phpdoc. Props nacin. fixes #16521 for trunk

comment:8 @nacin4 years ago

Want to also point out that this removes an int cast on the $category/term_id. These were added in [15825] and did not previously exist. Keeping them out allows for the flexibility introduced here. My point is, a number as a string, such as '1', would have been treated as a slug in 3.0 too.

comment:9 @ryan4 years ago

(In [17438]) Return empty strings instead of WP_Errro from get_category_link(), get_term_link(), and get_tag_link() when passed an invalid term. Clarify phpdoc. Props nacin. fixes #16521 for 3.1

comment:10 @ryan4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Places such as get_the_category_list() are passing string ints and expecting them to be treated as IDs. Now they are treated as slugs which results in fail. We need a more conservative patch.

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

@ryan4 years ago

comment:11 @ryan4 years ago

If the arg is not an object, it is cast to int. This disallows slugs, but that seems the only sane path. get_term_link() is returned to what is was before since the phpdoc for it says WP_Error can be returned and there are places in the code that explicitly check for a WP_Error return.

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

comment:12 @nacin4 years ago

Testing well for me.

comment:13 @ryan4 years ago

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

(In [17443]) Don't allow passing slugs to get_tag_link() and get_category_link(); they are too ambiguous. Restore WP_Error return to get_term_link(). fixes #16521 for trunk

comment:14 @ryan4 years ago

(In [17444]) Don't allow passing slugs to get_tag_link() and get_category_link(); they are too ambiguous. Restore WP_Error return to get_term_link(). fixes #16521 for 3.1

comment:15 in reply to: ↑ 4 @scribu4 years ago

Replying to westi:

Template tags should never return WP_Error - I think that should be a rule

I agree, but which are template tags and which are simple API functions?

comment:16 @filosofo4 years ago

Anything that would likely be used in a non-functions.php template file to print markup.

Note: See TracTickets for help on using tickets.