WordPress.org

Make WordPress Core

#22856 closed defect (bug) (fixed)

Twenty Twelve upgrade issues

Reported by: nacin Owned by:
Milestone: 3.5.1 Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: needs-testing fixed-major
Focuses: Cc:

Description

Placeholder ticket. Will explain the full issue once the fire is out.

Attachments (8)

22856.diff (1.2 KB) - added by nacin 16 months ago.
22856.2.diff (1.5 KB) - added by nacin 16 months ago.
Adds transient-busting, as update-core.php only refreshes every 60 seconds.
22856.3.diff (1.5 KB) - added by nacin 16 months ago.
22856.4.diff (1.5 KB) - added by dd32 16 months ago.
Make WP_Theme recognise an empty theme directory is not a valid theme existance
22856.5.diff (754 bytes) - added by dd32 16 months ago.
22856.6.diff (797 bytes) - added by dd32 16 months ago.
22856.7.diff (1.1 KB) - added by dd32 16 months ago.
22856.7.2.diff (1.1 KB) - added by dd32 16 months ago.

Download all attachments as: .zip

Change History (31)

comment:1 lancewillett16 months ago

In r23173

Bump Twenty Twelve's version to 1.1.1.

comment:2 follow-up: nacin16 months ago

Okay, so:

When preparing api.wordpress.org for release, I missed that the new_bundled variable was not populated with 3.5. It remained at 3.2. You can see this variable here: http://api.wordpress.org/core/version-check/1.6/?debug. (I've added this to our release checklist in two different places to avoid us missing this in the future.)

When that occurred, Core_Upgrader::upgrade() chose the no-content package. This meant that the downloaded zip would not have any themes or plugins in it. Normally, this would be fine, and someone could just go over to theme-install.php and install Twenty Twelve from there.

But, they couldn't, because the "Copy New bundled plugins & themes" section of update_core() in wp-admin/includes/update-core.php loops through $_new_bundled_files and does a copy(), which manages to create an empty directory (but nothing else, as there was nothing else to copy).

So then what happens is the theme shows as broken — it's a directory without a stylesheet. But, wp_update_themes() does not check broken themes. (It calls wp_get_themes() rather than wp_get_themes( array( 'errors' => null ) ).) So, core thinks it is up to date, which means you can't install/update it. It's broken, which means you can't delete it. Overall, a bit of a mess. I think we can make some of this experience better in 3.6.

Here's an initial Hotfix that allows Twenty Twelve to updated to 1.1 if it exists but is a broken theme: http://plugins.trac.wordpress.org/changeset/637271 (and http://plugins.trac.wordpress.org/changeset/637297).

(Note: We had temporarily released 1.1.1 back when I didn't realize it wouldn't be updatable. To avoid hitting everyone with an update notice, I rolled it back. 1.1 is now current.)

nacin16 months ago

nacin16 months ago

Adds transient-busting, as update-core.php only refreshes every 60 seconds.

nacin16 months ago

comment:3 follow-up: nacin16 months ago

Hotfix 1.0 is out to allow users to update to Twenty Twelve: http://wordpress.org/extend/plugins/hotfix/.

1.1.1 is still getting pulled from the API. Trying to fix that.

comment:4 in reply to: ↑ 3 lancewillett16 months ago

Replying to nacin:

Hotfix 1.0 is out to allow users to update to Twenty Twelve: http://wordpress.org/extend/plugins/hotfix/.

I applied the hotfix plugin to my test sites that had the broken theme and all is good to go, Twenty Twelve was downloaded and I was able to activate it.

1.1.1 is still getting pulled from the API. Trying to fix that.

Noting from today's dev chat, Nacin and dd32 were discussing doing something for this in the 3.5.1 release.

comment:6 dd3216 months ago

In 23179:

When copying new bundled themes & plugins, bail early if the distro doesn't include the bundled item. This prevents us from creating an empty directory in the destination when the source doesn't exist. See #22856

comment:7 dd3216 months ago

As it turns out, an empty twentytwelve directory does not prevent WP_Upgrader from installing the theme properly, so no changes are needed there.

However, WordPress classifies an empty theme directory as being a theme which exists, but is broken. As a result, the current theme install listing page says there's an update available for TwentyTwelve.
However, as far as WP_Upgrader is concerned, an empty theme directory is not an installed theme, and as a result, can't perform an upgrade on an empty directory.

Now, I might be biased, but in the battle of WP_Upgrader vs WP_Theme, I'm inclined to go with WP_Upgrader. Attached is a patch which makes WP_Theme aware of empty theme directories, and doesn't claim that the theme exists. @nacin review?

dd3216 months ago

Make WP_Theme recognise an empty theme directory is not a valid theme existance

comment:8 DrewAPicture16 months ago

  • Cc xoodrew@… added

Looking at the error message in 22856.4.diff:

What about changing the error to say "The theme directory does not contain any theme files"? Many users consider the "theme" to be the folder. Saying "does not contain a theme" could be confusing.

comment:9 dd3216 months ago

Ideally in my mind, that error would *never* be shown to a end user, only a Developer.
I really don't think we should call an empty theme folder a theme, or list it in the broken themes list.
I'd be open to WP_Theme just completely ignoring the directory actually, but all thats for another ticket..

comment:10 nacin16 months ago

WP_Theme works exactly how get_themes() did in classifying an empty directory — containing neither files for a theme, nor other themes — as a broken theme. This wasn't a change in behavior in 3.4. Also, WP_Theme isn't actually checking that the directory is empty — only that it is missing both index.php and style.css. It could have every other file, for all we know.

I'm fine with considering such a change for 3.6. For 3.5.1, though, what should we do? [23179] looks like a good change for 3.6 only.

For 3.5.1, it seems like we have two options:

  1. Detect an empty directory and delete it, clearing the path for Twenty Twelve to be installed.
  2. Detect an empty directory and install the actual Twenty Twelve over it.

Option 1 is something we'd do in update-core.php for the foreseeable future. If we were do to option 2, it's probably something we'll do up to 3.6.0 only (either through 3.5.x, or including 3.6.0). Then in 3.6, we can rectify the WP_Theme/WP_Upgrader discrepancy.

dd3216 months ago

comment:11 dd3216 months ago

22856.5.diff tests to see if the theme exists, and isn't an empty directory. Not optimal long-term IMO, but covers the issue.

comment:12 nacin16 months ago

For 3.5.1, it seems like we have two options:

  1. Detect an empty directory and delete it, clearing the path for Twenty Twelve to be installed.
  2. Detect an empty directory and install the actual Twenty Twelve over it.

Looking at 22856.5.diff, I was actually thinking this was something for admin/includes/update-core.php. In that case, we can "detect an empty directory" by simply looking for the existence of the twentytwelve directory and the nonexistence of twentytwelve/style.css and index.php. Then, just delete the directory with $wp_filesystem->delete( $path_to_twentytwelve, false ), which will fire an rmdir, which will simply fail if the directory isn't empty.

I think that is nicer than a glob() at run-time.

dd3216 months ago

dd3216 months ago

dd3216 months ago

comment:13 nacin16 months ago

If we do 22856.7.diff, [23179] needs to be merged to 3.5.

comment:14 nacin16 months ago

  • Keywords has-patch added

comment:15 nacin16 months ago

  • Keywords commit added

Needs final testing and commit, I think.

comment:16 nacin16 months ago

In trunk (3.6), we only need to deal with the delete(), not with any of the new bundled theme logic. So more or less 22856.6.diff (minus the true on the delete() call).

comment:17 dd3216 months ago

In 23251:

When copying new bundled themes & plugins, bail early if the distro doesn't include the bundled item. This prevents us from creating an empty directory in the destination when the source doesn't exist. See #22856. Merges [23179] to the 3.5 branch

comment:18 dd3216 months ago

In 23252:

Upgrade: When upgrading from WordPress 3.5, if an empty twentytwelve theme directory exists, remove it and install Twenty Twelve. See #22856. For the 3.5 branch

comment:19 dd3216 months ago

In 23253:

Upgrade: When upgrading from WordPress 3.5, if an empty twentytwelve theme directory exists, remove it to allow the installation of Twenty Twelve. See #22856. For trunk

comment:20 dd3216 months ago

  • Keywords needs-testing added; has-patch commit removed

The above commits should cover 3.5 and trunk.

I'd like Nacin to triple test 3.5's behaviour with a 3.5.1 partial build before calling this fixed and closed, as the automated builds may differ from my testing zip.

comment:21 nacin16 months ago

  • Keywords fixed-major added

comment:22 in reply to: ↑ 2 SergeyBiryukov15 months ago

Replying to nacin:

It's broken, which means you can't delete it.

Related: #12694

comment:23 nacin15 months ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.