[wp-trac] [WordPress Trac] #14963: Plugin update shouldn't delete package from local filesystem

[WordPress Trac]

#14963: Plugin update shouldn't delete package from local filesystem
-------------------------+--------------------------------------------------
 Reporter:  joelhardi    |       Owner:                        
     Type:  enhancement  |      Status:  new                   
 Priority:  normal       |   Milestone:  Awaiting Review       
Component:  Plugins      |     Version:  3.1                   
 Severity:  normal       |    Keywords:  has-patch dev-feedback
-------------------------+--------------------------------------------------
Changes (by joelhardi):

 * cc: joel@… (added)


Comment:

 I think what I'm suggesting is already consistent with the way the code
 works. If you look at source:trunk/wp-admin/includes/class-wp-
 upgrader.php at 15978#L297 (the lines just before my patch is applied) you'll
 see that the variable `$download` is already set to either a URL or a
 local filepath by `download_package()`. If you look at
 `download_package()`, you'll see it already checks to see whether file is
 local or remote. Only if the file is remote is it downloaded to a
 temporary location by `download_url()` [in includes/file.php].

 So, my patch isn't determining whether file is local or remote, just using
 the result as already determined by `download_package()`. In the case
 where the file is a downloaded temp copy, it's deleted as before. In the
 case where the file is local, it hasn't been downloaded via
 `download_url()` and so isn't a temporary file that !WordPress should be
 deleting.

 And `unpack_package()` already accepts the optional second argument to
 tell it whether to delete the package or not after unpacking.

 So IMHO !WordPress is already doing the right thing and checking to see
 whether the file is local or remote, the only problem is that it is
 deleting the local file anyway. Thus the minor patch, to fix what I think
 is technically a bug, even if users aren't encountering it.

 (Also ... in the case where the file is local but !WordPress lacks
 filesystem permissions to unlink it, an E_WARNING is raised by the call to
 `unlink()` in `unpack_package()`.)

 As far as a regular user's use case ... I don't know of any. My use case,
 as a plugin developer, is that when I am writing a plugin that has
 install/activation hooks and I want to test it through the plugin
 upgrader, I mod the upgrader to use a local file rather than download via
 http.

 An alternate fix would be to change `download_package()` so that, if it
 determines a file is local, it copies it to the temp location and returns
 that filepath, so that the file that is later deleted by
 `unpack_package()` is the temp copy, not the original. This way,
 `download_package()` would '''always''' return an actual "downloaded"
 package in a temp dir, rather than the current return value of "sometimes
 a downloaded file, sometimes something from the local fs." That might be
 more transparent than my patch's branch in `run()`.

-- 
Ticket URL: <http://core.trac.wordpress.org/ticket/14963#comment:3>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software