Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 8 years 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's profile vilkatis Owned by: boonebgorges's profile 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 10 years ago.
Updated category-template.php with the changes to wp_list_categories.
wp_list_categories-fixed.diff (1.3 KB) - added by vilkatis 10 years ago.
Was wrong about my first patch , changed and tested it , it works
33460.diff (3.6 KB) - added by boonebgorges 10 years ago.
33460.2.diff (3.7 KB) - added by vilkatis 10 years 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
10 years 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
10 years ago

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

@vilkatis
10 years ago

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

#2 @vilkatis
10 years ago

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

#3 in reply to: ↑ 1 @vilkatis
10 years 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
10 years 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
10 years ago

#5 follow-up: @vilkatis
10 years 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
10 years 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
10 years ago

In 33765:

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

See #33460.

#8 in reply to: ↑ 5 @boonebgorges
10 years 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
10 years 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
10 years ago

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

#10 @boonebgorges
10 years 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
10 years ago

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

Fixed in [33767].

#12 @SergeyBiryukov
8 years 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.