WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#19092 closed defect (bug) (fixed)

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

Reported by: ocean90 Owned by: 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 2 years ago.
19092.2.patch (1.7 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 follow-up: azaozz2 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).

Version 0, edited 2 years ago by azaozz (next)

comment:2 azaozz2 years ago

  • Keywords close added; needs-patch removed

comment:3 ocean902 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.

SergeyBiryukov2 years ago

comment:4 in reply to: ↑ 1 SergeyBiryukov2 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.

comment:5 ocean902 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.

comment:6 azaozz2 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.

comment:7 ocean902 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.

comment:8 follow-up: nacin2 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".

comment:9 dougwrites2 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.

comment:10 ocean902 years ago

when Inactive Sidebar boxes disappear for a theme?

When you remove the widgets out of the sidebar.

comment:11 dougwrites2 years ago

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

comment:12 in reply to: ↑ 8 azaozz2 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.

comment:13 lancewillett2 years ago

  • Cc lancewillett added

Possibly related: #19274.

comment:14 azaozz2 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

comment:15 follow-up: SergeyBiryukov2 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

SergeyBiryukov2 years ago

comment:16 follow-up: SergeyBiryukov2 years ago

Refreshed the patch.

comment:17 aaroncampbell2 years ago

  • Cc aaroncampbell added

comment:18 in reply to: ↑ 16 ; follow-up: lancewillett2 years ago

Replying to SergeyBiryukov:

Refreshed the patch.

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

comment:19 in reply to: ↑ 18 lancewillett2 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.

comment:20 in reply to: ↑ 15 ; follow-ups: azaozz2 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.

comment:21 in reply to: ↑ 20 lancewillett2 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.

comment:22 in reply to: ↑ 20 ; follow-up: SergeyBiryukov2 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.

comment:23 azaozz2 years ago

In [19340]:

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

comment:24 in reply to: ↑ 22 azaozz2 years ago

Replying to SergeyBiryukov:

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

comment:25 azaozz2 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.