WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#31675 closed enhancement (fixed)

register_sidebar() without `id` parameter should be _doing_it_wrong()

Reported by: markjaquith Owned by: markjaquith
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: Widgets Keywords: has-patch commit
Focuses: Cc:

Description

If you register a widget area with register_sidebar() and neglect to provide an id parameter, WordPress generates auto-increment ones for you. The id is how WordPress stores the widget area's contents.

Consider this:

register_sidebar( array( 'One' ) );
register_sidebar( array( 'Two' ) );
register_sidebar( array( 'Three' ) );

If you comment out 'One' or 'Two', the contents of the remaining ones will get scrambled!

Best practice is to provide an 'id'. We should encourage that.

Attachments (7)

register-sidebar-id.31675.1.diff (524 bytes) - added by tschutter 5 years ago.
31675.2.diff (548 bytes) - added by valendesigns 5 years ago.
31675.diff (513 bytes) - added by markjaquith 5 years ago.
31675.3.diff (516 bytes) - added by markjaquith 5 years ago.
31675.4.diff (1.1 KB) - added by markjaquith 4 years ago.
31675.5.diff (1.1 KB) - added by markjaquith 4 years ago.
31675.6.diff (1.1 KB) - added by DrewAPicture 4 years ago.
Adjusted string

Download all attachments as: .zip

Change History (33)

#1 in reply to: ↑ description @DrewAPicture
5 years ago

  • Keywords needs-patch good-first-bug added

Replying to markjaquith:

Best practice is to provide an 'id'. We should encourage that.

I agree. +1.

#2 @tschutter
5 years ago

  • Keywords has-patch added; needs-patch removed

I added a patch to trigger _doing_it_wrong() with the message the id argument is empty.

@valendesigns
5 years ago

#3 @valendesigns
5 years ago

  • Keywords dev-feedback added

Patch 31675.2.diff changes the text a bit, and uses __FUNCTION__ not __METHOD__ as that seems to be the standard everywhere else.

@markjaquith
5 years ago

#4 @markjaquith
5 years ago

  • Milestone changed from Awaiting Review to 4.2
  • Owner set to markjaquith
  • Status changed from new to accepted

Updates the text a little more. Removes the comment (the code is pretty self-explanatory).

@markjaquith
5 years ago

#5 follow-up: @markjaquith
5 years ago

More tweaks. Pass in 'id' as a token so it doesn't get translated.

#6 in reply to: ↑ 5 @valendesigns
5 years ago

  • Keywords dev-feedback removed

Replying to markjaquith:

More tweaks. Pass in 'id' as a token so it doesn't get translated.

Nice catch! 31675.3.diff looks like a good commit candidate to me.

#7 @markjaquith
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 31850:

Trigger _doing_it_wrong() if register_sidebar() is not passed an id

  • If you don't pass an id, WP sets an auto-increment one for you.
  • But this depends on order of sidebar definition.
  • Change the order or remove a sidebar? They jumble.

fixes #31675
props tschutter, valendesigns

#8 @miqrogroove
4 years ago

See #31903. I think this could be a big problem.

#9 @DrewAPicture
4 years ago

#31903 was marked as a duplicate.

#10 @DrewAPicture
4 years ago

  • Keywords good-first-bug removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#11 @markjaquith
4 years ago

@miqrogroove makes the point on #31903 that if someone with an existing theme sees this notice and retroactively provides a new ID for the sidebars, then their existing content will be inaccessible.

So... this notice is useful when someone is developing a theme. We're warning them of an issue that could cause problems down the road.

If someone has already released a theme, then yes, changing the IDs to something else is going to make the contents of the old IDs inaccessible. The solution for theme authors who messed this up is to give their sidebars the auto-increment IDs that WordPress already gave them. We could, in the notice, suggest precisely this (even providing them the ID, and the "name" it corresponds to).

@markjaquith
4 years ago

@markjaquith
4 years ago

#12 @markjaquith
4 years ago

There's a more verbose message that identifies the problematic sidebar by name, says what auto-increment id it has defaulted to, and suggests that the dev hardcode that auto-increment id as a manual id, which will cement it and protect it against id "drift".

Last edited 4 years ago by markjaquith (previous) (diff)

#13 @miqrogroove
4 years ago

I'm wondering if this is still misleading, simply because the message doesn't say that the ID is specific to the site/network/whatever. "Manually set ... to silence this notice and keep existing sidebar content" is good advice only for a theme that wont be used on any other install, correct?

#14 @miqrogroove
4 years ago

Let's revert [31850] for 4.2 and start updating the args and codex to indicate that the register_sidebar args are no longer optional.

From an API point of view, you won't be able to force anyone to do this unless you add a new arg to the function. The id would become deprecated, and its replacement such as id2 could inherit the id value if set. If id and id2 are both not set, then it would be possible to notify the developer to add the id2 value only. When added, the data are not lost because the id value is unchanged.

#15 @miqrogroove
4 years ago

Maybe even better idea: Keep the id arg and add a new arg called old_id_was_blank

@DrewAPicture
4 years ago

Adjusted string

#16 @DrewAPicture
4 years ago

  • Keywords commit added

31675.6.diff adjusts the new string just a little bit, and adds the other two placeholders to the translator comment, where %1$s is id, %2$s is the sidebar name, %3$s is the sidebar id:

No %1$s was set in the arguments array for the "%2$s" sidebar, defaulting to "%3$s". Manually set the %1$s to "%3$s" to silence this notice and keep existing sidebar content.

#17 @markjaquith
4 years ago

"Manually set ... to silence this notice and keep existing sidebar content" is good advice only for a theme that wont be used on any other install, correct?

Not that I can think of. Essentially, by neglecting to choose an id, WordPress chooses one for you. But the one WordPress chooses is based on the order you defined the sidebars.

So say a theme update adds a new sidebar, in the middle of the others. Stuff going to get jumbled. The notice tells you to set manually the id values that WordPress is currently auto-choosing. Then, if you add a new sidebar (also with an id), the old ones don't jumble.

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


4 years ago

#19 @miqrogroove
4 years ago

So the sidebar id doesn't need to be globally unique? Then it makes more sense to just cement the default id.

#20 follow-up: @miqrogroove
4 years ago

I did some testing on this issue and found several more problems:

  1. The new E_USER_NOTICE is essentially a nuisance error because setting the sidebar id to its default value does nothing.
  2. The default value for a sidebar id is identical to the id hardcoded into the bundled themes, causing the themes to share sidebars regardless whether it is specified or not.
  3. Changing the sidebar id to a unique value not only causes any existing widgets to be deactivated, but may also cause any call to dynamic_sidebar() to fail because its default argument corresponds to the default sidebar id.

I really can't find a reason for throwing a new error when the recommended action has no effect.

#21 @miqrogroove
4 years ago

Correction: The themes do not "share" sidebars, the widgets just get copied between them. So there really is no need for an error message in the first place.

#22 in reply to: ↑ 20 @DrewAPicture
4 years ago

Replying to miqrogroove:

I really can't find a reason for throwing a new error when the recommended action has no effect.

There isn't a good reason for throwing a new error right now, other than that it paves the way toward actually requiring the id parameter in the future. My +1 recommendation for the new _doing_it_wrong() call stands.

Thank you for the feedback.

#23 follow-up: @markjaquith
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 32110:

Tell developers how to correctly silence register_sidebar() notices.

  • If we just tell them they they should set an id parameter, they might set an arbitrary one on an existing (i.e. not in-development) theme, causing widgets to be lost.
  • This way, we tell them to set it to the auto-generated id that the sidebar already has.
  • Widget content is not lost, and now their sidebar has a concrete handle.

fixes #31675

#24 @SergeyBiryukov
4 years ago

In 32112:

Translator comment should just reference placeholder numbers, not the actual placeholders.

see #31675.

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


4 years ago

#26 in reply to: ↑ 23 @juggledad
4 years ago

Replying to markjaquith:

In 32110:

Tell developers how to correctly silence register_sidebar() notices.

  • If we just tell them they they should set an id parameter, they might set an arbitrary one on an existing (i.e. not in-development) theme, causing widgets to be lost.
  • This way, we tell them to set it to the auto-generated id that the sidebar already has.
  • Widget content is not lost, and now their sidebar has a concrete handle.

fixes #31675

Just a question about this. For an existing themes - let's say that three users have the theme in production and have many widgets in the sidebar. Their sidebars have been given some ID and in each of the three users sites that ID could be different. Now how does a theme developer set an ID that won't mess up one or more of the users?

If the answer is to query the existing value and use that, then what is the use of forcing the use of the ID, why not leave this the way it is - if a theme developer uses an ID great, if not use the auto generated value.

Just wondering.

Last edited 4 years ago by juggledad (previous) (diff)
Note: See TracTickets for help on using tickets.