WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15407 closed defect (bug) (fixed)

get_the_category code duplication

Reported by: filosofo Owned by: filosofo
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.1
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

get_the_category needlessly duplicates the functionality of get_the_terms, and it lacks a filter that the corresponding get_the_tags has.

Attachments (4)

get_the_category.dry.15407.diff (965 bytes) - added by filosofo 3 years ago.
deprecate-get_the_category.15407.diff (5.6 KB) - added by filosofo 3 years ago.
15407.diff (787 bytes) - added by nacin 3 years ago.
15407.fix-for-no-category-posts-fatal-error.patch (639 bytes) - added by Viper007Bond 3 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 scribu3 years ago

Shouldn't the filter be named 'get_the_category' ?

comment:2 filosofo3 years ago

Well, my thought was to be consistent with get_the_tags, so that the general form would be get_the_[taxonomy]s

The function itself, get_the_category(), is rather poorly named.

comment:3 scribu3 years ago

How about we deprecate get_the_category() and add a get_the_categories() then?

comment:4 filosofo3 years ago

I'll make a patch.

comment:5 filosofo3 years ago

deprecate-get_the_category.15407.diff deprecates and replaces get_the_category().

comment:6 automattor3 years ago

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

(In [16332]) Replace get_the_category() with get_the_categories(). Props filosofo. Fixes #15407

comment:7 nacin3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

For consistency, the_category() with the_categories()?

These are very commonly used template tags so I'm not sure deprecating them for a pluralization is worth it. We've been fine for 8+ major releases with them as is.

comment:8 filosofo3 years ago

I'm not wedded to the name change, but I think the code duplication fix of the original patch should stay.

I also think that get_the_categories as a filter makes more sense than get_the_category or the like, because it's filtering an array.

comment:9 filosofo3 years ago

And actually, maybe it wouldn't be a bad idea to go with the_categories(). It's easier to remember, and the Extend theme reviewers will love having the_category() deprecated so they can smite even more themes. :)

comment:10 westi3 years ago

  • Cc westi added
  • Keywords revert added

The name change needs to be reverted - we should and will not rename a function just for the sake of the name being more proper.

comment:11 westi3 years ago

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

(In [16334]) Revert most of [16332] - renaming and deprecating a commonly used function is silly. Improving it is good. Fixes #15407.

comment:12 follow-up: Denis-de-Bernardy3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The newly introduced filter's name is plural and inconsistent with the function's name. Suggesting we rename it accordingly.

comment:13 filosofo3 years ago

It makes more sense to name a filter according to the use of the filter rather than its containing function, especially when the function is poorly named.

comment:14 in reply to: ↑ 12 westi3 years ago

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

Replying to Denis-de-Bernardy:

The newly introduced filter's name is plural and inconsistent with the function's name. Suggesting we rename it accordingly.

This is intentional.

comment:15 nacin3 years ago

(In [16343]) Move cast to within array_keys to prevent warning when get_the_terms returns false. see #15407.

comment:16 nacin3 years ago

  • Keywords revert removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

After an IRC discussion with Viper007Bond, the issue here is that get_the_terms() may return false. I was trying to prevent issues there, but it may be cast awkwardly into an array(0=>null) which then causes some problems.

Shuffling things around here a bit.

nacin3 years ago

comment:17 nacin3 years ago

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

(In [16395]) Properly force variable to an array. props Viper007Bond, fixes #15407.

comment:18 Viper007Bond3 years ago

Whoops, I guess you saw my message in IRC before you timed out. :)

comment:19 ryan3 years ago

(In [16487]) Populate post term relationship cache in get_the_terms(). Restores caching lost when we moved away from get_the_category(). see #15407

comment:20 gazouteast3 years ago

I struggled to follow this one - are you saying that get_the_category() and the_category() are now deprecated?

comment:21 nacin3 years ago

No. They were deprecated, then undeprecated.

Note: See TracTickets for help on using tickets.