Introduce WP_Term class

[scribu]

In the current taxonomy API, you end up having to pass the taxonomy name over and over again.

I propose we have a WP_Term class to avoid that.

Example

Current:

get_term_link( get_term_by( 'name', 'Term Name', 'taxname' ), 'taxname' )

Proposed:

get_term_by( 'name', 'Term Name', 'taxname' )->get_link()

get_term_by() would return a WP_Term instance instead of a stdClass instance.

Besides get_link(), the WP_Term class could also have an update() method that would replace (or complement) wp_update_term().

Inspired by #14156.

Chaining functions would have to wait until PHP 5.

Sure, but this enhancement is not dependant on chaining. You could still do this:

$term = get_term_by( 'name', 'Term Name', 'taxname' );
echo $term->get_link();

which would still be better than:

$term = get_term_by( 'name', 'Term Name', 'taxname' );
echo get_term_link( $term, 'taxname' );

Found out that this is pretty easy to implement as a plugin. See ​wp-term.php

Having a WP_Term object is a great idea.

Instead of an update method that gets an array of arguments, why not have it save the current object's properties to the database?

$term->set_description('my new description');
$term->update(); // now the new description is saved to the database

As suggested above, it would also be nice to have getters and possibly setters for the other salient properties in addition to "link."

Yeah, another one would be WP_Term::get_children().

implemented as a plugin: get_link(), get_children()

Personally, I'm not a big fan of this type of encapsulation working its way into WordPress.

I want to ensure that everything is simple to use and I'm not sure lots of objects like this is the way to go.

Replying to westi:

Personally, I'm not a big fan of this type of encapsulation working its way into WordPress.

I want to ensure that everything is simple to use and I'm not sure lots of objects like this is the way to go.

Could you elaborate your objections? Terms already are objects, so this would extend existing functionality, not diminish it or require people to use a new API.

extra check for get_terms()

This will definitely happen, hopefully soon

I think it is a good idea, an example of a practical use would also be the :

$term = get_term_by( 'name', 'Term Name', 'taxname' );
if ( is_a( $term, 'WP_Term' ) ) {
    ...
}

Since the term code is getting overhauled anyway, maybe we could skip the taxname requirement altogether and just use term_id which will soon be the equivalent of term_taxonomy_id Of course this assumes you know the ID already, which may not be 100% applicable in this case, but I thought it would be worth mentioning.

a ticket already exists - we should start thinking about what this should look like

A starting point for the WP_Term class and its integration

I created the patch above to have a starting point for the WP_Term class and its integration into its related term functions. I hope it's something we can build upon.

Please note that all function calls still require specification of the taxonomy - removing this requirement is part of #30262 I think.

One thing we should think about is which additional term properties and term functions we should make accessible using magic methods. Also whether we should rather use the __get() method (like in the patch) or include wrapper functions in the class instead (for example, should we be able to get the term link via $term->link or $term->get_link()?).

Another thing that should probably be added to the class later is the functionality to get term meta using __get(), similar to how it is handled in WP_Post.

In 34035:

Add unit tests for get_term().

These tests will be useful as we begin to refactor in favor of WP_Term.

See #14162.

Added unittest for get_term

@flixos90 Thanks for your patch. This is a helpful starting point. I've just spent a while working with it.

Please note that all function calls still require specification of the taxonomy - removing this requirement is part of #30262 I think.

Not necessarily. Taxonomy only needs to be specified if terms in two taxonomies can share the same ID. After the shared-term splitting in 4.3, this should no longer be the case. I would strongly prefer to remove this requirement, so that you can simply say get_term( $term_id ) and $term = new WP_Term( $term_id ).

The problem with this proposal is that it breaks our current cache implementation. Term caches are currently sorted into buckets based on taxonomy: $_term = wp_cache_get( $term_id, $taxonomy ). But this schema means that we need to know the taxonomy before we check the cache. This makes it hard to see how get_term( $term_id ) will work without restructuring how terms are cached - probably by moving them to a single bucket, ie $_term = wp_cache_get( $term_id, 'terms' ). This would mean that a cache reorganization would have to happen *before* (or at the same time as) WP_Term is introduced.

This makes the job somewhat more complicated, but I think it's far better to introduce the new class - which represents a brand new interface for the API - without the extra baggage of an extraneous $taxonomy parameter.

14162.2.patch​ is an attempt to do this. I had to change a couple of existing unit tests that were referencing the old cache locations. And there is some terrible logic built in that allows shared terms to continue to be split inside of wp_update_term() while still allowing $taxonomy to be an optional parameter. This patch passes all but 1 unit test, which I'm too tired to figure out at the moment.

From flixos90's original patch, I also made a few changes. I removed the to_array() juggling in _make_cat_compat() in favor of removing the by-reference assignments, which don't really do anything in PHP5 anyway.

I also removed the magic getters. I'm not opposed to adding them, but (a) it's not clear that it needs to be done in the first iteration, and (b) the backward compatibility concerns that led to the properties being added to WP_Post don't apply here. The discussion at https://core.trac.wordpress.org/ticket/21309 provides some helpful background. For the moment, I'd suggest that we get our cache implementation straight, and then we can talk about pulling in various bits of term functionality.

One additional note: The filter and sanitize_term() stuff here - and in WP_Post - is quite confusing. The idea is to provide out-of-the-box sanitization for any term field. But we should try to avoid double-sanitizing, and ideally we would centralize the sanitization logic more than it is in WP_Post. This needs an audit.

unittest for get_term updated

Replying to boonebgorges:

@flixos90 Thanks for your patch. This is a helpful starting point. I've just spent a while working with it.

Please note that all function calls still require specification of the taxonomy - removing this requirement is part of #30262 I think.

Not necessarily. Taxonomy only needs to be specified if terms in two taxonomies can share the same ID. After the shared-term splitting in 4.3, this should no longer be the case. I would strongly prefer to remove this requirement, so that you can simply say get_term( $term_id ) and $term = new WP_Term( $term_id ).

I totally agree, I just wasn't sure how we would handle the cache issue, so I left it unmodified first.

I also removed the magic getters. I'm not opposed to adding them, but (a) it's not clear that it needs to be done in the first iteration, and (b) the backward compatibility concerns that led to the properties being added to WP_Post don't apply here. The discussion at https://core.trac.wordpress.org/ticket/21309 provides some helpful background. For the moment, I'd suggest that we get our cache implementation straight, and then we can talk about pulling in various bits of term functionality.

I didn't know those magic methods in WP_Post were added for backwards compatibility. I would prefer to have them there, but, like you said, we should probably get the core of this issue figured out first.

Another thing I noticed while writing the __get() function was that there is no get_term_ancestors() function (similar to get_post_ancestors()) which might be useful I think. What do you think about that? Worth another ticket?

Made $taxonomy argument optional in more functions

Based on attachment:14162.2.patch​ I created the above patch with the following adjustments:

• if no taxonomy is defined in get_term(), make sure the filters still receive a valid taxonomy name
• made $taxonomy argument optional for the functions
  • get_term_by()
  • get_term_field()
  • get_term_to_edit()
  • term_is_ancestor_of()
  • sanitize_term()
• in the above functions, if a taxonomy name is not specified, but required (like for a filter), the taxonomy of the term is used

in the above functions, if a taxonomy name is not specified, but required (like for a filter), the taxonomy of the term is used

Good catch - I was too lazy to get to this :)

made $taxonomy argument optional for the functions

Thanks for this. When it comes time to commit, I'll probably break up the introduction of WP_Term from the swapping out of get_term() in existing functions; eliminating the required $taxonomy parameter will be part of that second commit (or set of commits). But it's helpful to see here what the full diff will look like

I need to take some time to look more seriously at the backward compatibility considerations that led to the handling of 'ancestors' etc in WP_Post. At a glance, it looks like there were places in core that were doing something like this:

$post = $wpdb->get_row( "SELECT * FROM $wpdb->posts WHERE post_id = 12345" );
$post->ancestors = some_function_to_get_post_ancestors();

// ...

foreach ( $post->ancestors as $ancestor ) {
     // ...
}

So the reason for using a magic method rather than get_ancestors() probably had to do with preventing overloaded WP_Post objects.

I don't think the same thing is true of term objects, or if it's true, it's true in different ways :) We should figure out the history before we go throwing it all into WP_Term.

I'll try to find time early next week to look at this, but if anyone beats me to svn blame before that, be my guest :)

Has anybody looked into the history as outlined by @boonebgorges above? We need to get this moving if it's wanted in 4.4.

14162.diff​ refreshes the patch. It also fixes a small bit of logic in the inline term splitting that happens when a non-matching taxonomy is passed to get_term() - this was the cause for at least one of the test failures. I also streamlined the way sanitization works a little bit.

I looked over the codebase to see if we were overloading $term objects anywhere, but couldn't find anything. So I feel fairly confident about what we've currently got. Let's drop it in and see if the world explodes.

DrewAPicture, can I ask you for a docs review?

Hot dog! Sure, I'll take a look.

Docs audit

@boonebgorges: 14162.2.diff​ includes fixes from my docs audit. Good to go!

In 34997:

Introduce WP_Term.

get_term() now returns a WP_Term object, instead of a stdClass object.
Cache support and sanitization filters for individual terms are now more
centralized. For example, get_term_by() is able to cast results of its query
to a WP_Term object by passing it through get_term().

The $taxonomy parameter for get_term() is now optional, as terms ought to
be unique to a taxonomy (ie, shared terms no longer exist). In cases where
get_term() detects that the term matching the specified term_id is from the
wrong taxonomy, it checks to see if you've requested a shared term, and if so,
it splits the term. This is used only for fallback purposes.

The elimination of shared terms allows the caching strategy for terms to be
simplified. Individual terms are now cached in a single 'terms' bucket.

Props flixos90, boonebgorges, scribu, dipesh.kakadiya.
See #14162.

In 34998:

Return WP_Term objects from get_terms().

Props boonebgorges, flixos90, DrewAPicture.
See #14162.

In 34999:

Return WP_Post objects from wp_get_object_terms().

A side effect of this change is that terms stored in the cache no longer have
an object_id associated with them. Previously, object_id had always been
cached when the term cache was populated via wp_get_object_terms(), a
strategy that was mostly harmless but still incorrect.

See #14162.

Basic functionality here is complete. Functions that return single terms (get_term(), get_term_by()) now return WP_Term objects. Functions that return arrays of terms (get_terms(), wp_get_object_terms()) now return arrays of WP_Term objects.

These changes don't really have much practical effect. Calls to the functions above have always primed term caches. The real performance improvements will come through more extensive leveraging of the individual term caches, especially in functions that return bulk terms. Let's handle that issue in a separate ticket.

FYI, the change to get_term_by to always return the term when passing ttid ​broke the REST API. We've fixed it, but might be worth noting in a field notes post for others that it might break.

This is actually #30620.

In 35227:

Return null from get_term() on taxonomy mismatch.

[34997] caused get_term() to return an error object in the case when
$taxonomy did not match the taxonomy of the located term. This was an
inadvertant change from the previous behavior, when get_term() would return
null in these cases.

Props dlh.
See #14162. Fixes #34332.