WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 2 years ago

Last modified 17 months ago

#14162 closed task (blessed) (fixed)

Introduce WP_Term class

Reported by: scribu Owned by: boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: dev-feedback has-patch
Focuses: Cc:

Description (last modified by 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.

Attachments (9)

wp-term.php (748 bytes) - added by scribu 8 years ago.
implemented as a plugin: get_link(), get_children()
wp-term.2.php (807 bytes) - added by scribu 8 years ago.
extra check for get_terms()
14162.patch (10.8 KB) - added by flixos90 2 years ago.
A starting point for the WP_Term class and its integration
14162.2.patch (13.7 KB) - added by boonebgorges 2 years ago.
14162_2.diff (12.2 KB) - added by dipesh.kakadiya 2 years ago.
Added unittest for get_term
14162_3.diff (12.2 KB) - added by dipesh.kakadiya 2 years ago.
unittest for get_term updated
14162.3.patch (16.4 KB) - added by flixos90 2 years ago.
Made $taxonomy argument optional in more functions
14162.diff (16.6 KB) - added by boonebgorges 2 years ago.
14162.2.diff (18.5 KB) - added by DrewAPicture 2 years ago.
Docs audit

Download all attachments as: .zip

Change History (50)

#1 @nacin
8 years ago

Chaining functions would have to wait until PHP 5.

#2 @scribu
8 years ago

  • Description modified (diff)

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' );

#3 @scribu
8 years ago

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

#4 @filosofo
8 years ago

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."

#5 @scribu
8 years ago

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

@scribu
8 years ago

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

#6 @kevinB
8 years ago

  • Cc kevinB added

#7 follow-up: @westi
8 years ago

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.

#8 in reply to: ↑ 7 @filosofo
8 years ago

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.

#9 @sirzooro
8 years ago

  • Cc sirzooro added

@scribu
8 years ago

extra check for get_terms()

#10 @nacin
7 years ago

  • Milestone changed from Awaiting Review to Future Release

#11 @scribu
7 years ago

  • Summary changed from Introduce WP_Term to Introduce WP_Term class

#12 @mikeschinkel
7 years ago

  • Cc mikeschinkel@… added

#13 @andyadams
5 years ago

  • Cc andyadamscp@… added

#14 @wonderboymusic
4 years ago

  • Keywords needs-refresh added

This will definitely happen, hopefully soon

#15 @fgirardey
3 years ago

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' ) ) {
    ...
}

#16 @theMikeD
3 years ago

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.

Last edited 3 years ago by theMikeD (previous) (diff)

This ticket was mentioned in Slack in #core by helen. View the logs.


3 years ago

#18 @wonderboymusic
2 years ago

  • Milestone changed from Future Release to 4.4
  • Owner set to boonebgorges
  • Status changed from new to assigned

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

This ticket was mentioned in Slack in #core by boone. View the logs.


2 years ago

#20 @wonderboymusic
2 years ago

  • Keywords needs-patch added; needs-refresh removed

@flixos90
2 years ago

A starting point for the WP_Term class and its integration

#21 @flixos90
2 years ago

  • Keywords dev-feedback added

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.

#22 @boonebgorges
2 years ago

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.

@dipesh.kakadiya
2 years ago

Added unittest for get_term

#23 follow-up: @boonebgorges
2 years ago

@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.

@dipesh.kakadiya
2 years ago

unittest for get_term updated

#24 in reply to: ↑ 23 @flixos90
2 years ago

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?

@flixos90
2 years ago

Made $taxonomy argument optional in more functions

#25 @flixos90
2 years ago

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

#26 @boonebgorges
2 years ago

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 :)

This ticket was mentioned in Slack in #core by drew. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by boone. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by sergey. View the logs.


2 years ago

#30 @helen
2 years ago

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

@boonebgorges
2 years ago

#31 @boonebgorges
2 years ago

  • Owner changed from boonebgorges to DrewAPicture
  • Status changed from assigned to reviewing

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?

#32 @wonderboymusic
2 years ago

  • Type changed from enhancement to task (blessed)

#33 @DrewAPicture
2 years ago

  • Status changed from reviewing to accepted

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

@DrewAPicture
2 years ago

Docs audit

#34 @DrewAPicture
2 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner changed from DrewAPicture to boonebgorges
  • Status changed from accepted to assigned

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

#35 @boonebgorges
2 years ago

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.

#36 @boonebgorges
2 years ago

In 34998:

Return WP_Term objects from get_terms().

Props boonebgorges, flixos90, DrewAPicture.
See #14162.

#37 @boonebgorges
2 years ago

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.

#38 @boonebgorges
2 years ago

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

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.

#39 @rmccue
2 years ago

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.

Last edited 2 years ago by rmccue (previous) (diff)

#40 @boonebgorges
2 years ago

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.

#41 @johnbillion
17 months ago

In 38489:

Taxonomy: Update various docs for parameters which are now WP_Term objects.

See #37770
See #14162

Note: See TracTickets for help on using tickets.