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

[blurback]

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.

[blurback]

Screenshot of directory and output from upgrader

[dd32]

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.

[blurback]

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 because I took it from the first half of wp's move function. As for systems, I tested on a unix system and win system without issue.

[dd32]

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.

[DrewAPicture]

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

[dd32]

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

[knutsp]

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.

[dd32]

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.

[blurback]

Enhancement may be more appropriate than wontfix.

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

[dd32]

With no real movement here in 5 years, I'm marking this as wontfix, although I recognise that filesystem locks are going to cause problems for others in the future.

If someone wants to create a ticket to detect locks before upgrading, please do so.