Uploading an existing plugin saves the zip in media library

[Ipstenu]

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

[Ipstenu]

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

[nacin]

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.)

[SergeyBiryukov]

Related: #18182

[dd32]

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.

[dd32]

punting out of 3.7 for now.

[nacin]

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?

[dd32]

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