Make WordPress Core

Opened 10 years ago

Closed 2 years ago

#22840 closed defect (bug) (fixed)

Uploading an existing plugin saves the zip in media library

Reported by: ipstenu's profile Ipstenu Owned by:
Milestone: 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 9 years ago.

Download all attachments as: .zip

Change History (14)

#2 @nacin
10 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
9 years ago

  • Milestone changed from Awaiting Review to 3.7

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

9 years ago

#6 @dd32
9 years ago

  • Milestone changed from 3.7 to Future Release

punting out of 3.7 for now.

#7 @nacin
9 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
9 years ago

  • Milestone changed from Future Release to 3.7

#9 @dd32
9 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
9 years ago

  • Milestone changed from 3.7 to Future Release

#11 @chriscct7
7 years ago

  • Keywords close added

This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.

2 years ago

#13 @pbiron
2 years ago

  • Resolution set to fixed
  • Status changed from new to closed

With the ability in 5.5 to update a plugin/theme by uploading a ZIP (see #9757), the problem described in this ticket has been resolved.

Note: See TracTickets for help on using tickets.