WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 15 months ago

Last modified 15 months ago

#50612 closed defect (bug) (fixed)

Cancelling a plugin update from an uploaded .zip file leaves the plugin .zip in the uploads directory

Reported by: psykro Owned by: whyisjake
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.5
Component: Upgrade/Install Keywords: has-patch needs-testing commit
Focuses: Cc:

Description

When using the new feature which allows a plugin to be updated by uploading a zip file, if the user chooses to cancel the update, the uploaded plugin file is left in the relevant uploads directory.

Over time this might cause that directory to become full. As the zip file is not listed in the media library, the user has no idea it needs to be deleted.

Ideally this file should be listed in the Media library, for manual clean up, and/or automatically deleted if the user clicks cancel.

Attachments (1)

50612.diff (7.8 KB) - added by azaozz 15 months ago.

Download all attachments as: .zip

Change History (18)

#1 follow-up: @desrosj
15 months ago

To clarify, the uploaded zip file is listed in the Media Library, but only when using the list view. It does not show when using grid view.

#2 in reply to: ↑ 1 @psykro
15 months ago

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

Replying to desrosj:

To clarify, the uploaded zip file is listed in the Media Library, but only when using the list view. It does not show when using grid view.

Ah, thanks @desrosj, I wasn't aware of that. In that case I will close this ticket.

#3 follow-up: @desrosj
15 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I think it's still worth having a discussion about. Especially where grid view is the default Media Library view, and plugins/themes can be quite large.

I also can't think of a use case where having the zip in the library would be useful.

#4 in reply to: ↑ 3 @psykro
15 months ago

Replying to desrosj:

I think it's still worth having a discussion about. Especially where grid view is the default Media Library view, and plugins/themes can be quite large.

I also can't think of a use case where having the zip in the library would be useful.

One way could be to remove it if the user clicks cancel.

However that leaves the option open for if the user clicks anything else (back button, menu item).

The only way I can think of to mitigate that would be to set a flag (option) somewhere and trigger an admin notice, warning of a failed plugin/theme update.

#5 @joyously
15 months ago

I was wondering why it is entered into the database at all, but I assume there's a technical reason for that. It seems like the file would be in the upgrade folder, not the uploads folder.

#6 follow-up: @azaozz
15 months ago

if the user chooses to cancel the update, the uploaded plugin file is left in the relevant uploads directory.

Yes, that happens for all packages if the installing/updating was interrupted for some reason. There's a cron job to do cleanup: https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/class-file-upload-upgrader.php#L87 in two hours.

However these can be deleted immediately when the user has clicked "Cancel and go back". Patch coming up.

I assume there's a technical reason for that.

Right. The packages are handled by creating a "private attachment" (to reuse the file upload functionality). Perhaps can look into not listing these attachments in the Media Library "list view". They will disappear in couple of hours anyway.

#7 in reply to: ↑ 6 ; follow-up: @psykro
15 months ago

Replying to azaozz:

Yes, that happens for all packages if the installing/updating was interrupted for some reason. There's a cron job to do cleanup: https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/class-file-upload-upgrader.php#L87 in two hours.
However these can be deleted immediately when the user has clicked "Cancel and go back". Patch coming up.

Forgive me for jumping the gun here then :-) I've just been following this for 3 of it's 11 years, and I was excited to test it out before the 5.5 release.

#8 in reply to: ↑ 7 @azaozz
15 months ago

Replying to psykro:

Forgive me for jumping the gun here

Ah, no problems, thanks for reporting this! :)

There will probably be more left-over files now, as users would cancel updates or navigate away from the "update from uploaded zip" screen.

There is another possible edge case: when a user leaves the update from uploaded zip screen open for more than two hours, the file will be "cleaned up". Can perhaps add some js to block the screen when left unused for a while, and ask the user to upload the file again.

@azaozz
15 months ago

#9 @azaozz
15 months ago

In 50612.diff:

  • Fix the "Cancel and go back" button to do a cleanup (delete the attachment and file).
  • In the Media Library list view, hide attachments for recently uploaded install/update packages when scheduled to be deleted in the next two hours.
  • Hide the "Replace current with uploaded" button in 1 hour and 59 minutes. The attachment/file will be deleted in 2 hours.

Think this fixes the edge cases, please test :)

#10 @azaozz
15 months ago

  • Keywords has-patch needs-testing added

#11 @noisysocks
15 months ago

  • Keywords commit added

I tested 50612.diff locally by applying the patch, changing the timers to be 5 minutes instead of 2 hours, and then running through the upgrade flow. I was able to confirm that the attachment is eventually removed, that it doesn't show in the media page, and that the Replace current with uploaded button is eventually disabled.

Code looks good. My only minor comment is: Why is the check for window.wp.a11y necessary when we know we're including wp-a11y on update.php?

I'd say this is good to go! 👍

#12 @whyisjake
15 months ago

  • Owner set to whyisjake
  • Status changed from reopened to accepted

Looks good in my testing too.

#13 @whyisjake
15 months ago

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

In 48417:

Upgrade/Install: Ensure cleanup after canceled update.

Ensure that the uploaded zip is hidden from the media library, where a task will remove failed installs after two hours.

Fixes #50612.

Props psykro, desrosj, joyously, azaozz, noisysocks, whyisjake.

#14 follow-up: @joyously
15 months ago

There was a comment in Slack: https://wordpress.slack.com/archives/C02RQBWTW/p1594281462266200

Today I worked all night and took some time to test WP 5.5 plugin and themes update feature using zip files. It works with plugins, but I couldn't update any themes using the same method, has anyone tested/reported this yet?

#15 @SergeyBiryukov
15 months ago

In 48427:

Docs: Improve description for the JS function that hides the update button for expired plugin or theme uploads.

Add missing @since tag.

Follow-up to [48417].

See #50612.

#16 in reply to: ↑ 14 ; follow-up: @psykro
15 months ago

Replying to joyously:

There was a comment in Slack: https://wordpress.slack.com/archives/C02RQBWTW/p1594281462266200

Today I worked all night and took some time to test WP 5.5 plugin and themes update feature using zip files. It works with plugins, but I couldn't update any themes using the same method, has anyone tested/reported this yet?

I can confirm that I have tested this, rolled back Twenty Twenty, upgraded successfully. Also tested clean up of file if user elects to Cancel, as well as cron clean up of file, of user navigates away

#17 in reply to: ↑ 16 @azaozz
15 months ago

Replying to psykro:

Thanks for testing and confirming.

Note: See TracTickets for help on using tickets.