WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 20 months ago

#30415 closed enhancement (duplicate)

Add alternative to get_category_parents() that will work for any taxonomy

Reported by: hlashbrooke Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: needs-unit-tests needs-patch
Focuses: Cc:

Description

The get_category_parents() function is, as the name suggests, limited to only fetching the parents for the category taxonomy. It would be great if we had the same function available for any specified taxonomy to use.

My suggested patch (attached) adds a new function, get_term_parents(), which is a direct copy of the existing function, but with an added $taxonomy parameter so you can use it for any given taxonomy. I have then updated the original get_category_parents() to simply be a wrapper for this new function, so it is fully backwards compatible.

The only modification to the original function (aside from some coding standards updates) is the addition of an is_taxonomy_hierarchical() check, which now becomes necessary.

Attachments (3)

30415.diff (2.6 KB) - added by hlashbrooke 4 years ago.
Patch as described in ticket body.
30415.2.diff (2.7 KB) - added by hlashbrooke 4 years ago.
Fixing function references as well as return value if taxonomy is not hierarchical
30415.3.diff (4.4 KB) - added by hlashbrooke 4 years ago.
Refactoring solution

Download all attachments as: .zip

Change History (18)

@hlashbrooke
4 years ago

Patch as described in ticket body.

#1 @hlashbrooke
4 years ago

  • Component changed from General to Taxonomy
  • Keywords has-patch needs-unit-tests added

#2 follow-up: @jeffikus
4 years ago

I'm just thinking, the category-template file contains mostly category specific functions. It would probably be better to move this functionality into wp-includes/taxonomy.php and call the new function from get_category_parents() to eventually deprecate it.

#3 in reply to: ↑ 2 ; follow-up: @hlashbrooke
4 years ago

Replying to jeffikus:

I'm just thinking, the category-template file contains mostly category specific functions. It would probably be better to move this functionality into wp-includes/taxonomy.php and call the new function from get_category_parents() to eventually deprecate it.

That's true and I considered that, but there are other general term functions in that file, so thought I'd stick it there for now until there's some official go ahead to move it to a different file. I don't want to mess with the file structure too much without knowing if it's the best move or not.

#4 in reply to: ↑ 3 @jeffikus
4 years ago

Replying to hlashbrooke:

Replying to jeffikus:

I'm just thinking, the category-template file contains mostly category specific functions. It would probably be better to move this functionality into wp-includes/taxonomy.php and call the new function from get_category_parents() to eventually deprecate it.

That's true and I considered that, but there are other general term functions in that file, so thought I'd stick it there for now until there's some official go ahead to move it to a different file. I don't want to mess with the file structure too much without knowing if it's the best move or not.

I'll take a stab at it over the weekend, maybe tonight.

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


4 years ago

#6 follow-up: @ChriCo
4 years ago

On your current patch: 30415.diff

L1399

@return string|WP_Error A list of category parents on success, WP_Error on failure.

why then return false; ? :-) Wheres the new WP_Error ?

L1421

$chain .= get_category_parents( $parent->parent, $link, $separator, $nicename, $visited );

shouldn't you call get_term_parents() ?

and a note from my side: Wouldn't it better to implement a function ( get_term_parent_tree()) which returns the a tree (array) with parent => child ? Instead we should implement a function called get_term_parent_list_html().

Last edited 4 years ago by ChriCo (previous) (diff)

#7 in reply to: ↑ 6 @hlashbrooke
4 years ago

Replying to ChriCo:

On your current patch: 30415.diff

L1399

@return string|WP_Error A list of category parents on success, WP_Error on failure.

why then return false; ? :-) Wheres the new WP_Error ?

L1421

$chain .= get_category_parents( $parent->parent, $link, $separator, $nicename, $visited );

shouldn't you call get_term_parents() ?

and a note from my side: Wouldn't it better to implement a function ( get_term_parent_tree()) which returns the a tree (array) with parent => child ? Instead we should implement a function called get_term_parent_list_html().

Good point about the boolean return there - will generate a WP_Error in that case. Will fix up the other incorrect category references too - thanks for pointing that out :)

In terms of generating an array instead of the markup - that would be solid I think. For this patch I was simply improving the existing function, but you're right it would actually be better to have the functionality more split like that - I guess that's in the scope of this patch too, so will work on that, but will fix the other issues in the mean time first.

@hlashbrooke
4 years ago

Fixing function references as well as return value if taxonomy is not hierarchical

@hlashbrooke
4 years ago

Refactoring solution

#8 @hlashbrooke
4 years ago

I've modified the solution to include two new functions instead - both included in taxonomy.php:

  • get_term_parents() - returns an array of parent term IDs
  • get_term_parents_html() - return an HTML string of term parents in hierarchical order

get_category_parents() is now a wrapper for get_term_parents_html(), which in turn calls get_term_parents() to fetch the array from which to generate the HTML. This means that everything is still backwards compatible.

I named get_term_parents() as such in order to mimic the naming convention already in place with get_term_children().

#9 @hlashbrooke
4 years ago

  • Summary changed from Allow get_category_parents() to be used for any taxonomy to Add alternative to get_category_parents() that will work for any taxonomy

I've realised my folly of describing the patch in the ticket description - most of the original description is no longer relevant and I can't edit it now :(

Last edited 4 years ago by hlashbrooke (previous) (diff)

#10 @boonebgorges
3 years ago

  • Version trunk deleted

#11 @boonebgorges
3 years ago

In 31299:

Add tests for get_category_parents().

See #30415.

#12 follow-up: @boonebgorges
3 years ago

Thanks for the patches so far.

I understand the motivation behind breaking get_term_parents() into a separate function from get_term_parents_html(). But this creates a disparity between get_category_parents(), which returns a string, and get_term_parents(), which returns an array of parents.

If we're going to have a general tool for getting ancestors of a term, I'd rather have it be a new parameter for get_terms(), and it would be more efficient for it to work by fetching the cached hierarchy with _get_term_hierarchy() instead of crawling backward with a while loop. (Notwithstanding what get_category_parents() already does :) )

#13 in reply to: ↑ 12 @hlashbrooke
3 years ago

Replying to boonebgorges:

Thanks for the patches so far.

I understand the motivation behind breaking get_term_parents() into a separate function from get_term_parents_html(). But this creates a disparity between get_category_parents(), which returns a string, and get_term_parents(), which returns an array of parents.

That's a good point - it's frustrating when there's a disparity like that.

If we're going to have a general tool for getting ancestors of a term, I'd rather have it be a new parameter for get_terms(), and it would be more efficient for it to work by fetching the cached hierarchy with _get_term_hierarchy() instead of crawling backward with a while loop. (Notwithstanding what get_category_parents() already does :) )

So in that case get_terms() would be able to return an array of ancestors and then the new get_term_parents() simply uses that to return an HTML string identical to get_category_parents(). I think that makes a lot of sense - I'll rework my patch here and upload a new one once it's done.

#14 @boonebgorges
3 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to Future Release

#15 @johnbillion
20 months ago

  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #17069.

Note: See TracTickets for help on using tickets.