[wp-trac] [WordPress Trac] #11160: Inconsistancies in Naming and Using Sidebar Names and IDs.

[WordPress Trac]

#11160: Inconsistancies in Naming and Using Sidebar Names and IDs.
-----------------------------+----------------------------------------------
 Reporter:  CharlesClarkson  |       Owner:  azaozz                 
     Type:  defect (bug)     |      Status:  new                    
 Priority:  normal           |   Milestone:  2.9                    
Component:  Widgets          |     Version:  2.9                    
 Severity:  normal           |    Keywords:  has-patch needs-testing
-----------------------------+----------------------------------------------

Comment(by CharlesClarkson):

 Replying to [comment:6 azaozz]:
 > Replying to [comment:5 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.

 [http://core.trac.wordpress.org/attachment/ticket/11160/11160-2.patch
 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,

-- 
Ticket URL: <http://core.trac.wordpress.org/ticket/11160#comment:7>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software