Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#29839 closed enhancement (fixed)

Enhance get_terms to return only bottom child terms (terms without children) when taxonomy is heirarchical

Reported by: themiked's profile theMikeD Owned by:
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.0
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

In get_terms() currently there is an option called 'parent' that will return only top-level terms (meaning terms with no parent term). This request is for an option that does the opposite: get only bottom-level terms (meaning terms with no children).

Attachments (19)

29839.diff (2.4 KB) - added by horike 10 years ago.
taxonomy.diff (2.9 KB) - added by theMikeD 10 years ago.
Fix for #29839
20141112_01.diff (3.3 KB) - added by theMikeD 10 years ago.
20141112_02.diff (2.7 KB) - added by theMikeD 10 years ago.
Patch for #29839
taxonomy-20141113_01.diff (2.6 KB) - added by theMikeD 10 years ago.
Code fix for #29839
getTerms-20141113_01.diff (2.9 KB) - added by theMikeD 10 years ago.
unit test for #29839
getTerms-20141113_02.diff (2.9 KB) - added by theMikeD 10 years ago.
unit test for #29839; error_log removed
20141115_01-taxonomy.diff (3.2 KB) - added by theMikeD 10 years ago.
Fix for #29839
20141115_01-getTerms.diff (2.9 KB) - added by theMikeD 10 years ago.
unit test for #29839
20141115_02-taxonomy.diff (3.2 KB) - added by theMikeD 10 years ago.
Fix for #29839
20141115_02-getTerms.diff (2.9 KB) - added by theMikeD 10 years ago.
unit test for #29839 with updated option name
20141115_03-taxonomy.diff (3.1 KB) - added by theMikeD 10 years ago.
Fix for #29839: changed option name, fixed typos
20141115_03-getTerms.diff (2.9 KB) - added by theMikeD 10 years ago.
Unit test for #29839: changed option name, fixed typos
20141116_01-taxonomy.diff (3.1 KB) - added by theMikeD 10 years ago.
Fix for #29839: added typecast to boolean
20141116_01-taxonomy.2.diff (2.6 KB) - added by theMikeD 10 years ago.
Patch for #29839: removed check for hierarchical taxonomy
20141116_01-getTerms.diff (5.1 KB) - added by theMikeD 10 years ago.
Broken unit test for 29839
20141116_03-getTerms.diff (5.3 KB) - added by theMikeD 10 years ago.
20141116_05-getTerms.diff (5.3 KB) - added by theMikeD 10 years ago.
unit test for #29839; hierarchical and flat tests are split
20141201_01-taxonomy.diff (2.6 KB) - added by theMikeD 10 years ago.
Updated doc block

Download all attachments as: .zip

Change History (84)

#1 @SergeyBiryukov
10 years ago

  • Description modified (diff)
  • Type changed from feature request to enhancement

#2 @boonebgorges
10 years ago

  • Keywords needs-patch good-first-bug added

#3 @theMikeD
10 years ago

I'd actually like to take a stab at fixing this one myself. Can someone point me to authoritative steps on how I can go about this?

#4 @boonebgorges
10 years ago

theMikeD - Cool, that'd be great. Not sure about "authoritative steps", but you will probably find https://make.wordpress.org/core/handbook/handbook/ useful, perhaps especially https://make.wordpress.org/core/handbook/fixing-bugs/

#5 follow-up: @McGuive7
10 years ago

Hey theMikeD,

I'm curious about this as well. Can you clarify what you mean by "only bottom level terms"? Say you've the following terms

- Term 1
- Term 2
- - Term 1a
- - Term 1b

Based on your description, it seems like it could return one of two things:

  1. All terms with no children, which would be: Term 1, Term 1a, and Term 1b.
  2. Just top-level terms with no children: Term 1

Are you thinking that it would do one or both (or something else entirely)? What's the use case for this?

#6 @TimothyBlynJacobs
10 years ago

We've needed this for a project. Right now we are going to write the query manually.

We need it to return all leaf nodes, so all terms with no children.

Various use cases for this exist. In our use case we need it because we need to present a drop-down to a user and for our data it would only make sense for them to select leaf nodes.

#7 in reply to: ↑ 5 @theMikeD
10 years ago

Replying to McGuive7:

Hey theMikeD,

I'm curious about this as well. Can you clarify what you mean by "only bottom level terms"? Say you've the following terms

- Term 1
- Term 2
- - Term 1a
- - Term 1b

Based on your description, it seems like it could return one of two things:

  1. All terms with no children, which would be: Term 1, Term 1a, and Term 1b.

This.

  1. Just top-level terms with no children: Term 1

Not this.

Are you thinking that it would do one or both (or something else entirely)? What's the use case for this?

My use case is with an hierarchical taxonomy and showing a different page when we are at a terminal node as opposed to a node with children.

#8 @theMikeD
10 years ago

BTW "leaf nodes" is a much better description, and is the one I'll use from now on.

@horike
10 years ago

#9 @horike
10 years ago

  • Keywords has-patch added; needs-patch removed

#10 @theMikeD
10 years ago

Guess I won't be doing it after all.

#11 @boonebgorges
10 years ago

  • Keywords needs-unit-tests added

horike - Thanks for the patch. A couple initial thoughts (for you and/or theMikeD :) ):

  • The proposed 'child' appears to do a bit more than the original suggestion. The idea was to get leaf nodes, but 'child' will only get leaf nodes of a given ancestor. How about separating the leaf logic into its own parameter, which could then be combined with 'child_of' to accomplish the more specific filtering?
  • Function documentation will need updating.
  • Needs unit tests.

#12 @theMikeD
10 years ago

Here is what I have found.

_get_term_hierarchy returns an array of arrays, and each of these sub-arrays is a termID-indexed list of that terms children; said another way, each sub-array contains the children of a given term. Crucially it does not list terms without children, so this can be used as the test for finding which terms are leaf terms: any term that does not have its own array has no children and is by definition a leaf term.

So all we need to do is get a list of all terms, and any term that does not have an array in the results of _get_term_hierarchy is a leaf term. This is what I've done with the attached patch.

I've also made it fail if the taxonomy is not hierarchical as a non-hierarchical taxonomy would return all terms in this case.

Thoughts?

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

@theMikeD
10 years ago

Fix for #29839

#13 follow-up: @boonebgorges
10 years ago

theMikeD - Thanks for the updated patch and for your continued attention to this ticket. The general technique of checking _get_term_hierarchy() seems smart to me.

Instead of calling get_terms() again after identifying the leaf nodes, we should be appending the IDs to the $inclusions array, to keep the logical flow of the function. And actually, now that I think about it: the number of non-leaf nodes in most trees is likely to be lower than the number of leaves, so it might make sense to identify the *non* leaves and add them to $exclusions instead.

I'll also feel better about this when we have some unit tests that demonstrate that the technique works for hierarchies that have more than 2 levels :)

#14 @theMikeD
10 years ago

Thanks boonebgorges, I'll take some more time with it tomorrow. I'm testing this with a "location" custom post type of country-state-city-district so I'm certain it'll work with any level, but I'd be happy to do a unit test if you can point me at some docs on how I'd go about doing that. Would it be helpful to write out the unit tests in logic here first, or just do them and address omissions at a later date?

#15 in reply to: ↑ 13 @theMikeD
10 years ago

Replying to boonebgorges:

Instead of calling get_terms() again after identifying the leaf nodes, we should be appending the IDs to the $inclusions array, to keep the logical flow of the function. And actually, now that I think about it: the number of non-leaf nodes in most trees is likely to be lower than the number of leaves, so it might make sense to identify the *non* leaves and add them to $exclusions instead.

That is better: any term listed as a top-level entry in the array returned by _get_term_hierarchy() is a non-leaf term by definition, so can be excluded. That would skip the need to call get_terms() at all. Clever, I missed the $exclusions array the first time around.

#16 @boonebgorges
10 years ago

I'd be happy to do a unit test if you can point me at some docs on how I'd go about doing that. Would it be helpful to write out the unit tests in logic here first, or just do them and address omissions at a later date?

Sure thing. Here's a guide to getting set up with PHPUnit: https://make.wordpress.org/core/handbook/automated-testing/ As for the tests themselves, have a look at the existing tests for get_terms(): https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/term/getTerms.php. Find one that does something similar to what you are trying to test, then copy and modify it :) I'm happy to lend a hand with that if you hit a snag.

#17 @theMikeD
10 years ago

Updated patch is attached. I'll look at the unit tests + more robust testing in the next few days. Thanks for the tips boonebgorges!

@theMikeD
10 years ago

Patch for #29839

#18 @theMikeD
10 years ago

Some extra stuff got into 20141112_01.diff so I reset my repo and re-diffed. Sorry about that.

#19 follow-up: @theMikeD
10 years ago

OK, I have written the unit tests and updated the code patch. Is it normal for a bug to have multiple patches assigned to a bug/feature? Are the old ones deleted?

@theMikeD
10 years ago

Code fix for #29839

@theMikeD
10 years ago

unit test for #29839

@theMikeD
10 years ago

unit test for #29839; error_log removed

#20 @theMikeD
10 years ago

  • Keywords needs-unit-tests removed

#21 in reply to: ↑ 19 @helen
10 years ago

Replying to theMikeD:

Is it normal for a bug to have multiple patches assigned to a bug/feature? Are the old ones deleted?

Totally normal - seeing the progression of work on a ticket is a good thing. A thing to note is that attachments don't fire any notifications, so as you upload new patches, it's a good idea to leave a comment about it and explain what's going with it so testers and committers know what to look at and for.

#22 @theMikeD
10 years ago

Ah, thanks Helen, that's good to know.

So then: the patch addresses the issue in a way that preserves child_of and hide_empty. It's over-ridden by all and it's using the $exclusions list as suggested by boonebgorges. This eliminates the call to get_terms that the first patch had.

The unit test function creates a 4-level custom taxonomy and tests for the new option (children_only) along with child_of and hide_empty. There are three tests total. The unit tests all pass.

#23 @theMikeD
10 years ago

Can I be set as the owner of this one?

#24 @theMikeD
10 years ago

This is the proposed addition to the get_terms documentation. I don't know how to do a diff on a doc so I'll just describe the changes.


Default Usage

'children_only' => false,


Possible Arguments

children_only
(boolean) The opposite of parent: returns terms that have no children. Null if taxonomy is not hierarchical.

#25 @boonebgorges
10 years ago

Thanks for the additional patches, theMikeD.

I'd like for the unit tests to be a bit more specific. You're testing count() values, but it's far more accurate (ie less likely to result in false positives) if you assemble an array and use assertEqualSets(). Something like:

$terms = get_terms( $tax, array( 'children_only' => true, 'hide_empty' => false, 'child_of' => $quebec );
$this->assertEqualSets( array( $montreal ), wp_list_pluck( $terms, 'term_id' ) ); 

I also wanted to ask about the parameter name 'children_only'. To me, this is a bit misleading: 'children_only' suggests to me that it'll return any item that has a 'parent', but what you're *really* querying is items that have no children. Something like 'childless' or 'leaf' or 'terminal' seems more precise. See https://en.wikipedia.org/wiki/Tree_%28data_structure%29#Terminology.

For documentation - what I mean is the function docblock, though an update to the codex will be most welcome as well :)

#26 @boonebgorges
10 years ago

  • Keywords 4.2-early added
  • Milestone changed from Awaiting Review to Future Release

#27 @theMikeD
10 years ago

I'm also concerned about children_only but the rest of the codex and the other relevant options all use the child-parent metaphor, so I'm not sure that introducing 'leaf node' would fit in.

Some suggestions:
terminal_children sounds grim though
leaf_children mixed metaphor, but still
childless
childless_terms This would be my pick
childless_only

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

#28 @theMikeD
10 years ago

OK, two new patches.

First, the unit tests were updated as advised.

Second, the option name is now childless_terms The doc block is in there too, if I found the correct spot.

@theMikeD
10 years ago

unit test for #29839

#29 @theMikeD
10 years ago

Updated the taxonomy.php patch to actually set the default value to false (up until now it was set to '')

@theMikeD
10 years ago

unit test for #29839 with updated option name

#30 follow-up: @boonebgorges
10 years ago

Cool, thanks, theMikeD. I think this is pretty close, though I'm going to nitpick a little more since 4.1 is in beta so we'll have to wait for 4.2, meaning there's no rush.

  • I think the '_terms' part of 'childless_terms' is redundant. (We don't say 'parent_term' or 'term_is_child_of', etc.)
  • You got the right place for the docblock :) I don't think it's correct to say that 'childless' is the opposite of 'parent', because 'parent' takes a term ID and returns only children of the parent, while 'childless' doesn't care what part of the taxonomy tree you're in and takes a boolean argument.
  • You bail out of the function with an empty array if the taxonomy hierarchy is empty. Why? (This is kind of a trick question: a couple other parameters do this, and it's not totally obvious why they do it either.) If it's intended to save database queries, or prevent false positives, or whatever, it would be nice to see a unit test that demonstrates it. Alternatively, if that block doesn't affect anything, we should tear it out.
  • When you are building the $exclusions string, make sure you run each item through intval() - better safe than sorry when building SQL queries.
  • It's spelled "hierarchical" and "hierarchy" :)

#31 in reply to: ↑ 30 @theMikeD
10 years ago

Replying to boonebgorges:

  • I think the '_terms' part of 'childless_terms' is redundant. (We don't say 'parent_term' or 'term_is_child_of', etc.)

True. How about just childless then?

  • You got the right place for the docblock :) I don't think it's correct to say that 'childless' is the opposite of 'parent', because 'parent' takes a term ID and returns only children of the parent, while 'childless' doesn't care what part of the taxonomy tree you're in and takes a boolean argument.

I'll remove it for sure. I had a misunderstanding in my head on what parent did.

  • You bail out of the function with an empty array if the taxonomy hierarchy is empty. Why? (This is kind of a trick question: a couple other parameters do this, and it's not totally obvious why they do it either.) If it's intended to save database queries, or prevent false positives, or whatever, it would be nice to see a unit test that demonstrates it. Alternatively, if that block doesn't affect anything, we should tear it out.

I bomb out for the same reason that parent does: there is no such thing as a child term if there is no hierarchy. If I didn't then this option would be the same as all which doesn't sit with me.

  • When you are building the $exclusions string, make sure you run each item through intval() - better safe than sorry when building SQL queries.

This would only be the case if _get_term_hierarchy() somehow returned non-IDs, right? I don't find that likely, but I'll add the intval() wrapper to be safe.

  • It's spelled "hierarchical" and "hierarchy" :)

And one day, I'm certain I'll spell it that way too!

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

@theMikeD
10 years ago

Fix for #29839: changed option name, fixed typos

@theMikeD
10 years ago

Unit test for #29839: changed option name, fixed typos

#32 @theMikeD
10 years ago

Updated codex info for this option


Default Usage

'childless' => false,


Possible Arguments

childless
(boolean) Returns terms that have no children. Null if taxonomy is not hierarchical.

#33 @theMikeD
10 years ago

I have another question.

$childless = $args['childless']; // boolean

is in the code I added to taxonomy.php but I don't enforce the fact that $args['childless'] is in fact boolean. Is that handled somewhere or is it safe to assume that !false or !0 is true with the boolean options? I based this on $args['hierarchical'] which also doesn't check this.

#34 follow-up: @boonebgorges
10 years ago

I bomb out for the same reason that parent does: there is no such thing as a child term if there is no hierarchy. If I didn't then this option would be the same as all which doesn't sit with me.

Hm. In cases where the hierarchy is flat, technically *all* nodes are leaf nodes. So my inclination would be to return all items.

Is that handled somewhere or is it safe to assume that !false or !0 is true with the boolean options? I based this on $argshierarchical? which also doesn't check this.

It's not handled anywhere that I can see. This code is quite old and not necessarily up to standards, so don't use it as a model here. That being said, I think it's OK to accept 0 as an option here - just cast to bool at some point.

#35 in reply to: ↑ 34 @theMikeD
10 years ago

Replying to boonebgorges:

I bomb out for the same reason that parent does: there is no such thing as a child term if there is no hierarchy. If I didn't then this option would be the same as all which doesn't sit with me.

Hm. In cases where the hierarchy is flat, technically *all* nodes are leaf nodes. So my inclination would be to return all items.

There's def. two ways to look at it. Personally I think returning all terms would lead to confusing results for users who accidentally run this option on a non-hierarchical taxonomy. How are such differences of opinion handled in a situation like this? In any case, it's an easy fix: change

	$childless = (bool) $args['childless']; // boolean
	if ( $childless ) {
		// Empty if taxonomy is not hierarchical; term ids if it is.
		// Note that we are not checking for a specific child or parent, just the
		//  existence of an hierarchical taxonomy
		$hierarchy = _get_term_hierarchy( reset( $taxonomies ) );
		if ( empty( $hierarchy )) {
			return $empty_array;
		}
	}

to

	$childless = (bool) $args['childless']; // boolean
	if ( $childless ) {
		// Empty if taxonomy is not hierarchical; term ids if it is.
		// Note that we are not checking for a specific child or parent, just the
		//  existence of an hierarchical taxonomy
		$hierarchy = _get_term_hierarchy( reset( $taxonomies ) );
		if ( empty( $hierarchy )) {
			$args['get'] = 'all'; // This is the change
		}
	}

and move it up above the check for all around line 1663. I may even be able to remove the check altogether, but I'd have to test that first. The unit test would be to create a flat taxonomy and compare all with childless so it's easy enough to confirm.

It's not handled anywhere that I can see. This code is quite old and not necessarily up to standards, so don't use it as a model here. That being said, I think it's OK to accept 0 as an option here - just cast to bool at some point.

I think the best spot to do this would be here:

$childless = $args['childless']; // boolean

changing it to

$childless = (bool) $args['childless']; // boolean

According to the PHP info ( http://php.net/manual/en/language.types.boolean.php ) this is done automatically when a var is used in boolean context (as this one is) but, like the intval done earlier, better safe right?

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

@theMikeD
10 years ago

Fix for #29839: added typecast to boolean

#36 @boonebgorges
10 years ago

Personally I think returning all terms would lead to confusing results for users who accidentally run this option on a non-hierarchical taxonomy.

Imagine a hierarchical taxonomy with 27 terms: A-Z (all top-level) and A_child (child of A). get_terms() with childless=true would return B-Z + A_child. Now imagine you deleted A_child. Expected behavior, IMO, would be to return A-Z, but on your suggestion it'd return nothing. I'm not too concerned with what happens to people using this argument on a non-hierarchical taxonomy, but even in that case, returning all items would be the semantically correct thing to do.

I may even be able to remove the check altogether, but I'd have to test that first. The unit test would be to create a flat taxonomy and compare all with childless so it's easy enough to confirm.

Yes, I think we should do this.

Boolean typecast looks good.

#37 @theMikeD
10 years ago

Interesting case. Further, the two use cases in this ticket (mine and TimothyBlynJacobs') both support your interpretation. Strangely, it still doesn't feel correct to me, but I'll do it anyway as, especially in my case, i'd want it to return all terms. Give me a bit.

#38 @theMikeD
10 years ago

[Edited]

False alarm. Still working on it.

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

@theMikeD
10 years ago

Patch for #29839: removed check for hierarchical taxonomy

@theMikeD
10 years ago

Broken unit test for 29839

#39 @theMikeD
10 years ago

I've attached the updated working unit test. Is there an option on $this->factory->term->create to return the term object rather than the id? wp_set_post_terms() expects parameter 2 to be a string when the taxonomy is flat and it would be nice to avoid the three calls to get_term()

#40 follow-ups: @boonebgorges
10 years ago

  • Keywords good-first-bug removed

Is there an option on $this->factory->term->create to return the term object rather than the id?

$this->factory->term->create_and_get() should work. But, I think the problem is not that wp_set_object_terms() expects slugs for non-hierarchical taxonomies; I think it's that the term_exists() check in that function will only work for a term_id if it's passed as an integer, and you were probably passing '5' instead of 5. Just a guess :)

One last (I think last!) request. We generally try to avoid single unit test methods that do so much assertion. In the cases where you're using the same data, go ahead and keep the methods together. But when you switch to the non-hierarchical taxonomy, could you please start a new test method for that? So something like test_childless_for_hierarchical_taxonomy() and test_childless_for_non_hierarchical_taxonomy(). Then I think we're pretty much good, and we'll get this in as soon as 4.1 has been released. Thanks for your work on it!

#41 @theMikeD
10 years ago

You replied before I could post the patch :)

wp_set_object_terms() requires that a non-hierarchical taxonomy pass slugs, not IDs, and I was passing IDs. The fix is to use get_term() to get the object for the created term and use that from that point on.

I'll break up the two tests and post the patch shortly.

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

@theMikeD
10 years ago

unit test for #29839; hierarchical and flat tests are split

#42 @theMikeD
10 years ago

Updated unit tests attached. My get_term() comment will now make sense.

#43 in reply to: ↑ 40 ; follow-up: @theMikeD
10 years ago

Replying to boonebgorges:

Is there an option on $this->factory->term->create to return the term object rather than the id?

$this->factory->term->create_and_get() should work.

Can't find this. I see $this->factory->tag->create_and_get() though. Maybe you were thinking of that?

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

#44 in reply to: ↑ 43 @boonebgorges
10 years ago

Replying to theMikeD:

Replying to boonebgorges:

Is there an option on $this->factory->term->create to return the term object rather than the id?

$this->factory->term->create_and_get() should work.

Can't find this. I see $this->factory->post->create_and_get() though. Maybe you were thinking of that?

All the factory classes inherit create_and_get() from WP_UnitTest_Factory_For_Thing. https://core.trac.wordpress.org/browser/tags/4.0/tests/phpunit/includes/factory.php#L299

#45 @theMikeD
10 years ago

$brazil =    $this->factory->term->create_and_get( array( 'name' => 'Brazil',  'taxonomy' => $flat_tax ) );
error_log( print_r( $brazil, true));

gives a null response, and from the look of it it shouldn't. In any case, there is little difference between using this one (once I get it to work) and doing the extra step of getting the term object manually, so I think we're probably done.

#46 in reply to: ↑ 40 @theMikeD
10 years ago

Replying to boonebgorges:

Then I think we're pretty much good, and we'll get this in as soon as 4.1 has been released. Thanks for your work on it!

Thanks for your guidance, it really made the difference. Can't wait to find another one to work on!

#47 @valendesigns
10 years ago

I know this is a little late in the progression of the ticket, but using childless imo still sounds strange. I may be alone in that thought. If I'm not, I propose using last_ancestor or something in that vein.

#48 @theMikeD
10 years ago

Yes, you're a little late :)

The parent-child metaphor is in use already in get_terms() so I think we should stick with what we have.

#49 @valendesigns
10 years ago

@theMikeD From what I can tell, you've only added the ability to display the bottom level and not the parent tree if you pass in an ID. Shouldn't this truly be the opposite of parent? For example, the docs would read like this if it were:

child
(integer) Get direct parents of this term (only terms whose explicit child is this value). If 0 is passed, only bottom-level terms are returned. Default is an empty string.

#50 @theMikeD
10 years ago

I'm not after parent terms at all.

#51 follow-up: @boonebgorges
10 years ago

childless imo still sounds strange. I may be alone in that thought. If I'm not, I propose using last_ancestor or something in that vein.

I also think that 'childless' is maybe not the ideal name, but it's accurate. 'last_ancestor' suggests that we're only looking at a single branch in the tree, which we're not.

From what I can tell, you've only added the ability to display the bottom level and not the parent tree if you pass in an ID. Shouldn't this truly be the opposite of parent?

I can see the use of a parameter like this, but it'd be a totally separate enhancement from 'childless'.

#52 in reply to: ↑ 51 @valendesigns
10 years ago

Replying to theMikeD:

I'm not after parent terms at all.

I understand where you are coming from and you may not be after the parent terms, but others might. If we're going to add another parameter to get_terms(), shouldn't it at least be useful in more situations? Not just for one limited scenario where you add false to get the bottom terms. What is the utility here? I'm not trying to be argumentative for the sake of it, I'm just having a hard time seeing the value in this change as it's current presented. Could you please give me an example of how other developers could leverage this enhancement so I understand your point of view better?

Replying to boonebgorges:

I can see the use of a parameter like this, but it'd be a totally separate enhancement from 'childless'.

In the description of this ticket it describes adding a parameter that does the opposite of parent, so shouldn't it live up to that a little bit more? As well, if we went that route and added a separate parameter (new ticket) wouldn't we be breaking up what is essentially one function set, as parent has already proven? If this stays named childless and we decide to proceed at some point in the future with that second enhancement merging the two would be confusing at best.

#53 follow-ups: @boonebgorges
10 years ago

In the description of this ticket it describes adding a parameter that does the opposite of parent, so shouldn't it live up to that a little bit more?

I think that phrase in the original ticket description was ill-chosen. What the discussion quickly turned to was the fetching of "leaf" or "terminal" nodes. See eg https://core.trac.wordpress.org/ticket/29839#comment:6

If this stays named childless and we decide to proceed at some point in the future with that second enhancement merging the two would be confusing at best.

There would be no merging, just the possible addition of separate parameters. I guess 'child' would take an integer and return an array with a maximum of one member, which would be the direct parent (since 'parent' returns only direct descendants - though tbh I find this naming convention to be pretty confusing). 'ancestor_of' would take an integer and return an array of all ancestors. In any case, these params have very different use cases from what's described in the latest patch, and the two are not at odds with each other. If you have a use case for one of these other ideas, please feel free to open a separate ticket describing that case, with a patch if you wish to provide one.

#54 in reply to: ↑ 53 @valendesigns
10 years ago

Replying to boonebgorges:

If you have a use case for one of these other ideas, please feel free to open a separate ticket describing that case, with a patch if you wish to provide one.

I'm wonder what the use case is to begin with, because I don't have a use case for either of them. I'll leave it alone though. I just wanted to be the voice of objectivity. Obviously I came into this ticket late, but that's due to being new on the Trac. I was getting caught up reading emails from the firehose and this jumped out at me because of the odd parameter name and I thought I should speak my mind before it gets committed into the core, as it will likely be that way for a long time. My other comments are just me getting up to speed and wondering why the ticket wasn't taken a bit further for the sake of symmetry. Anyhow, thanks for listening.

Cheers,
Derek

#55 in reply to: ↑ 53 @theMikeD
10 years ago

I guess 'child' would take an integer and return an array with a maximum of one member, which would be the direct parent (since 'parent' returns only direct descendants ...

You can get this already: use get_term( $id, $tax, OBJECT ) and then check the parent parameter.

... though tbh I find this naming convention to be pretty confusing).

I do as well. IMHO parent is doing two different things ( returning all parent items, or returning children of a supplied term ) and should have been written as two options. I don't think repeating that decision here is a good idea, regardless of how my original summary is phrased and especially because that functionality already exists in get_term().

#56 @theMikeD
10 years ago

The updated codex docs for this option. I noticed that it still made reference to the return of null with non-hierarchical tax, so that's fixed.

Updated codex info for this option


Default Usage

'childless' => false,


Possible Arguments

childless
(boolean) Returns terms that have no children if taxonomy is hierarchical, all terms if taxonomy is not hierarchical.

@theMikeD
10 years ago

Updated doc block

#57 @theMikeD
10 years ago

I'm unfamiliar with the process of what happens now, or what the status is of this one. Do I need to do anything to get this into 4.2?

#58 follow-ups: @johnbillion
10 years ago

  • Keywords needs-unit-tests 2nd-opinion added

This will need unit tests.

Also, the foreach loop in the patch can be replaced with something simpler, like this:

$terms_to_exclude = implode( ', ', array_map( 'absint', array_keys( $term_hierarchy ) ) );

Finally, I'm not entirely convinced this argument is very useful. It can be manually implemented with minimal code:

$terms = get_terms( $taxonomy, array(
  'exclude' => array_keys( _get_term_hierarchy( $taxonomy ) )
) );

#59 @theMikeD
10 years ago

Replying to johnbillion:

This will need unit tests.

The unit tests are attached to this ticket already. The most recent attachment that covers the unit tests is 20141116_05-getTerms.diff. Did you mean something else?

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

#60 in reply to: ↑ 58 @theMikeD
10 years ago

Replying to johnbillion:

Also, the foreach loop in the patch can be replaced with something simpler, like this:

$terms_to_exclude = implode( ', ', array_map( 'absint', array_keys( $term_hierarchy ) ) );

This seems neither simpler nor clearer to me. Is there a performance issue with how it was done in the submitted patch?

Finally, I'm not entirely convinced this argument is very useful. It can be manually implemented with minimal code:

$terms = get_terms( $taxonomy, array(
  'exclude' => array_keys( _get_term_hierarchy( $taxonomy ) )
) );

_get_term_hierarchy is marked as a private function. Doesn't that mean it shouldn't be used in userland?

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

#61 in reply to: ↑ 58 ; follow-up: @boonebgorges
10 years ago

  • Keywords 4.2-early needs-unit-tests 2nd-opinion removed
  • Milestone changed from Future Release to 4.2

Replying to johnbillion:

Finally, I'm not entirely convinced this argument is very useful. It can be manually implemented with minimal code:

$terms = get_terms( $taxonomy, array(
  'exclude' => array_keys( _get_term_hierarchy( $taxonomy ) )
) );

The same argument could be made for all parameters related to term hierarchy - child_of, parent, exclude_tree, etc could all be implemented using include/exclude, by making reference to _get_term_hierarchy(). We provide these params as a convenience for two reasons. (1) Doing everything with 'include' and 'exclude' means that you need to do various array_intersect() tricks if you want to do anything of moderate complexity (which would otherwise be accomplished by using standalone arguments together). (2) As theMikeD notes, _get_term_hierarchy() is part of our internal cache implementation, which plugin devs should be free (encouraged!) to ignore.

Your foreach suggestion seems fine to me.

I can think of a number of legit use cases for wanting to pull up terminal nodes in a taxonomy hierarchy. So unless there's a pressing argument against it, let's go with this param for 4.2. Leaving open for the time being because I'm still not bonkers about the 'childless' naming convention.

#62 in reply to: ↑ 61 @theMikeD
10 years ago

Replying to boonebgorges:

Leaving open for the time being because I'm still not bonkers about the 'childless' naming convention.

I'd like nothing more than to reset the option names for get_terms for consistency but we all know that will never happen. So we work with what we have.

get_terms is using mixed metaphors in its options: parent and child_of for parent-child metaphor, and exclude_tree for the tree metaphor. childless seems clear to me, but even if we switch it to something like leaf or leaf_only it's not going to change the fact that we can't turn only_terms_that_have_no_children into a one or two word option that will be 100% clear to everyone. That's what the docs are for.

Changing it at this point seems to be a distinction without a difference. Having said that, I'm not the gatekeeper so if it's decided that childless is not good, then lets sort out what *is* good. I've provided some alternatives in comment 27 and went with the best of that bunch. If anyone has any alternatives, I'm all ears. But lets be sure, because I don't want to have to do an option name change for a fourth time if it can be avoided.

#63 @boonebgorges
10 years ago

I'm going to move forward with this. A few changes I'm making to the most recent patches:

  • break up unit tests into smaller pieces, and eliminate some redundancies (I don't see a need to test alongside 'hide_empty' variants)
  • change the way the $exclusions clause is built, so that it compiles an array and translates it into a SQL string one time, rather than stringing together little bits of SQL
  • ensure that 'childless' respects all passed $taxonomies, not just the first one

#64 @theMikeD
10 years ago

All good ideas. I'm interested to see the changes.

#65 @SergeyBiryukov
10 years ago

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

[31275] missed the ticket.

Note: See TracTickets for help on using tickets.