Implementation of is_registered_sidebar() doesn't match its documented signature

[mdgl]

Function is_registered_sidebar() was added in release 4.4 but the documentation for its function signature was changed after the initial commit and now no longer matches the implementation (see #24878).

The implementation of the function requires the string kind of sidebar "id" and will not accept a numeric "id" which would first need converting to a string of the form "sidebar-$id".

The simplest fix would be to just change the documentation, but if this function is considered useful for themes/plugins (as I suspect it is), additional code would be needed to convert a numeric parameter appropriately (see the beginning of existing function is_active_sidebar() for example).

If this code change is applied, then for consistency a similar change should probably be made to function unregister_sidebar() which also only accepts a string sidebar "id" and confusingly refers to it as the "name" (this more properly refers to the displayable name/title for the sidebar).

Suggested patch incoming, although in the patch I didn't rename the parameter to unregister_sidebar() as I'm not sure whether there are any backwards-compability issue with that.

[PieWP]

Improves numeric matching

[PieWP]

The usage of is_int is to type specific, it would return false on a string value of "1". The usage of is_numeric solves that issue.

As commeted by Rachelbaker in slack chat, tested it with "var_dump(is_numeric('1love1heart'));" it returns false.

[mdgl]

Thanks for the interest, PieWP but I think the original behaviour here with is_int() is actually correct and matches both the inline documentation and related functions such as is_active_sidebar() and dynamic_sidebar().

With your patch, a call to register_sidebar(array('id' => "1")) or is_active_sidebar("1") would refer to a different sidebar than unregister_sidebar("1") which would be rather inconsistent and confusing. In fact, you would not be able to unregister a sidebar identified and registered in that way.

Yes, the way WordPress handles sidebar identifiers is all a bit messy, but fixing it properly would require a much larger patch, changing many more things and potentially affecting existing themes and plugins. WordPress prides itself on maintaining backwards compatibility so it is not easy to "improve" such things without careful thought.

[welcher]

Can we regenerate the patch from the root level of the repo as opposed to the src folder? This will allow us to add unit tests to the same patch.

Also, there was a failure when running unit tests after I applied the patch:

Tests_Widgets::test_unregister_sidebar_with_numeric_id
Failed asserting that an array does not have the key 2.

/srv/www/wordpress-develop/tests/phpunit/tests/widgets.php:237

[mdgl]

@welcher you didn't say which patch you applied, nor did you respond to the "second opinion" request from @PieWP. There is a conflict of views here about how best to resolve this ticket.

As far as I can see, the failing test function test_unregister_sidebar_with_numeric_id() assumes sidebars can be registered with integer values for id but this actually conflicts with the documented interface for function register_sidebar() which specifies a string only. It also assumes that such sidebars are stored directly in global array $wp_registered_sidebars but this conflicts with the behaviour of function dynamic_sidebar() and is_active_sidebar() which assume an integer id is first converted to a string of form sidebar-$id before indexing the global array.

Which interface semantics would you like to support?

[welcher]

@mdgl I applied both patches and got the same failure on each.

As to the second opinion, I think that introducing changes that cause errors in order to match the documentation is not the best approach. In this case, I would say that the fix is to update the inline documentation and be explicit as to the discrepancies.

To your point, we will need to have a higher-level look at to the best way of standardizing these functions.

[helen]

I'm not sure I'm seeing consensus here, and seeing as we're into beta and this does not appear to be a new or particularly pressing issue, I'm punting. If a committer wants to own it and sees it as a bug fix, you know how to bring it back.

[mdgl]

It's not clear what question is being asked of the reporter here. The "bug" was raised to try and make the widget functions a bit more consistent and correspond to the documentation. But this may not be possible due to backwards compatibility concerns. From a development perspective, however I think there are two questions:

Should global wp_registered_sidebars be part of the public interface of the widgets "module"?

This ticket was really a response to #24878 which added a new function to try and hide the global wp_registered_sidebars from the rest of the codebase. At the same time, a few references to the global were updated to use the new function, but unfortunately this was left incomplete and parts of the admin tool, customizer and unit tests were not updated. The principles of good design would suggest we try and eliminate external dependencies on global variables such as this, but full backwards compatibility may prevent us from doing so.

Should the id referencing each widget be of mixed int/string type or just string?

The codebase is inconsistent as to the treatment of the id parameter for widgets. The code for many widget functions along with the unit tests all assume that the id can be either int or string, although this is often not matched by the accompanying documentation. Functions dynamic_sidebar and is_active_sidebar on the other hand purport to allow either int or string but in practice these are not compatible with the other functions and if an int is passed, this is converted immediately into a string of the form "sidebar-$id". In effect, these two functions only allow the widget id to be a string.

[welcher]

Replying to mdgl:

It's not clear what question is being asked of the reporter here.

Sorry I was unclear. I was referring to both patches having failures in them. https://core.trac.wordpress.org/ticket/37893?replyto=18#comment:14.

Should global wp_registered_sidebars be part of the public interface of the widgets "module"?

This ticket was opened to address an issue with code not matching its documentation. IMO, this question is a much bigger question that might be better suited in a new ticket. Otherwise, we run the risk of this ticket becoming more unfocused - see Helen's comment above.

Should the id referencing each widget be of mixed int/string type or just string?

Again, just my opinion, but updating the documentation to indicate that the type is mixed feels like an appropriate solution. At the very least, it's a first step towards the larger question about wp_registered_sidebars.

[mdgl]

The point about wp_registered_sidebars is that it is not clear whether in fact the tests are at fault for assuming how the widget "module" is implemented. For example, since #24878 the tests could now use function is_registered_sidebar which would insulate them from this implementation detail.

Updating the documentation blocks might be an appropriate fix, but it would need to be clear that the use of an int in functions dynamic_sidebar and is_active_sidebar is not the same as the use of int in all the other widget functions.

Still, not much interest in this, so suggest close.

[welcher]

Replying to mdgl:

Updating the documentation blocks might be an appropriate fix, but it would need to be clear that the use of an int in functions dynamic_sidebar and is_active_sidebar is not the same as the use of int in all the other widget functions.

Still, not much interest in this, so suggest close.

Instead of closing, why not write up a patch that updates the doc? Well documented code goes a very long way and is extremely helpful in situations like this where there are inconsistent implementations. I think it's worth the effort.

[welcher]

No traction here and the original reporter marked it as close. Closing.