Opened 13 years ago
Closed 5 years ago
#22840 closed defect (bug) (fixed)
Uploading an existing plugin saves the zip in media library
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 3.4.2 |
Component: | Upgrade/Install | Keywords: | close |
Focuses: | Cc: |
Description
Action: Install the same plugin twice.
Expected behavior: Second time, plugin should fail and not install.
Actual Behavior: Install fails, but the plugin's .zip shows in the Media Library. (Shows in Site #1 on Multisite)
This is not a 3.5 regression, same things happens in 3.4.2
Attachments (1)
Change History (14)
#2
@
13 years ago
- Component changed from Plugins to Upgrade/Install
We insert zips into the media library, to then delete them later. The same thing occurs during imports. This sounds like we are simply not always cleaning up the attachment in case of a known error. (Note that if things go wrong and the attachment still exists two hours later, we remove it via cron.)
#5
@
12 years ago
This occurs because in the event of a falsey (in this case, a null) result from the Upgrader, it assumes that the FTP credentials have been requested.
At that point, it switches from an upload, to simply a install-from-local-file situation, after which, it deletes the attachment it's installing from.
The problem here though, is that there is no current way to distinguish that the return value from Plugin_Upgrader::install() was a "Hang on, we just need to ask this person for some credentials", verses, "Yeah, We can't install this zip..".
attachment:22840.diff is an untested POC patch for a way around it for now.
#7
@
12 years ago
I'm moving this back to 3.7. At worst, we should do what we do with imports (which makes sense anyway since a critical failure *could* occur, leaving the file there either way).
Really simple:
wp_schedule_single_event( time() + DAY_IN_SECONDS, 'upgrader_upload_scheduled_cleanup', array( $this->id ) ); add_action( 'upgrader_upload_scheduled_cleanup', 'wp_delete_attachment' );
But, it seems like we might not always have an ID, based on the elseif
in the current cleanup() method?
#9
@
12 years ago
But, it seems like we might not always have an ID, based on the elseif in the current cleanup() method?
The File_Uploader handler DOES remove the file (It queues a single cronjob for +2hours - that can be reduced to 30m) when the file is uploaded directly via it.
wp-includes/default-filters.php 260:add_action( 'upgrader_scheduled_cleanup', 'wp_delete_attachment' );
I can't remember exactly, but I suspect there's a back-compat case there, where the class was used for installing from a local package rather than an uploaded file.
So, Aside from the deletion time being 2hours (which is overly long), I don't see anything that *needs* to change here for 3.7
FYI: Source is the forums - http://wordpress.org/support/topic/35-rc3-multisite-network-settings-allow-other-than-default-file-types-in-upload?replies=11