WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 7 months ago

#17680 reviewing defect (bug)

Using "update automatically" can destroy a theme in a Windows environment

Reported by: blurback Owned by: dd32
Milestone: Future Release Priority: normal
Severity: major Version:
Component: Upgrade/Install Keywords: has-patch dev-feedback
Focuses: Cc:

Description

If there are any locked files or files in use, Update Automatically destroys the theme.

I've experienced this three different ways:

  1. The machine is really slow to delete the original theme dir, when the update is moved into the themes dir, an error occurs: "Could not create directory". The theme is now gone.
  2. If there are any locked files in the outdated theme (e.g. SVN), all the files except the locked one will be deleted. Upgrader bails and you're left with a broken/empty theme.
  3. If any of the files are in use, all files inside the theme will be deleted but the file system will throw an error. The updater bails with an empty theme directory.

Steps to duplicate:

  1. On a Win hosted install, open the twentyten theme directory
  2. Edit stylesheet, change version to 1.0
  3. Open screenshot.png in paint
  4. In the wp-admin, upgrade the theme
  5. Sad times (screenshot attached)

Proposed solution is to try to rename the old theme first. Provides two benefits:

  1. Have a backup of original theme if something breaks mid-way
  2. Never left with a broken site

Note: all the more reasons why one should not use Windows :/ but I have no choice.

Attachments (2)

WP-Upgrader-Error-Screen-shot-2011-06-03.png (109.1 KB) - added by blurback 4 years ago.
Screenshot of directory and output from upgrader
Proposed Upgrader Fix.diff (4.2 KB) - added by blurback 4 years ago.

Download all attachments as: .zip

Change History (11)

@blurback4 years ago

Screenshot of directory and output from upgrader

comment:1 follow-up: @dd324 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to Future Release

In most windows environments, renaming the folder moves the locks with it, which is what makes this work.. In other windows environments, locks are lost when moving the folder, which allows this to work, In some windows environments, locks prevent the parent directory from being renamed, which allows this to work.

All in all, this is probably the best option, an extension of this would be if there's an error during copying the new theme in place, to delete the partial and rename the old one back into place.

Note, the patch is incomplete as it uses rename directly, which we can't do on many systems, instead, $wp_filesystem->move() (Which is a single fs request compared to copy_dir() which makes many fs requests.)

Leaving as has-patch for now given it's minor details we can look closer at when implementing.

comment:2 in reply to: ↑ 1 @blurback4 years ago

Replying to dd32:

if there's an error during copying the new theme in place, to delete the partial and rename the old one back into place.

Agree.

Note, the patch is incomplete as it uses rename directly, which we can't do on many systems, instead, $wp_filesystem->move()

I did first try $wp_filesystem->move() but encountered an error. If I remember correctly, the problem was that $wp_filesystem->move() first tries @rename and if that fails it tries to copy (which is not a recursive function and errors).

I used @rename directly I took it from the first have of wp's move function. I did try it on a unix system and win system without issue.

Version 1, edited 4 years ago by blurback (previous) (next) (diff)

comment:3 @dd324 years ago

I used @rename directly because I took it from the first half of wp's move function.

On some systems which use FTP (and there are windows systems which require that), rename() directly will cause it to fail every time.

It'll need some testing regardless, but I'm fairly confident a solution similar to the patch can be used. We've got a set of IIS test VM's that we'll hopefully be able to test this under.

comment:4 @chriscct77 months ago

  • Keywords dev-feedback added

comment:5 @DrewAPicture7 months ago

  • Owner set to dd32
  • Status changed from new to reviewing

@dd32: Mind following up here and either closing or recommending a course of action?

comment:6 @dd327 months ago

I'm quite tempted to say wontfix here, I also suspect this might be less of an issue with the later windows releases.

comment:7 @knutsp7 months ago

I've run several production sites on Windows 2008 for several years. This problem happens only when I open the theme or plugin folder through Windows Explorer while at the same time doing an update of the same theme/plugin through WordPress. The update will fail and theme/plugin will be gone.

It has happened that I have forgot to navigate up, or to close Explorer, overnight, and this has happened during an automatic update.

This is an edge case, and I guess it may happen mostly for developers, who will be able to delete the folder completely and reinstall.

A wontfix is not unreasonable unless there are many support cases having this problem.

A fix could also be not to require a folder to be deleted, to proceed with an update, as long as it's empty. I'm not sure why and update must fail if the folder itself is not deleted, but I guess there is a reason.

comment:8 @dd327 months ago

A fix could also be not to require a folder to be deleted, to proceed with an update, as long as it's empty. I'm not sure why and update must fail if the folder itself is not deleted, but I guess there is a reason.

That's a valid point, As long as the original and destination folders are the same, there's no need to remove the directory, which would indirectly work around some of the filesystem level locks (It won't prevent a failure when the lock prevents deleting a file, or the lock is on a subdirectory).

While this sounds like a simple change, it's not exactly that simple when you look at the upgrade routines for that area of code, although it's a possibility.

comment:9 @blurback7 months ago

Enhancement may be more appropriate than wontfix.

As long as the swing from one theme to another lacks guaranteed atomicity, "update automatically" is volatile.

Note: See TracTickets for help on using tickets.