WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#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:
PR Number:

Description

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

Attachments (8)

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

Download all attachments as: .zip

Change History (31)

#1 @lancewillett
7 years ago

In r23173

Bump Twenty Twelve's version to 1.1.1.

#2 follow-up: @nacin
7 years 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.)

@nacin
7 years ago

@nacin
7 years ago

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

@nacin
7 years ago

#3 follow-up: @nacin
7 years 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.

#4 in reply to: ↑ 3 @lancewillett
7 years 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.

#6 @dd32
7 years 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

#7 @dd32
7 years 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?

@dd32
7 years ago

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

#8 @DrewAPicture
7 years 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.

#9 @dd32
7 years 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..

#10 @nacin
7 years 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.

@dd32
7 years ago

#11 @dd32
7 years 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.

#12 @nacin
7 years 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.

@dd32
7 years ago

@dd32
7 years ago

@dd32
7 years ago

#13 @nacin
7 years ago

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

#14 @nacin
7 years ago

  • Keywords has-patch added

#15 @nacin
7 years ago

  • Keywords commit added

Needs final testing and commit, I think.

#16 @nacin
7 years 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).

#17 @dd32
7 years 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

#18 @dd32
7 years 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

#19 @dd32
7 years 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

#20 @dd32
7 years 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.

#21 @nacin
7 years ago

  • Keywords fixed-major added

#22 in reply to: ↑ 2 @SergeyBiryukov
7 years ago

Replying to nacin:

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

Related: #12694

#23 @nacin
7 years ago

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