WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 7 months ago

#47099 closed defect (bug) (wontfix)

term_exists fail when term is surrounded with brackets

Reported by: leogermani Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: close reporter-feedback
Focuses: Cc:
PR Number:

Description (last modified by SergeyBiryukov)

Steps to reproduce:

  1. Create a term. For example test.
  2. run term_exists('[test]', $taxonomy)

Expected result: It should return false, because it is a different term

Actual result: It returns the test term.

This issue is related to #45333 where there are more details around the discussions in regard to term_exists inconsistencies. Basically this function handles the term name string in a different way that wp_insert_term() does.

If someone could review it and agree we should work on a fix for term_exists I could do this, writing also the related tests.

Cheers.

Change History (4)

#1 @SergeyBiryukov
7 months ago

  • Component changed from General to Taxonomy
  • Description modified (diff)

#2 @boonebgorges
7 months ago

  • Keywords close reporter-feedback added

The issue is that term_exists() doesn't know whether you're passing in a slug or a name. When it does a slug check, the value is passed through sanitize_title(). This "sanitizes" [test] to test.

One way to address the problem is for you to be able to specify whether [test] is intended as a slug or a name. This is called get_term_by() :) As I've noted previously, my recommendation is to use this function instead.

If we were to change the behavior of term_exists(), it'd presumably be something along the lines of: when the $term passed to the function contains characters that'd normally be stripped by sanitize_title(), skip the slug check (or modify it somehow, or something like that). But any change of this sort is highly likely to break someone's existing code that depends on the existing idiosyncracies of term_exists().

Given that we have a viable alternative in get_term_by(), I don't see the benefit of risking these compatibility breaks.

It's possible that there's a technical strategy for addressing the issue that would *not* break compatibility (though it's hard for me to see what it'd be, since you are, in fact, suggesting a behavior change!). If you've got concrete ideas, please feel free to pitch them. But note that, in my opinion, a "fix" for this longstanding behavior that breaks existing plugins is probably not going to be viable.

#3 @leogermani
7 months ago

Hi @boonebgorges ,

As I mentioned in #45333, get_term_by() is not a complete replacement for term_exists() because it does not expect parent as a parameter, so you can not achieve everything term_exists() was supposed to do.

But I completely agree with you when you say: "But any change of this sort is highly likely to break someone's existing code that depends on the existing idiosyncracies of term_exists()." This is a very good way to put it :)

So maybe, and just maybe, we could create a new version of this method, with another name, and deprecate current term_exists() in favor of this new one... And check how the core uses it.

Do you believe there is any chance that people would like this approach? Am I the only one bothered by term_exists() idiosyncracies? :)

cheers!

just as an example: in my plugin I had to implement my own version of term_exists because I cant rely on the native function. (https://github.com/tainacan/tainacan/blob/develop/src/classes/repositories/class-tainacan-terms.php#L293)

#4 @boonebgorges
7 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

term_exists() was built to be a "magic" function: you can pass it different kinds of input (name or slug) and it'll magically try to figure out which one you mean. This kind of "magic" is the root of the issue described in this ticket, as well as many other current and past term_exists() tickets. This indicates that we should not be creating more magic :)

You're right that get_term_by() doesn't support 'parent'. Perhaps it should - I can see an argument for doing so. Maybe you could open a separate enhancement ticket with this idea?

Alternatively, you can always use get_terms() or WP_Term_Query, which is what get_term_by() uses internally anyway:

$found = get_terms( [
  'taxonomy' => $taxonomy,
  'parent'   => $parent_id,
  'name'     => $name,
] );

if ( $found ) {
  $term = array_shift( $terms );
} else {
  $term = null;
}
Note: See TracTickets for help on using tickets.