WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 5 months ago

#37893 closed defect (bug) (wontfix)

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

Reported by: mdgl Owned by: welcher
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: Widgets Keywords: needs-patch
Focuses: Cc:

Description

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.

Attachments (2)

37893.patch (1.0 KB) - added by mdgl 3 years ago.
37893.1.patch (1.4 KB) - added by PieWP 3 years ago.
Improves numeric matching

Download all attachments as: .zip

Change History (24)

@mdgl
3 years ago

#1 @DrewAPicture
3 years ago

  • Focuses docs added
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.7
  • Version changed from trunk to 4.4

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 years ago

#3 @desrosj
3 years ago

  • Keywords needs-testing added

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

@PieWP
3 years ago

Improves numeric matching

#5 @PieWP
3 years ago

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.

#6 @mdgl
3 years ago

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.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

#8 @PieWP
3 years ago

  • Keywords 2nd-opinion added

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

#11 @jbpaul17
3 years ago

  • Owner set to welcher
  • Status changed from new to assigned

#12 @welcher
3 years ago

  • Keywords needs-patch added; has-patch needs-testing 2nd-opinion removed

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

#13 @mdgl
3 years ago

  • Keywords 2nd-opinion added

@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?

#14 @welcher
3 years ago

  • Keywords 2nd-opinion removed

@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.

#15 @DrewAPicture
3 years ago

  • Focuses docs removed

#16 @helen
3 years ago

  • Milestone changed from 4.7 to Awaiting Review

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.

#17 @welcher
2 years ago

  • Keywords reporter-feedback added

#18 follow-up: @mdgl
2 years ago

  • Keywords reporter-feedback removed

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.

#19 in reply to: ↑ 18 @welcher
2 years ago

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.

Last edited 2 years ago by welcher (previous) (diff)

#20 follow-up: @mdgl
2 years ago

  • Keywords close added

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.

#21 in reply to: ↑ 20 @welcher
2 years ago

  • Keywords close removed

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.

#22 @welcher
5 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

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

Note: See TracTickets for help on using tickets.