WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 months ago

#17069 closed enhancement (fixed)

missing get_term_parents() function

Reported by: thomask Owned by: keesiemeijer
Milestone: 4.8 Priority: normal
Severity: normal Version: 3.1
Component: Taxonomy Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

we got get_category_parents (http://core.trac.wordpress.org/browser/tags/3.1/wp-includes/category-template.php#L30)
which use $parent = &get_category( $id ); limited to 'category' taxonomy, but there is nothing similar to other taxonomies. IMO we should rewrite this function to be generic and use it as get_term_parents function, the existing get_category_parents would use that get_term_parents, so something like (sorry, i do not know how to use diff) this:

42	function get_term_parents( $id, $taxonomy, $link = false, $separator = '/', $nicename = false, $visited = array() ) {
43	        $chain = '';
44	        $parent = &get_term( $id, $taxonomy );
45	        if ( is_wp_error( $parent ) )
46	                return $parent;
47	
48	        if ( $nicename )
49	                $name = $parent->slug;
50	        else
51	                $name = $parent->name;
52	
53	        if ( $parent->parent && ( $parent->parent != $parent->term_id ) && !in_array( $parent->parent, $visited ) ) {
54	                $visited[] = $parent->parent;
55	                $chain .= get_term_parents( $parent->parent, $link, $separator, $nicename, $visited );
56	        }
57	
58	        if ( $link )
59	                $chain .= '<a href="' . get_term_link( $parent->term_id ) . '" title="' . esc_attr( sprintf( __( "View all posts in %s" ), $parent->name ) ) . '">'.$name.'</a>' . $separator;
60	        else
61	                $chain .= $name.$separator;
62	        return $chain;
63	}

and

function get_category_parents( $id, $link = false, $separator = '/', $nicename = false, $visited = array() ) {
   return get_term_parents ($id,'category',$separator,$nicename,$visited);
}

Attachments (5)

category-template.patch (1.6 KB) - added by rafaehlers 6 years ago.
the patch with the new function
17069.patch (2.7 KB) - added by SergeyBiryukov 6 years ago.
17069.1.patch (7.9 KB) - added by keesiemeijer 7 months ago.
Add get_term_parents_list() function and unit tests
17069.2.patch (8.4 KB) - added by keesiemeijer 7 months ago.
Add format argument and new unit test.
17069.3.patch (665 bytes) - added by keesiemeijer 6 months ago.
Replace get_category_link() with get_term_link()

Download all attachments as: .zip

Change History (30)

#1 @scribu
6 years ago

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

We have something even better than get_term_parents(): get_ancestors().

See #12443

#2 @scribu
6 years ago

Granted, it returns a raw array, not a list of links.

#3 @scribu
6 years ago

  • Keywords needs-testing has-patch removed
  • Milestone set to Future Release
  • Resolution duplicate deleted
  • Status changed from closed to reopened

I guess I was a bit hasty.

We could add a get_term_parents() template tag that uses get_ancestors() internally.

For creating patches: http://core.trac.wordpress.org/#HowtoSubmitPatches

#4 @rafaehlers
6 years ago

thomask: you missed on line 55 the argument $taxonomy

actual: $chain .= get_term_parents( $parent->parent, $link, $separator, $nicename, $visited );

should be: $chain .= get_term_parents( $parent->parent, $taxonomy, $link, $separator, $nicename, $visited );

#5 @rafaehlers
6 years ago

  • Cc rafaehlers added

another argument missing on line 59 on function get_term_link, missed $taxonomy and use $parent->slug instead.

actual: $chain .= '<a href="' . get_term_link( $parent->term_id ) . '" title="' . esc_attr( sprintf( __( "View all posts in %s" ), $parent->name ) ) . '">'.$name.'</a>' . $separator;

should be: $chain .= '<a href="' . get_term_link( '''$parent->slug, $taxonomy''') . '" title="' . esc_attr( sprintf( __( "View all posts in %s" ), $parent->name ) ) . '">'.$name.'</a>' . $separator;

Last edited 5 years ago by scribu (previous) (diff)

@rafaehlers
6 years ago

the patch with the new function

#6 @SergeyBiryukov
6 years ago

  • Keywords has-patch added

#7 @scribu
6 years ago

  • Keywords needs-patch added; has-patch removed

As I said, get_term_parents() should use get_ancestors(). And then, get_category_parents() should be made to use get_term_parents().

#8 @SergeyBiryukov
6 years ago

  • Keywords has-patch added; needs-patch removed

Right, updated the patch.

Looks like $visited parameter can be deprecated (it isn't documented in Codex anyway).

#9 @SergeyBiryukov
6 years ago

Related: #7030 (the latest patches overlap a lot)

#10 @alexvorn2
4 years ago

so closing? I would like this patch to be included in the core.

#11 @wonderboymusic
4 years ago

  • Keywords needs-refresh added

#12 @boonebgorges
2 years ago

  • Keywords needs-unit-tests added

#13 @johnbillion
9 months ago

#30415 was marked as a duplicate.

#14 @johnbillion
9 months ago

#35886 was marked as a duplicate.

#15 @johnbillion
9 months ago

  • Keywords good-first-bug added

@keesiemeijer
7 months ago

Add get_term_parents_list() function and unit tests

#16 @keesiemeijer
7 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

In patch 17069.1.patch I've renamed the function to get_term_parents_list to make it more clear what it returns. The $visited parameter is deprecated in get_category_parents() as it was only used to avoid duplicates from the recursive calls to itself.

#17 @keesiemeijer
7 months ago

  • Keywords needs-refresh removed

#18 @boonebgorges
7 months ago

  • Keywords good-first-bug removed
  • Milestone changed from Future Release to 4.8
  • Owner set to keesiemeijer
  • Status changed from reopened to assigned

@keesiemeijer Thanks for the excellent patch. A couple questions/comments:

  • I gather you're flipping the value of $nicename in get_category_parents() because you're changing the meaning of the param in get_term_parents_list(). I agree with your instinct that "nicename" doesn't really accurately describe what's happening in get_category_parents(), but if we're going to flip the meaning, we should just go ahead and change the parameter structure, to avoid having the same param name do opposite things in two related functions :). How about if get_term_parents_list() takes a 'format' argument, with possible values 'slug' and 'name'?
  • Why array_reverse() before generating the markup? Does get_ancestors() return values in a reverse order from the way that get_category_parents() currently does it?
  • Minor code standards: negation ! should have a space afterward (! empty, etc); last item in multiline array should be trailed by comma

#19 @keesiemeijer
7 months ago

I agree, flipping the value in get_category_parents() feels off and doesn't give it any meaning of what the value actually does . With a format argument we can give the meaning back and make it much more readable in both functions.

Why array_reverse() before generating the markup? Does get_ancestors() return values in a reverse order from the way that get_category_parents() currently does it?

Yes, get_ancestors() returns the terms from the child up, while get_category_parents() displays it from the last parent down.

A new patch is on its way :)

Last edited 7 months ago by keesiemeijer (previous) (diff)

@keesiemeijer
7 months ago

Add format argument and new unit test.

#20 @keesiemeijer
7 months ago

In patch 17069.2.patch the format argument is added and the functions are cleaned up to follow the code standards. I've also added a new unit test for terms without parents. I've ran the new test on the original get_category_parents() code as well to make sure it was returning the same value.

#21 @boonebgorges
7 months ago

@keesiemeijer Looks great! On closer inspection, I'm going to make a couple minor changes:

  1. It looks like, while it's not documented as such, the old get_category_parents() would accept a term object for $term_id. It's easy to add silent support for this in get_term_parents_list(), so let's do it.
  2. I'm adding a test to demonstrate the distant-to-nearest order issue you're solving with array_reverse().

#22 @boonebgorges
7 months ago

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

In 39549:

Taxonomy: Introduce get_term_parents_list().

This new function is a taxonomy-agnostic version of get_category_parents().

Props keesiemeijer, SergeyBiryukov, rafaehlers.
Fixes #17069.

#23 @keesiemeijer
7 months ago

Thanks @boonebgorges :)

#24 @keesiemeijer
6 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The function should use get_term_link() instead of get_category_link().

The difference between them is that if an error occurred, get_term_link() returns a WP_Error, where get_category_link() would return an empty string. Because we use the the term ids found by get_ancestors() no errors should occur when passing them to get_term_link().

@keesiemeijer
6 months ago

Replace get_category_link() with get_term_link()

#25 @boonebgorges
6 months ago

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

In 39593:

Taxonomy: Use get_term_link() instead of get_category_link() in get_term_parents_list().

get_category_link() is a wrapper for get_term_link(). Using the
unwrapped function makes more sense semantically (it's taxonomy-
agnostic) and it's also more parsimonious (the WP_Error check in
get_category_link() is redundant with similar checks just before
in get_term_link()).

Props keesiemeijer.
Fixes #17069.

Note: See TracTickets for help on using tickets.