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

[markjaquith]

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.

[DrewAPicture]

Replying to markjaquith:

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

I agree. +1.

[tschutter]

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

[valendesigns]

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

[markjaquith]

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

[markjaquith]

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

[valendesigns]

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.

[markjaquith]

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

[miqrogroove]

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

[DrewAPicture]

#31903 was marked as a duplicate.

[markjaquith]

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

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

[miqrogroove]

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?

[miqrogroove]

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.

[miqrogroove]

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

[DrewAPicture]

Adjusted string

[DrewAPicture]

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.

[markjaquith]

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

[miqrogroove]

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

[miqrogroove]

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.

[miqrogroove]

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.

[DrewAPicture]

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.

[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

[SergeyBiryukov]

In 32112:

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

see #31675.

[juggledad]

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.