Inconsistancies in Naming and Using Sidebar Names and IDs.

[CharlesClarkson]

register_sidebar() allows more sidebar names and IDs to be registered than dynamic_sidebar() recognizes as valid names and IDs.

For example, register_sidebar() allows me to name a side bar "1" with a id of "first". I don't know why anyone would choose those values, but register_sidebar() allows it [1].

register_sidebar( array('name' => 1, id => 'first') );

dynamic_sidebar() will not be able to find the sidebar given its name (1).

    if ( is_int($index) ) {
        $index = "sidebar-$index"; /// 1 becomes 'sidebar-1'
        ...

The main problem is that dynamic_sidebar() is trying to process both IDs and names through the same variable ($index) while register_sidebar() separates the two with an array ( array('name' => 'Top', 'id' => 'sidebar-1' ).

According to the in-line docs for dynamic_sidebar():

It is confusing for the $index parameter, but just know that it should just work. When you register the sidebar in the theme, you will use the same name for this function or "Pay no heed to the man behind the curtain." Just accept it as an oddity of WordPress sidebar register and display.

It does "just work" if you never use your own sidebar IDs.

I started looking at this because I wanted to use is_active_sidebar() which tests to see if a dynamic_sidebar() has anything in it. There is no get_dynamic_sidebar(). dynamic_sidebar() sends everything to the browser or returns false.

    register_sidebar( array('name' => 'Top') ); // id defaults to "sidebar-1"
    ...

    if ( is_active_sidebar('Top') )
        dynamic_sidebar('Top');

Which fails because is_active_sidebar() just completely skips over searching for an id to go with a name. To get it to work you need to know when it was registered. Not something theme authors and designers are going to follow easily. There's a ticket to fix this: ​#10440

    if ( is_active_sidebar(1) )
        dynamic_sidebar('Top');

Like dynamic_sidebar(), is_active_sidebar() converts 1 to "sidebar-1". Unlike dynamic_sidebar() it assumes everything is entered as an id.

unregister_sidebar() assumes its parameter (incorrectly named $name, not $id) is an id. But it wants a literal id, like "sidebar-1". unregister_sidebar(1) unregisters a sidebar with an id of 1, while dynamic_sidebar(1) tries to display a sidebar with an id of "sidebar-1".

Widgets (Admin Page)

The dynamic_sidebar() function is used by the Widgets management page. So, it is possible to create a sidebar with register_sidebar() that dynamic_sidebar() cannot find. You can populate it with drag and drop [2] and not have it appear on the web site.

After Patch

If committed, this patch would remove the need for tickets ​#10440 and ​#10956. It changes the current argument behavior of unregister_sidebar(), but doesn't break backward compatibility. It allows is_active_sidebar(), unregister_sidebar() and dynamic_sidebar() all point to the same sidebar.

Before

These all refer to the same sidebar:

	is_active_sidebar(1);
	unregister_sidebar('sidebar-1');
	dynamic_sidebar('Sidebar Top');

In an admittedly contrived case, dynamic_sidebar() would silently fail to allow this sidebar to show:
register_sidebar( array('name'=>'Sidebar Top', 'id' => 1) );

After

These all refer to the same sidebar (the first two would have broken before the patch):

	is_active_sidebar('Sidebar Top');
	unregister_sidebar('Sidebar Top');
	dynamic_sidebar('Sidebar Top');

After the patch this shows fine:

register_sidebar( array('name'=>'Sidebar Top', 'id' => 1) );

After the patch it is possible to force an argument to be only a name or only an id:

	is_active_sidebar(array( 'name' => 'Sidebar Top' ));
	unregister_sidebar(array( 'name' => 'Sidebar Top' ));
	dynamic_sidebar(array( 'id' => 1 ));

Notes

[1] register_sidebar() allows the user to override the default setting of: 'id' => "sidebar-$i",

[2] When you refresh the Widgets management page the widgets will disappear from the sidebar. They are still attached to a sidebar, but dynamic_sidebar() cannot see the sidebar.

[scribu]

Related: #10954

[azaozz]

Removed some duplicate code. Needs quite a bit of testing if we want it in 2.9.

[Denis-de-Bernardy]

See also r11562, r11691, and #10300.

[CharlesClarkson]

Replying to azaozz:

Removed some duplicate code. Needs quite a bit of testing if we want it in 2.9.

The second patch (​11160-1.patch) breaks backward compatibility. Especially with dynamic_sidebar which I wanted to leave alone. Here's the more efficient code from the second patch:

    if ( (int) $args ) { // $args could be int 1 or string '1'
        return "sidebar-$args";
    } elseif ( is_string($args) ) {
        $name = sanitize_title($args);

        if ( array_key_exists($name, (array) $wp_registered_sidebars) )
            return $name;

        foreach ( (array) $wp_registered_sidebars as $id => $value ) {
            if ( sanitize_title($value['name']) == $name )
                return $id;
        }
    }

    return false;

That first test catches strings, which dynamic_sidebar() doesn't catch now. Users who have figured out that they can place 1 in quotes and name their sidebars by numbers, will find their sidebars break after the patch.

    if ( (int) $args ) { // $args could be int 1 or string '1'

In second test of the code you make the id take precedence over the name. This is the opposite of dynamic_sidebar() which assumes you have passed the name of a sidebar if a string was passed.

if ( array_key_exists($name, (array) $wp_registered_sidebars) )
    return $name;

It is not necessary (or desirable) to fix dynamic_sidebar(). The array interface allows programmers to specify unambiguous sidebar ids and names.

The rest of the second patch is great.

[azaozz]

Replying to CharlesClarkson:
I see your point about (int) $args.

Don't think the order of checking the id and name is that important. It's very unlikely one sidebar's id to match another sidebar's sanitized name and if that happens it will fail anyways. Perhaps we can put both tests in the same code block.

[CharlesClarkson]

Replying to azaozz:

Replying to CharlesClarkson:
Don't think the order of checking the id and name is that important.
It's very unlikely one sidebar's id to match another sidebar's
sanitized name and if that happens it will fail anyways. Perhaps
we can put both tests in the same code block.

I'm not looking at the likelihood. I am looking at what register_sidebar() allows. I realize that many of the cases it allows are weird, bizarre and unusual, but if register_sidebar() allows it and we can program it in, then why do that?

Well, okay, to answer my own question. We don't want to slow down WordPress needlessly. Before we take the shortcuts that eliminate some edge cases, let's try to include them. We can always revert back to the more limiting code.

​11160-2.patch fails if id or name in entered as an integer. It reverts back to the output given by the dynamic_sidebar() code.

foreach ( array(1, 2, 3,) as $id ) {
    echo wp_get_sidebar_id( array( 'id' => $id ) ) . '<br />';
}

In this test I have these sidebars registered

register_sidebar( array( 'name' => 'Sidebar Top', 'id' => 1 ));
register_sidebar( array( 'name' => 'Sidebar 1',));
register_sidebar( array( 'name' => 'Sidebar 2',));
register_sidebar( array( 'name' => '468x60 Header Banner', ));
register_sidebar( array( 'name' => 1, 'id' => 'first') );

The output of the latest version of the wp_get_sidebar_id() should be this (the 2 blank lines are false):

1

But, what we get is this:

sidebar-1
sidebar-2
sidebar-3

Using a similar test with names:

foreach ( array(1, 2, 3,) as $name ) {
    echo wp_get_sidebar_id( array( 'name' => $name ) ) . '<br />';

We get this:

sidebar-1
sidebar-2
sidebar-3

We were expecting this (the 2 blank lines are false):

first

This seems an easy fix, typecast the $args value coming off the array as a string:

if ( !empty($args['id']) )
    $args = (string) $args['id'];
elseif ( !empty($args['name']) )
    $args = (string) $args['name'];
else
    return false;

This fixes our first test and reveals a new problem for the second one (the 2 blank lines are false):

1

We got back the name, not the id because the name also matched an id. The idea of the named arguments is that we want to get the correct id back. Thus the name of the function.

The reason we are getting the id for the 'Sidebar Top' sidebar, is because we are testing for both ids and names in the same pass. PHP arrays will always return their elements in a given order. We will come to $id == 1 before we come to $name == 1:

if ( sanitize_title($value['name']) == $name || $id == $name )
    return $id;

Please don't shoot the messenger. It gets worse:

$names = array(
    '1', '2', '3', 'Sidebar Top',
    'Sidebar 1', 'Sidebar 2',
    '468x60 Header Banner', 'first',
);

foreach ( $names as $name ) {
    echo wp_get_sidebar_id( array( 'name' => $name ) ) . '<br />';
}

Here's what we get

1


1
sidebar-2
sidebar-2
sidebar-4
first

We should have this:

first


1
sidebar-2
sidebar-3
sidebar-4

My last concern I didn't feel like testing. When dynamic_sidebar() steps through its algorithm to separate ids from names, it ends up with the original name (string) if there were no matches. This new algorithm returns false in the same instance. I didn't test to see if this changes the behavior of dynamic_sidebar(). I would think it doesn't, but I didn't test it.

Oh One more item. Why are we using sanitize_title() to compare names? It is removing something important or harmful? I ask because comparing the strings like this is one reason why some sidebar names look like ids. Try echo sanitize_title('Sidebar 1'); .

HTH,

[azaozz]

Perhaps it would be better to standardize dynamic_sidebar() and require a name and a proper ID for each sidebar. We need to revisit the widgets API and remove deprecated functions anyways.

[Denis-de-Bernardy]

See also r11562, r11691, and #10300.

[CharlesClarkson]

Replying to azaozz:

Perhaps it would be better to standardize dynamic_sidebar() and require a name and a proper ID for each sidebar. We need to revisit the widgets API and remove deprecated functions anyways.

Rewriting an API may be above my pay grade, but I'm willing to give it a try. What functions need to be deprecated? Are we going to preserve backward compatibility? Is there any WordPress API standard that I could read or emulate?

[hakre]

Replying to azaozz:

Perhaps it would be better to standardize dynamic_sidebar() and require a name and a proper ID for each sidebar. We need to revisit the widgets API and remove deprecated functions anyways. I suggest to declare it's syntax like [5] Name in ​Names and Tokens (XML 1.0):

[4]   	NameStartChar ::=  ":" | [A-Z] | "_" | [a-z] | [#xC0-#xD6] | [#xD8-#xF6] | [#xF8-#x2FF] | [#x370-#x37D] | [#x37F-#x1FFF] | [#x200C-#x200D] | [#x2070-#x218F] | [#x2C00-#x2FEF] | [#x3001-#xD7FF] | [#xF900-#xFDCF] | [#xFDF0-#xFFFD] | [#x10000-#xEFFFF]
[4a]   	NameChar      ::=  NameStartChar | "-" | "." | [0-9] | #xB7 | [#x0300-#x036F] | [#x203F-#x2040]
[5]   	Name          ::=  NameStartChar (NameChar)*
[6]   	Names         ::=  Name (#x20 Name)*
[7]   	Nmtoken       ::=  (NameChar)+
[8]   	Nmtokens      ::=  Nmtoken (#x20 Nmtoken)*

(this handles US-ASCII and UTF-8)

---

Replying to CharlesClarkson:

Rewriting an API may be above my pay grade, but I'm willing to give it a try. What functions need to be deprecated? Are we going to preserve backward compatibility? Is there any WordPress API standard that I could read or emulate?

First of all since you have a higher insight right now into this issue I would name the problematic areas which need improvements in your eyes. For example that the edge cases are removed and what it needs in the data structure(s) therefore.

In a first guess I would suggest to define the syntax of a name and I would take care that it will never evaluate to an integer number under any circumstances to prevent the side-effects you're already reporting.

Next thing I can think of is that we do not have a propper widget API defined no-where (it just grew out of a plugin some time ago), so I think a list of existing widget and sidebar related functions is good to have to decide what to improve and what to drop.

[jimisaacs]

Many people are talking about backwards compatibility with functions that are most likely a little too old to worry about.

I believe the Widget API needs an overall like the next guy, but I don't believe it requires consistently trying to revamp something that is destined for eventual depreciation.

I say lets look to the future for the Widget API, and similarly to how the WP_Widget class standardized a lot with how plugins utilize the API.

I believe there is a hole that needs to be filled with the API that something like a class WP_WidgetArea would fill quite nicely.

The factory pattern is used for Widgets, while widget areas are still trapped in global "sidebar" variables and functions.
I believe that a WP_WidgetArea class would create a nice transition to a more secure and consistent API. We leave the current "sidebar" functions and variables the way they are, while utilizing a new API for 3.0

Anybody on the same page here?

[scribu]

+1 on the WP_WidgetArea class.

[jimisaacs]

How about, WP_WidgetArea and WP_WidgetArea_Factory.

Or utilizing the WP_Widget_Factory, but adding new properties and methods specific for widget areas?

[scribu]

Yeah, I assume we'll need a factory for the widget areas as well.

Btw, I think you should open a separate ticket for this.

[azaozz]

This sounds like a good proposal for new functionality in 3.1. We should be able to remove most of the back-compat code from the widgets API at the same time and perhaps add some refinements where needed.

[hakre]

Does this still sounds like a good proposal for new functionality in 3.1? Ticket still on Triage so far.

[nacin]

Closed #14548 as a duplicate.

[banago]

I suggest we replace dynamic_sidebar with dynamic_content since widgets are not meant and used only in sidebars.

[azaozz]

Closed #18187 as duplicate.

[azaozz]

This will need unit tests to go in.

[rofflox]

Any update here?

I'm using the latest trunk (3.5-alpha-21157) and I've stepped into an issue using register_sidebar() with an ID in camelCase format. Possibly related to this ticket. Widgets aren't shown in the admin page or the template using dynamic_sidebar().

[nacin]

What this ticket needs to proceed: Unit test coverage and Full backwards compatibility. I would rather not try to re-architect a new API in the process. Let's fix what we have.

If an upgrade routine is necessary to fix the database, that's fine. But existing code needs to continue to work.

[CharlesClarkson]

Replying to rofflox:

I'm using the latest trunk (3.5-alpha-21157) and I've stepped into an issue using register_sidebar() with an ID in camelCase format.

IIRC that is one of the problems with the current IDs. If possible, you should only use lowercase letters and numbers with the _ or - and the ID can only start with a letter: [a-z][a-z0-9_-]+

HTH,

[CharlesClarkson]

Replying to nacin:

What this ticket needs to proceed: Unit test coverage and Full backwards compatibility.

One day I will get around to a local install of WP and learn how to do unit testing.

People keep throwing money at me to do other things, but this is high on my list.