Twenty Twelve upgrade issues

[nacin]

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

[lancewillett]

In r23173

Bump Twenty Twelve's version to 1.1.1.

[nacin]

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

[nacin]

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

[nacin]

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.

[lancewillett]

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.

[lancewillett]

Related: ​http://core.trac.wordpress.org/changeset/1169/tests

[dd32]

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

[dd32]

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?

[dd32]

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

[DrewAPicture]

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.

[dd32]

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

[nacin]

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.

[dd32]

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.

[nacin]

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.

[nacin]

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

[nacin]

Needs final testing and commit, I think.

[nacin]

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

[dd32]

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

[dd32]

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

[dd32]

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

[dd32]

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.

[SergeyBiryukov]

Replying to nacin:

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

Related: #12694