WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 15 months ago

#20733 new defect (bug)

Theme customizer doesn't order sections based on order added

Reported by: andyadams Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4
Component: Appearance Keywords: 3.6-early needs-patch settings-api
Focuses: Cc:

Description

When adding sections to the theme customizer, sections with the same priority are given seemingly random order. From the original customizer ticket:

Settings and sections both contain priority parameters (you can specify these in the constructor or alter them afterwards) which serve as the primary means of sorting sections before they're rendered. The order settings/sections are added serves as a secondary sorting mechanism (tiebreaker) when multiple items share the same priority.

I was under the impression that if the priority was the same, the sections would appear in the order they were added. However, if you add sections A, B, and C (in that order) it seems to display them in order B, C, A.

To replicate, add this code to your theme:

add_action( 'customize_register', 'theme_customize_register' );

function theme_customize_register( $wp_customize ) {
	// Register Section A
	$wp_customize->add_section( 'theme_section_a', array(
		'title'    => 'Section A',
		'priority' => 35,
	) );
	$wp_customize->add_setting( 'theme_option_a', array(
		'type'              => 'option',
	) );
	$wp_customize->add_control( 'theme_option_a', array(
		'section'    => 'theme_section_a',
		'type'       => 'text',
	) );

	// Register Section B
	$wp_customize->add_section( 'theme_section_b', array(
		'title'    => 'Section B',
		'priority' => 35,
	) );
	$wp_customize->add_setting( 'theme_option_b', array(
		'type'              => 'option',
	) );
	$wp_customize->add_control( 'theme_option_b', array(
		'section'    => 'theme_section_b',
		'type'       => 'text',
	) );

	// Register Section C
	$wp_customize->add_section( 'theme_section_c', array(
		'title'    => 'Section C',
		'priority' => 35,
	) );
	$wp_customize->add_setting( 'theme_option_c', array(
		'type'              => 'option',
	) );
	$wp_customize->add_control( 'theme_option_c', array(
		'section'    => 'theme_section_c',
		'type'       => 'text',
	) );
}

Expected result: Sections show up in order A, B, C

Actual result: Sections show up in order B, C, A

Sorry if I'm doing something stupid, or if this isn't the intended functionality. I'm running trunk without any plugins.

Attachments (3)

20733.patch (406 bytes) - added by SergeyBiryukov 2 years ago.
20733.patch.2 (557 bytes) - added by andyadams 2 years ago.
Comparing array key index instead
20733.3.diff (557 bytes) - added by Japh 18 months ago.
Refreshed patch

Download all attachments as: .zip

Change History (18)

comment:1 nacin2 years ago

  • Milestone changed from Awaiting Review to 3.4

comment:2 follow-up: jane2 years ago

This seems fairly trivial and not worth holding up 3.4. I suggest fixing for 3.5 but not worrying about it for 3.4. As long as they all actually show up that's what counts, right?

comment:3 ocean902 years ago

  • Cc ocean90 added
Version 0, edited 2 years ago by ocean90 (next)

comment:4 in reply to: ↑ 2 andyadams2 years ago

Replying to jane:

This seems fairly trivial and not worth holding up 3.4. I suggest fixing for 3.5 but not worrying about it for 3.4. As long as they all actually show up that's what counts, right?

Ultimately, I suppose that's true. In my case, I'm trying to add support for the customizer into a theme options library. Having this work as intended would prevent those of us who want to control order from having to set incrementing priorities to each section.

The use case where this would become an issue is when two separate sources (say a theme and a plugin) try to add sections to the same priority level. Instead of the theme sections and plugin sections being grouped together, they'd be intermingled at random. Not a disaster, but not ideal, either. A use case like that isn't something I see being common, but I thought it was worth mentioning.

It's not a huge deal, so if it ends up being a major issue to fix I think we can work around it.

SergeyBiryukov2 years ago

comment:5 SergeyBiryukov2 years ago

  • Keywords has-patch added

Perhaps we could use strcmp( $a->id, $b->id ) as a fallback for equal priorities.

Seems to work properly with the example from the description.

andyadams2 years ago

Comparing array key index instead

comment:6 andyadams2 years ago

Replying to SergeyBiryukov:

Perhaps we could use strcmp( $a->id, $b->id ) as a fallback for equal priorities.

Seems to work properly with the example from the description.

I think that will only work if the IDs are in alphabetical order. If we compare the position of the array keys instead, it works in my tests.

Sorry for the patch format - my first time trying SVN patching.

comment:7 jeffsebring2 years ago

You can use the priority arg to set the control order.

Here is an example:

      $wp_customize->add_control( 'customize[header_width]', array(
         'label'           => __( 'Logo Width ( px )', 'render' ),
         'section'         => 'header',
         'type'            => 'text',
         'priority'        => 11,
      ) );
Last edited 2 years ago by jeffsebring (previous) (diff)

comment:8 nacin2 years ago

Just so everyone is aware of why this is happening, PHP's user-sorting algorithm does not guarantee the original order of the array: http://php.net/usort. "If two members compare as equal, their relative order in the sorted array is undefined." and "The cmp_function doesn't keep the original order for elements comparing as equal."

We avoid these issues in the plugin API (and elsewhere) by keying things by priority. Or we could do the sorting ourself by simply looping through the elements and adding them to an array keyed by priority. Not necessarily the best idea here. Some kind of array_search is probably isn't going to be the fastest, but seems reasonable enough.

comment:9 westi2 years ago

I think for 3.4 we should leave it as is because there is a simple workaround - use distinct priorities to order your things.

Later we can revisit how we index the original arrays so that we can not need to do any sorting at all.

My feeling is we just push this to 3.5

comment:10 ocean902 years ago

  • Keywords 3.5-early needs-patch added; has-patch removed
  • Milestone changed from 3.4 to Future Release

My feeling is we just push this to 3.5

I agree, since you can still use the priority arg to change the order.


keying things by priority

+1

comment:11 SergeyBiryukov22 months ago

Closed #21180 as a duplicate (controls are also affected).

Japh18 months ago

Refreshed patch

comment:12 Japh18 months ago

  • Cc japh@… added
  • Keywords has-patch added; needs-patch removed

I've refreshed @andyadams' patch. If there's a different approach that should be taken, I'd be happy to re-patch with that.

comment:13 Japh18 months ago

  • Keywords needs-patch added; has-patch removed

Actually, the patch I refreshed is too specific. We need this patch to resolve for both sections and controls...

comment:14 Otto4218 months ago

  • Cc Otto42 added

comment:15 unknowndomain15 months ago

  • Cc me@… added
  • Keywords 3.6-early settings-api added; 3.5-early removed

What is the best way forward with this as it also has implications on #22487 where the same block of code is currently being proposed for adding priority to settings fields. It seems like rather a massive job to change the array layout for priority, is there a working solution for this?

Note: See TracTickets for help on using tickets.