WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 12 months ago

#10676 closed enhancement (fixed)

current-cat-ancestor in wp_list_categories

Reported by: spathon Owned by: boonebgorges
Milestone: 4.5 Priority: normal
Severity: normal Version: 2.8.5
Component: Taxonomy Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

The wp_list_categories should apply current-cat-ancestor like wp_list_pages not just current-cat-parent to the closest parent

Attachments (5)

category-template-10676.diff (752 bytes) - added by ardathksheyna 3 years ago.
10676.diff (799 bytes) - added by wonderboymusic 3 years ago.
10676_short_circuit.diff (822 bytes) - added by jrchamp 2 years ago.
10676_wordpress_4_4.diff (894 bytes) - added by jrchamp 12 months ago.
10676.2.diff (2.2 KB) - added by swissspidy 12 months ago.

Download all attachments as: .zip

Change History (22)

#1 @spathon
7 years ago

  • Milestone changed from Future Release to 2.9
  • Version changed from 2.8.4 to 2.8.5

#2 @azaozz
7 years ago

  • Milestone changed from 2.9 to Future Release

No patch.

#3 @ardathksheyna
3 years ago

  • Cc jgreen@… added

#4 @ardathksheyna
3 years ago

  • Keywords has-patch needs-testing added

#5 @DrewAPicture
3 years ago

  • Component changed from General to Taxonomy
  • Keywords class categories removed

@wonderboymusic
3 years ago

#6 @wonderboymusic
3 years ago

  • Milestone changed from Future Release to 3.7

10676.diff​ rehabs the whitespace

#7 @nacin
3 years ago

Two considerations here:

  • Are the ancestor queries OK and worth it?
  • Does the class name we're going with make sense?

#8 @nacin
3 years ago

  • Type changed from feature request to enhancement

#9 @nacin
3 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from 3.7 to Future Release

Punting pending answers to the two questions.

#10 @jrchamp
2 years ago

The class name makes sense. It's the logical conclusion given that I reimplemented it before finding this ticket.

I dislike the use of get_ancestors() because it has extra overhead, we already have the first term and only need to check the terms until the point that it matches. I'm attaching a modified version of this patch that doesn't waste time array_reversing and leverages the short circuit opportunities to often avoid the get_ancestors() call altogether.

Really, given the inherent static response given by the get_ancestors() call for the provided current_category, if the Walker_Category object initialized a private member variable with the get_ancestors() array the first time it was needed, you could easily use a simple isset on the member variable on all subsequent calls. Additionally, if this path were pursued I would recommend an array_flip to allow use of the isset language construct on the array of ancestor IDs itself rather than the much slower in_array() function.

#11 @SergeyBiryukov
2 years ago

#30635 was marked as a duplicate.

#13 @swissspidy
12 months ago

  • Keywords needs-refresh needs-unit-tests added; needs-testing removed

#14 @jrchamp
12 months ago

  • Keywords needs-refresh removed

Refreshed for 4.4

#15 @boonebgorges
12 months ago

  • Keywords 2nd-opinion removed

The technique in 10676_wordpress_4_4.diff looks good to me. Improved cache support for get_term() in 4.4 should minimize the overhead of walking the ancestor tree like this.

Can we get tests for this? At least one, which would show the class being applied to multiple levels of ancestor. (We test these classes via wp_list_categories(): https://core.trac.wordpress.org/browser/tags/4.4/tests/phpunit/tests/category/wpListCategories.php

@swissspidy
12 months ago

#16 @swissspidy
12 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Milestone changed from Future Release to 4.5

10676.2.diff is a new patch containing a unit test for the new class.

#17 @boonebgorges
12 months ago

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

In 36008:

Add current-cat-ancestor class to ancestor items in wp_list_categories().

Pairs nicely with current-cat-parent.

Props jrchamp, swisssipdy, ardathksheyna, wonderboymusic.
Fixes #10676.

Note: See TracTickets for help on using tickets.