Make WordPress Core

Opened 3 years ago

Last modified 22 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:
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 23 months ago.

Download all attachments as: .zip

Change History (11)

comment:2 @nacin3 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.)

comment:4 @dd3223 months ago

  • Milestone changed from Awaiting Review to 3.7

comment:5 @dd3223 months 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.

@dd3223 months ago

comment:6 @dd3223 months ago

  • Milestone changed from 3.7 to Future Release

punting out of 3.7 for now.

comment:7 @nacin22 months 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?

comment:8 @nacin22 months ago

  • Milestone changed from Future Release to 3.7

comment:9 @dd3222 months 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

comment:10 @nacin22 months ago

  • Milestone changed from 3.7 to Future Release
Note: See TracTickets for help on using tickets.