WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 8 months ago

#22116 new defect (bug)

bug in dynamic_sidebar function

Reported by: alexvorn2 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4.2
Component: Widgets Keywords: has-patch
Focuses: Cc:

Description

In the dynamic_sidebar function is for displaying the right sidebar, by id, but in the code it is searched by name:

function dynamic_sidebar($index = 1) {
	global $wp_registered_sidebars, $wp_registered_widgets;

	if ( is_int($index) ) {
		$index = "sidebar-$index";
	} else {
		$index = sanitize_title($index);
		foreach ( (array) $wp_registered_sidebars as $key => $value ) {
			if ( sanitize_title($value['name']) == $index ) {
				$index = $key;
				break;
			}
		}
	}


must be changed to :

function dynamic_sidebar($index = 1) {
	global $wp_registered_sidebars, $wp_registered_widgets;

	if ( is_int($index) ) {
		$index = "sidebar-$index";
	} else {
		$index = sanitize_title($index);
		foreach ( (array) $wp_registered_sidebars as $key => $value ) {
			if ( sanitize_title($value['id']) == $index ) {
				$index = $key;
				break;
			}
		}
	}

Attachments (1)

22116.patch (676 bytes) - added by SergeyBiryukov 19 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 alexvorn219 months ago

If two sidebars have the same name that they will have the same Widgets, changing one widget will harm the second one...

comment:2 follow-up: SergeyBiryukov19 months ago

  • Keywords reporter-feedback added

Related: [5474] (for #4258).

Not sure I understand the issue. If I copy Twenty Twelve's main sidebar under the ID of sidebar-4, then dynamic_sidebar( 'sidebar-4' ) correctly displays the new sidebar, even if both have the same name ("Main Sidebar").

If dynamic_sidebar( 'Main Sidebar' ) is used, then the one that was registered earlier is displayed, which I guess is the expected result.

The suggested change doesn't seem correct, it would break the ability to display a sidebar by its name: dynamic_sidebar( 'Main Sidebar' ).

comment:3 in reply to: ↑ 2 alexvorn219 months ago

Replying to SergeyBiryukov:

Yeah you are right but if you have a sidebar ID: widget-area-1 not sidebar-1 that this can be a problem,

Example: you have ID: widget-area-1

name: Main Sidebar

AND

ID: widget-area-2

with the same name: Main Sidebar

So you will see two Widget Areas in the Widgets page, right? And adding a widget to the first or to the second widget area will make these two "Main Sidebar" widget areas to have the same widgets... or worse that can happen is that these widgets areas can lose everything

Hope it was clear what I wanted to say...

comment:4 SergeyBiryukov19 months ago

Still can't reproduce. My steps:

  1. Created two sidebars: widget-area-1 and widget-area-2, both named "Main Sidebar".
  2. Went to the Widgets screen, added "Archives" widgets to the first one and "Tag Cloud" widget to the second one.
  3. Refreshed the page, verified that the sidebars are completely independent in the admin.
  4. Verified that the sidebars are independent on the front end as well:
    • dynamic_sidebar( 'widget-area-1' ) displays the first sidebar.
    • dynamic_sidebar( 'widget-area-2' ) displays the second one.
    • dynamic_sidebar( 'Main Sidebar' ) displays the one that was registered earlier.

comment:5 alexvorn219 months ago

I was using other id and names, and wanted to show an example but it seems I must share the original code, here it is...
try this please:

	$name = 'Widget Area 121';

	register_sidebar( array(
		'name' => $name,
		'id' => 'widget-area-127',
		'description' => 'Widget Area 127',
		'before_widget' => '<div id="%1$s" class="widget %2$s">',
		'after_widget' => '<div style="clear: both;"></div></div>',
		'before_title' => '<h3 class="widget-title">',
		'after_title' => '</h3>',
	) );	
	
	register_sidebar( array(
		'name' => $name,
		'id' => 'widget-area-121',
		'description' => 'Widget Area 121',
		'before_widget' => '<div id="%1$s" class="widget %2$s">',
		'after_widget' => '<div style="clear: both;"></div></div>',
		'before_title' => '<h3 class="widget-title">',
		'after_title' => '</h3>',
	) );


Version 0, edited 19 months ago by alexvorn2 (next)

SergeyBiryukov19 months ago

comment:6 SergeyBiryukov19 months ago

  • Keywords has-patch added; reporter-feedback removed

Reproduced with the example above.

The issue happens when a widget has the same ID as the sanitized name of another widget registered earlier. Seems to be an edge case.

22116.patch only falls back to searching by name if the ID (which is also the array key) is not found.

comment:7 alexvorn219 months ago

I tested this... It seems to work great, need to push to 3.5 :)

comment:8 SergeyBiryukov19 months ago

  • Keywords 3.6-early added
  • Milestone changed from Awaiting Review to Future Release

Not a regression, moving to 3.6.

comment:9 alexvorn217 months ago

can you please update to 3.6 milestone.
My name is already on the credits page, does this mean I will have more rights now? for example changing the priority or selecting the milestone?

comment:10 follow-up: markoheijnen17 months ago

  • Keywords 3.6-early removed
  • Milestone changed from Future Release to 3.6
  • Severity changed from major to normal

I did move this to the milestone. Indeed an edge case but it should be dealt with.

To current rules you are lucky to be on the credits page. The solutions you have added to the 3.5 tickets are good but it would be better that you also create the patches ;) If you don't know how you can read the handbook: http://make.wordpress.org/core/handbook/

comment:11 in reply to: ↑ 10 alexvorn217 months ago

For me this is a big bug, but I'm not good at submitting patches, for me it takes a lot of time...
Thanks!

Last edited 17 months ago by alexvorn2 (previous) (diff)

comment:12 ryan11 months ago

  • Milestone changed from 3.6 to Future Release
Note: See TracTickets for help on using tickets.