WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 19 months ago

Last modified 4 months ago

#33460 closed enhancement (fixed)

wp_list_categories should display title_li only if the $categories is not empty , or have an option to hide it if $categories is empty

Reported by: vilkatis Owned by: boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.4
Component: Taxonomy Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

This part of wp_list_categories:

if ( $r['title_li'] && 'list' == $r['style'] ) {
$output = '<li class="' . esc_attr( $r['class'] ) . '">' . $r['title_li'] . '<ul>';
}

Should be

if ( $r['title_li'] && 'list' == $r['style'] && (!empty($categories))) {
$output = '<li class="' . esc_attr( $r['class'] ) . '">' . $r['title_li'] . '<ul>';
}

or is this intended behavior?

Attachments (4)

wp_list_categories.diff (1.3 KB) - added by vilkatis 19 months ago.
Updated category-template.php with the changes to wp_list_categories.
wp_list_categories-fixed.diff (1.3 KB) - added by vilkatis 19 months ago.
Was wrong about my first patch , changed and tested it , it works
33460.diff (3.6 KB) - added by boonebgorges 19 months ago.
33460.2.diff (3.7 KB) - added by vilkatis 19 months ago.
Changed the name of the argument to hide_title_if_empty in all the relevant places

Download all attachments as: .zip

Change History (16)

#1 follow-up: @boonebgorges
19 months ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

Thanks for the ticket, vilkatis. Welcome to Trac!

I'm not sure whether it's intended behavior, but it's been like this forever, and I think it'd cause problems to change it so that the title doesn't appear when there are no categories to show.

However, having an option to hide the title if $categories is empty is not a bad idea. Patches welcome.

@vilkatis
19 months ago

Updated category-template.php with the changes to wp_list_categories.

@vilkatis
19 months ago

Was wrong about my first patch , changed and tested it , it works

#2 @vilkatis
19 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

#3 in reply to: ↑ 1 @vilkatis
19 months ago

Replying to boonebgorges:

Thanks for the ticket, vilkatis. Welcome to Trac!

I'm not sure whether it's intended behavior, but it's been like this forever, and I think it'd cause problems to change it so that the title doesn't appear when there are no categories to show.

However, having an option to hide the title if $categories is empty is not a bad idea. Patches welcome.

Thank you !
I hope I did everything right , I have read the codex about patches.

#4 @boonebgorges
19 months ago

  • Milestone changed from Future Release to 4.4

Thanks, vilkatis! Patch looks pretty good. I'm thinking about the best name for this parameter. It seems to me that most of our existing params of this type begin with 'hide_' - 'hide_empty', 'hide_if_empty', etc. So maybe something like 'hide_title_if_no_cats', which would default to false? My only concern is that it feels a little like a double-negative. See 33460.diff, which adds unit tests.

@boonebgorges
19 months ago

#5 follow-up: @vilkatis
19 months ago

  • Keywords needs-testing removed

Yeah your code looks great , it really makes more sense naming it that way. I went over the unit tests to understand them and I think I got the idea of how those work.

What's the next step for me to do ?

#6 @boonebgorges
19 months ago

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

In 33764:

Introduce hide_title_if_no_cats parameter to wp_list_categories().

When generating a <ul> using wp_list_categories(), a title <li> element
is put at the top of the term list. Current behavior is that this title <li>
appears even when no terms are found. The new hide_title_if_no_cats param
allows developers to specify that the title should be hidden when the term list
is empty.

Props vilkatis.
Fixes #33460.

#7 @boonebgorges
19 months ago

In 33765:

After [33764], fix docblock formatting for wp_list_categories().

See #33460.

#8 in reply to: ↑ 5 @boonebgorges
19 months ago

Replying to vilkatis:

Yeah your code looks great , it really makes more sense naming it that way. I went over the unit tests to understand them and I think I got the idea of how those work.

What's the next step for me to do ?

Nothing. We're done here :) Thanks for the contribution!

#9 @DrewAPicture
19 months ago

  • Keywords 2nd-opinion added; good-first-bug removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

I'd like to suggest that we change the name of the new argument to hide_title_if_empty. This does two things:

  1. It's gets us away from pigeonholing the argument to categories, especially since this function can be used with other taxonomies
  2. It follows more closely with the naming scheme of other "empty"-related arguments.

On point #1, if/when we move to taxonomy-agnostic namespacing, this would be one less thing we'd have to account for if/when wp_list_categories() becomes a wrapper for a wp_list_terms() function, for instance. Plus I think it's just clearer all around.

@vilkatis
19 months ago

Changed the name of the argument to hide_title_if_empty in all the relevant places

#10 @boonebgorges
19 months ago

DrewAPicture - Thanks for chiming in. When patching, I'd originally used hide_title_if_empty, for the reasons you list, but I backed off because I felt that "empty" was ambiguous between an empty *title* and an empty *category list*. (An empty *title* is *never* shown.)

#11 @SergeyBiryukov
19 months ago

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

Fixed in [33767].

#12 @SergeyBiryukov
4 months ago

In 39280:

Taxonomy: Prevent wp_list_categories() from producing not well-nested output if hide_title_if_empty is true.

Props chesio.
Fixes #38839. See #33460.

Note: See TracTickets for help on using tickets.