Make WordPress Core

Opened 4 years ago

Last modified 18 months ago

#22840 new defect (bug)

Uploading an existing plugin saves the zip in media library

Reported by: Ipstenu Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4.2
Component: Upgrade/Install Keywords: close
Focuses: Cc:


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)

22840.diff (3.8 KB) - added by dd32 4 years ago.

Download all attachments as: .zip

Change History (12)

#2 @nacin
4 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.)

#4 @dd32
4 years ago

  • Milestone changed from Awaiting Review to 3.7

#5 @dd32
4 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.

4 years ago

#6 @dd32
4 years ago

  • Milestone changed from 3.7 to Future Release

punting out of 3.7 for now.

#7 @nacin
3 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?

#8 @nacin
3 years ago

  • Milestone changed from Future Release to 3.7

#9 @dd32
3 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.

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

#10 @nacin
3 years ago

  • Milestone changed from 3.7 to Future Release

#11 @chriscct7
18 months ago

  • Keywords close added
Note: See TracTickets for help on using tickets.