WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#29460 closed defect (bug) (fixed)

Change the default orderby in wp_get_nav_menus()

Reported by: pento Owned by: SergeyBiryukov
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.0
Component: Menus Keywords: has-patch needs-unit-tests needs-docs
Focuses: Cc:

Description

In #28126, we changed the orderby that is passed to wp_get_nav_menus() to name. For a more complete solution, we should make this the default orderby in wp_get_nav_menus(), rather than none.

Attachments (4)

29460.diff (1.1 KB) - added by voldemortensen 4 years ago.
29460.revised.diff (2.0 KB) - added by voldemortensen 4 years ago.
29460.patch (3.1 KB) - added by igmoweb 4 years ago.
Unit tests included
29460.2.diff (560 bytes) - added by curtjen 4 years ago.

Download all attachments as: .zip

Change History (15)

@voldemortensen
4 years ago

#1 @voldemortensen
4 years ago

Removed changes made by #28126 and changed the default orderby to 'name'.

#2 @pento
4 years ago

  • Keywords has-patch needs-unit-tests added

Nice. There are a few other places in core where wp_get_nav_menus() is called with 'orderby' => 'name' (which can be removed), or called without an orderby parameter (which need to be tested, to make sure they don't break existing behaviour).

This could also do with some unit tests.

#3 @voldemortensen
4 years ago

Removed all the instances of 'orderby' => 'name' I could find passed in wp_get_nav_menus(). Kept the default at 'orderby' => 'name'.

#4 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 4.1

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.


4 years ago

@igmoweb
4 years ago

Unit tests included

#6 @SergeyBiryukov
4 years ago

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

In 29792:

Change the default orderby value in wp_get_nav_menus() to 'name'.

props voldemortensen, igmoweb.
fixes #29460.

#7 @DrewAPicture
4 years ago

  • Keywords needs-docs added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry, late to the party here.

I'd like to see an additional @since 4.1.0 tag added to the docblock for wp_get_nav_menus() specifying that the default 'orderby' argument value changed from 'none' to 'name'.

@curtjen
4 years ago

#8 @curtjen
4 years ago

Added code documentation for changing "none" to "name" for wp_get_nav_menus().

#9 @DrewAPicture
4 years ago

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

In 29796:

Update documentation for wp_get_nav_menus() to reflect that the default value for the the 'orderby' argument was changed from 'none' to 'name' in 4.1.

Props curtjen.
Fixes #29460.

#10 @nacin
4 years ago

I'd consider this change to be internal enough to not warrant a special @since as added in [29796]. I'm all for using these @since lines when there are changes to arguments or other major changes. This falls way down the slippery slope as to what warrants an @since and what doesn't. My two cents.

#11 @DrewAPicture
4 years ago

I think anytime we're dealing with something that could be construed as a "default" value -- making a change like this needs to be documented as part of the overall "changelog" for that function.

That goes for actual arguments supplied to the function, as well as array items of arguments (which we're now documenting through hash notations). I think it was a lot easier to dismiss notating these changes before we started documenting them, but now it's time to have a little more accountability in core saying, "no, you aren't crazy, we changed this. And this is when."

Note: See TracTickets for help on using tickets.