Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#19092 closed defect (bug) (fixed)

Inactive sidebar box can be on the wrong place in one situation

Reported by: ocean90's profile ocean90 Owned by: azaozz's profile azaozz
Milestone: 3.3 Priority: high
Severity: normal Version: 3.3
Component: Widgets Keywords: has-patch
Focuses: Cc:

Description

  • Clean install.
  • Twenty Eleven as default theme.
  • Change sidebar content: http://cl.ly/BQBN
  • Switch to another theme (in my case Toolbox). Screenshot of widget screen now: http://cl.ly/BOn9
  • Now open functions.php from Twenty Eleven and remove the last sidebar ('id' => 'sidebar-5',), save.
  • After that switch back to Twenty Eleven and you will get this: http://cl.ly/BPEZ

Scenario could be, that Toolbox is a maintenance theme so that you can change the old theme.

Related: #17979

Attachments (2)

19092.patch (1.4 KB) - added by SergeyBiryukov 12 years ago.
19092.2.patch (1.7 KB) - added by SergeyBiryukov 12 years ago.

Download all attachments as: .zip

Change History (27)

#1 follow-up: @azaozz
12 years ago

Looks like the last sidebar has the wrong name after switching back to Twenty Eleven. It is in the right place and has the right widget(s). Tweaking the theme's functions.php would do that.

Last edited 12 years ago by azaozz (previous) (diff)

#2 @azaozz
12 years ago

  • Keywords close added; needs-patch removed

#3 @ocean90
12 years ago

  • Keywords close removed

Did you try to reprodouce it?

The last sidebar shouldn't be a sidebar anymore, because I have removed it.

It is in the right place and has the right widget(s).

Right widgets is right, but the place is IMO wrong, should be on the left side, see second screenshot above.

#4 in reply to: ↑ 1 @SergeyBiryukov
12 years ago

  • Keywords has-patch added

Replying to azaozz:

Looks like the last sidebar has the wrong name after switching back to Twenty Eleven. It is in the right place and has the right widget(s).

If it's not registered by the theme any more, shouldn't it be considered orphaned? Adding a patch.

#5 @ocean90
12 years ago

If it's not registered by the theme any more, shouldn't it be considered orphaned? Adding a patch.

Right, this is my expected behaviour. Patch works for me.

#6 @azaozz
12 years ago

  • Milestone changed from 3.3 to Future Release

We save the widgets for the previous theme and then restore them on switching back to it. This seems to be a side effect: we trust that the theme matches the saved data.

Seems like a very rare case when the number of the theme's registered sidebars would change before switching back. In any case, it's too late to be able to test the patch now.

#7 @ocean90
12 years ago

  • Milestone changed from Future Release to 3.3
nacin: So I just switched themes and found like eight empty "Intactive sidebars (from previous them"
nacin: That is terrible.
nacin: The text is cut off no matter what I scale it to.
nacin: And the theme only has like two sidebars.

#8 follow-up: @nacin
12 years ago

  • Priority changed from normal to high

I have a feeling that when you introduce child themes into the mix, things get messy quickly.

I made a theme change on a site in the wild, and suddenly I had a bunch of inactive sidebar boxes, but all of them but one of them were empty.

Also, the inactive sidebar description needs some work, it doesn't read right, and the title doesn't fit, so all you see is "Inactive Sidebar (from previous them".

#9 @dougwrites
12 years ago

  • Cc heymrpro@… added

Is this a good place to ask when Inactive Sidebar boxes disappear for a theme? I'm assuming they go away for the first theme upon activation of a third theme.

Asking because the help tab for Widgets is still out (one of many in #19020) and needs more work.

#10 @ocean90
12 years ago

when Inactive Sidebar boxes disappear for a theme?

When you remove the widgets out of the sidebar.

#11 @dougwrites
12 years ago

ocean90: would those Inactive Sidebar boxes otherwise stick around permanently and/or through many theme changes?

#12 in reply to: ↑ 8 @azaozz
12 years ago

Replying to nacin:

I have a feeling that when you introduce child themes into the mix, things get messy quickly.

Agreed, changing to another theme and then back to the previous theme but this time with a child theme (that changes the number of sidebars) would make the saved data bad. Perhaps we should invalidate the data in this case and threat the old theme + child theme as a new/different theme.

I made a theme change on a site in the wild, and suddenly I had a bunch of inactive sidebar boxes, but all of them but one of them were empty.

This sounds like another bug. As far as I see it there shouldn't be any empty inactive sidebars at all.

Also, the inactive sidebar description needs some work, it doesn't read right, and the title doesn't fit, so all you see is "Inactive Sidebar (from previous them".

Maybe make the "(from previous theme)" a title/tooltip on "Inactive Sidebar"? We need to shorten it.

#13 @lancewillett
12 years ago

  • Cc lancewillett added

Possibly related: #19274.

#14 @azaozz
12 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In [19332]:

When restoring sidebars after switching back to an old theme, make sure the saved data matches the theme's current sidebars, fixes #19092

#15 follow-up: @SergeyBiryukov
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[19332] breaks the sidebars mapping feature. When switching from Twenty Eleven to Twenty Ten, all widgets end up as inactive.

Also causes a notice:

NOTICE: wp-includes/widgets.php:1244 - Undefined index: wp_inactive_widgets

Wouldn't 19092.patch be the proper solution here?

The wp_inactive_widgets and orphaned_widgets check is based on the one from wp-admin/widgets.php:
http://core.trac.wordpress.org/browser/trunk/wp-admin/widgets.php#L367

#16 follow-up: @SergeyBiryukov
12 years ago

Refreshed the patch.

#17 @aaroncampbell
12 years ago

  • Cc aaroncampbell added

#18 in reply to: ↑ 16 ; follow-up: @lancewillett
12 years ago

Replying to SergeyBiryukov:

Refreshed the patch.

Refreshed patch fixes the PHP notice and tests out. I'd suggest going with it.

#19 in reply to: ↑ 18 @lancewillett
12 years ago

Replying to lancewillett:

Replying to SergeyBiryukov:

Refreshed the patch.

Refreshed patch fixes the PHP notice and tests out. I'd suggest going with it.

See also #19291. To capture lost widgets to orphans.

Relies on this patch being committed.

#20 in reply to: ↑ 15 ; follow-ups: @azaozz
12 years ago

Replying to SergeyBiryukov:

[19332] breaks the sidebars mapping feature. When switching from Twenty Eleven to Twenty Ten, all widgets end up as inactive.

Hmm, don't see that happening. All it does is check whether all currently registered sidebars match the stored data for the theme and ignores any extra data.

Also causes a notice:

NOTICE: wp-includes/widgets.php:1244 - Undefined index: wp_inactive_widgets

Seems the notice comes from the fact that 'wp_inactive_widgets' hasn't been registered yet. Can add that by hand to $sidebars. Strangely didn't see that notice while testing.

Wouldn't 19092.patch be the proper solution here?

19092.2.patch threats the saved data as "main" and looks for orphaned widgets in it. Not sure that's the proper behavior. If the saved data for a theme doesn't match the current theme's setup, think we should be discarding it, not appending to orphaned.

User case:

  • Twenty Ten with 5 widgets in each sidebar,
  • user installs theme X with only 4 sidebars leaving 2 orphaned sidebars,
  • after some time user modifies Twenty Ten and enables only 2 sidebars,
  • when activating the modified Twenty Ten, user ends up with 2 orphaned sidebars from theme X plus 4 orphaned sidebars from the saved data for Twenty Ten, where two of the sidebars are exactly the same as the previous orphaned sidebars.

#21 in reply to: ↑ 20 @lancewillett
12 years ago

Replying to azaozz:

Also causes a notice:

NOTICE: wp-includes/widgets.php:1244 - Undefined index: wp_inactive_widgets

Seems the notice comes from the fact that 'wp_inactive_widgets' hasn't been registered yet. Can add that by hand to $sidebars. Strangely didn't see that notice while testing.

I saw the error after your commit in r19332, and the latest patch fixes it.

Wouldn't 19092.patch be the proper solution here?

19092.2.patch threats the saved data as "main" and looks for orphaned widgets in it. Not sure that's the proper behavior. If the saved data for a theme doesn't match the current theme's setup, think we should be discarding it, not appending to orphaned.

I disagree. This goes against the point of the Orphaned widgets idea. Nothing should go into Inactive unless it's moved there explicitly.

The consensus in #17979 was that we'd save the data for previous themes indefinitely -- and that data trumps anything new when you switch back.

See notes in http://core.trac.wordpress.org/ticket/19291#comment:5.

At that time I'd argued to remove the stored data after a week. But that was overruled by the core team. :)

I think on WP.com we might end up only storing it for a week. If a theme is switched back after that time, we'll just treat it as a new theme, not on old one you're going back to.

#22 in reply to: ↑ 20 ; follow-up: @SergeyBiryukov
12 years ago

Replying to azaozz:

[19332] breaks the sidebars mapping feature. When switching from Twenty Eleven to Twenty Ten, all widgets end up as inactive.

Hmm, don't see that happening. All it does is check whether all currently registered sidebars match the stored data for the theme and ignores any extra data.

Can't reproduce now. I probably forgot to clear theme mods that time. Sorry for the false alarm.

#23 @azaozz
12 years ago

In [19340]:

Prioritize the saved data over the current data when restoring widget positions, props SergeyBiryukov, see #19092

#24 in reply to: ↑ 22 @azaozz
12 years ago

Replying to SergeyBiryukov:

No worries :) We have to use the stored data anyways, going with your patch.

#25 @azaozz
12 years ago

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

This seems fixed with [19340].

Note: See TracTickets for help on using tickets.