WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#12526 closed enhancement (fixed)

Introduce has_term() to the tax API

Reported by: ptahdunbar Owned by:
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.0
Component: Taxonomy Keywords: has-patch needs-refresh
Focuses: Cc:

Description

Simple patch that just extends has_tag() into has_taxonomy(). Tested, and works like charm.

Attachments (4)

has_taxonomies.diff (2.1 KB) - added by ptahdunbar 4 years ago.
has_term.diff (6.1 KB) - added by ptahdunbar 4 years ago.
introduces has_term, deprecates in_category in favor of has_category, and has_category and has_tag are wrappers for has_term
has_term.2.diff (3.8 KB) - added by ptahdunbar 4 years ago.
same as previous patch, but does not deprecate in_category.
clipboard.patch (2.9 KB) - added by hakre 4 years ago.
Argument Order and Optionality

Download all attachments as: .zip

Change History (25)

comment:1 dd324 years ago

  • Milestone changed from Unassigned to 3.1
  • Version set to 3.0

Missing a return in there.

There might be a better location for the has_taxonomy() function too, But i'm not too sure where all the other taxonomy functions are stored right now..

ptahdunbar4 years ago

ptahdunbar4 years ago

introduces has_term, deprecates in_category in favor of has_category, and has_category and has_tag are wrappers for has_term

comment:2 ptahdunbar4 years ago

  • Keywords dev-feedback added
  • Summary changed from Introduce has_taxonomy() to the tax API to Introduce has_term() to the tax API

ptahdunbar4 years ago

same as previous patch, but does not deprecate in_category.

comment:3 filosofo4 years ago

  • Owner filosofo deleted
  • Status changed from new to assigned

comment:4 simonwheatley4 years ago

  • Cc simon@… added

+1 for this, I've just concocted something very similar.

comment:5 kevinB4 years ago

  • Cc kevinB added

comment:6 scribu4 years ago

  • Cc scribu@… added

comment:7 scribu4 years ago

  • Milestone changed from Awaiting Triage to 3.1

comment:8 ryan4 years ago

Patch needs refresh. Also, we can get rid of the $GLOBALSpost? stuff and just use get_post().

comment:9 nacin4 years ago

  • Keywords needs-refresh added; dev-feedback removed

comment:10 ryan4 years ago

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

(In [15677]) has_term(). Props ptahdunbar. fixes #12526

comment:11 hakre4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Please improve the function argument order, first $taxonomy, then $term. Looks more natural to me.

Example:

function has_term( $term = '', $taxonomy = '', $post = null ) { 

to:

function has_term( $taxonomy = '', $term = '', $post = null ) { }

Next to that, those arguments are not optional, in this implementation they are. That's probably a mistake.

I don't see any necessity to introduce optional arguments here. The post->id can be generated in the deprecated function.

I check if I can make a useful patch, reopening so far.

comment:12 hakre4 years ago

Functions that are easy to read (e.g. that consists only of a function call to has_term()), when the make use of the @use docblock that states this simple fact, is only adding redundant information and instead of helping, makes it harder to read and understand the function.

comment:13 scribu4 years ago

That would be inconsistent with all the rest of the tax api which has $term first and $taxonomy second.

comment:14 hakre4 years ago

Replying to scribu:

That would be inconsistent with all the rest of the tax api which has $term
first and $taxonomy second.

Oh that's not true. For example is_object_in_term() has $taxnomy first and then $term. All the functions that make use of has_term not even have the taxonomy name in their function name.

hakre4 years ago

Argument Order and Optionality

comment:15 hakre4 years ago

Made a patch that puts the arguments into the expected order and removes their optional status (at least for the first two) as it makes no sense to run a check on a NULL taxononmy or look for the NULL term.

Additionally I tried to make the docblocks less complicate. It's far away from perfect, just one little step, I'm sure everybody else will find additional stuff that can be improved with the docblocks. Looks like a lot of redundant information in there.

There were some minor issues that looked like being created by doing copy & paste unreviewed.

comment:16 scribu4 years ago

All the functions that make use of has_term not even have the taxonomy name in their function name.

They're not relevant to the discussion precisely because they don't have a $taxonomy parameter.

Oh that's not true. For example is_object_in_term() has $taxnomy first and then $term.

Ok, so you found a single counter-example. That doesn't validate adding yet another inconsistency.

Made a patch that puts the arguments into the expected order and removes their optional status (at least for the first two) as it makes no sense to run a check on a NULL taxononmy or look for the NULL term.

NULL taxonomy no, but NULL term yes: check if the post has any terms in the given taxonomy.

comment:17 hakre4 years ago

Well the natural order is that you start with the more broad one (e.g. taxnomony) and then you locate the concrete in there (e.g. term). So my first argument is the expected order of arguments. Remember, this is introducing a new function.

Next to that I just prooved you wrong. There are more than that one function where this is the case.

NULL term as a joker. Awful if you ask me. I suggest to add an aditional function: has_taxonomy() and then to check for taxnonomy only - not for a term (as the function name suggests).

Naming should be precise. At least we should try hard if we introduce something new :)

comment:18 nacin4 years ago

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

Our API is quite consistent with term then tax.

Closing as fixed.

comment:19 hakre4 years ago

Related: #13257

comment:20 hakre4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Last commit did contain misleading information in the docblocks at least. Just pick the stuff you're ok with from the patch and drop the rest nacin. But even try to iterate when there's some feedback for the good of us all.

comment:21 ryan3 years ago

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

(In [16307]) phpdoc fixes. Props hakre. fixes #12526

Note: See TracTickets for help on using tickets.