WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 3 months ago

#9757 closed feature request (fixed)

Allow Plugin/Theme updates from a uploaded .zip file.

Reported by: hakre Owned by: azaozz
Milestone: 5.5 Priority: high
Severity: normal Version: 2.8
Component: Upgrade/Install Keywords: has-patch early has-dev-note commit dev-reviewed
Focuses: ui, ui-copy Cc:

Description (last modified by SergeyBiryukov)

Plugin administration lacks of an update possibility by uploading zip files. This feature is only half-done in 2.7, you can only upload plugins as zip to install them, not to update them.

Zip file uploads should be treated the same as the other install/update possibilities like remote.

#9708 provides a testcase that can be used to test such am update by zip.

currently a over-upload in the install page does throw an error that the directory already exists:

Installing Plugin from uploaded file: 9708-plugin-testcase-9708-plugin-a-v-0.2.zip

Unpacking the package.

Installing the plugin.

Destination folder already exists. [...]worpress-trunk/wp-content/plugins/9708-plugin-testcase/

Plugin Install Failed.

Attachments (25)

upload_upgrade.diff (5.4 KB) - added by cyberhobo 9 years ago.
Refresh, improvements
9757.1.patch (23.5 KB) - added by imath 3 years ago.
9757.2.patch (403.1 KB) - added by imath 3 years ago.
9757.3.patch (29.7 KB) - added by azaozz 4 months ago.
1.png (108.5 KB) - added by azaozz 4 months ago.
Uploading - same version.
3.png (105.9 KB) - added by azaozz 4 months ago.
Updating to newer version.
4.png (103.7 KB) - added by azaozz 4 months ago.
Downgrading.
5.png (105.0 KB) - added by azaozz 4 months ago.
In this case the backup warning should be replaced with "required WP/PHP version not met" error and the "Remove current and install the uploaded version" link should not be outputted?
9757.4.patch (32.4 KB) - added by azaozz 4 months ago.
10.2.png (108.7 KB) - added by azaozz 4 months ago.
Prevent update when required WP version is not met.
11.png (116.8 KB) - added by azaozz 4 months ago.
Both WP and PHP "requirement not met" error messages for a plugin.
12.png (119.4 KB) - added by azaozz 4 months ago.
Both WP and PHP "requirement not met" error messages for a theme.
Theme.png (41.2 KB) - added by shaunandrews 4 months ago.
theme-errors
plugin.png (40.8 KB) - added by shaunandrews 4 months ago.
plugin-errors
9757.5.patch (34.2 KB) - added by mariovalney 4 months ago.
Avoid install plugin with wrong PHP requirement.
9757.6.patch (35.0 KB) - added by azaozz 4 months ago.
20.png (107.4 KB) - added by azaozz 4 months ago.
21.png (102.7 KB) - added by azaozz 4 months ago.
22.png (110.2 KB) - added by azaozz 4 months ago.
23.png (108.9 KB) - added by azaozz 4 months ago.
24.png (104.9 KB) - added by azaozz 4 months ago.
9757.diff (35.0 KB) - added by whyisjake 4 months ago.
composer format updates
9757.7.patch (36.5 KB) - added by earnjam 4 months ago.
Fixes unit test errors and PHPCS errors from 9757.6.patch
9757.2.diff (36.6 KB) - added by desrosj 4 months ago.
9757.3.diff (591 bytes) - added by sabernhardt 3 months ago.
changing $args to $parsed_args in class-theme-upgrader.php

Change History (222)

#1 @DD32
11 years ago

  • Component changed from Plugins to Upgrade/Install
  • Milestone changed from Unassigned to Future Release
  • Priority changed from normal to low
  • Summary changed from Install of plugin as zip is possible but update is not. to Allow Plugin/Theme updates from a uploaded .zip file.
  • Type changed from defect (bug) to enhancement

Moving to a Future release, If its so chosen to be integrated at some stage.

Personally. I'd prefer to leave it to plugin material (I may even write something for that).

If a trac admin could edit that description to remove the huge code block, that'd be nice.

#2 @hakre
11 years ago

If this is rated as plugin material then I would suggest to move the current remote installer in the plugin domain as well. I mean, it's more important to be able to install from zip then from third party remote even that is wordpress.org.

And for clearing the mess in the code example above: please, yes.

#3 @DD32
11 years ago

My reasoning was this:

  • For Installs: You've only got a source to deal with, its a straight up install.
  • For Upgrades: You're upgrading a specific plugin, It'd need to be set to select the plugin/theme to upgrade. The automatic upgrades are more suited to an automatic thing IMO. Theres nothing stopping plugins from adding their own update notifications + automatic upgrades.

Other items which such a plugin could implement include Installation from a remote user-specified URL. Currently it only allows uploaded files.

I personally see upgrades and Installations as vastly different things, A user might install plugins from multiple locations, WordPress.org, A remote URL(which WP doesnt support) or a plugin they've downloaded. Upgrades are different though, the user needs to be notified of an upgrade, and then the user would need to identify the plugin which the package is supposed to upgrade.

It'd be a simple patch though, If someones interested in making a UI for it (And a dev likes it) I'll write in the stuff for the backend-upgrade if need be (Only since i understand it pretty well, And realise some people might be better suited at UI programming)

#4 @Denis-de-Bernardy
11 years ago

just checking... can't the install screen already be used to upgrade?

#5 @DD32
11 years ago

No. Thus the error message which was provided above in the ticket.

#6 @Denis-de-Bernardy
11 years ago

  • Keywords needs-patch added; 2.8 removed

thoughts about setting clear_destination to true in this case?

#7 @DD32
11 years ago

thoughts about setting clear_destination to true in this case?

a -1 from me there.

IMO zips should be trusted less than whats already on the system, Auto-installed Plugins have the catch that all folder names are unique, and therefor, dont conflict, but if i download a plugin called "something Cool by Denis" should it overwrite "Something cool by DD32" which i installed from elsewhere?

Another option for this, Is to wp_unique_filename() (well..dirname) on the dest. folder, That'd have the benefit of no conflicts, However, It does have the downside that the plugin upgrader (still) doesnt respect the plugin folders name (ie. it creates a new folder based on the zip name, or the .org slug in the .zip rather than using the current folder (eg. core-control installed in my-plugin-cc/ will be renamed))) - So that basically rules that out at this stage..

Maybe for uploaded plugins a unique_name.. else if its from .org.. not.. But that doesnt feel right to me, since it varies depending on the use-case

#8 @Denis-de-Bernardy
11 years ago

how about this: we check if the folder exists. If so, we validate that the new plugin files return similar data as the old one (i.e. same plugin name, higher version being uploaded, not necessarily same author since it may have been maintained by someone new). If true, it's pretty safe to set the clear_destination to true.

#9 @DD32
11 years ago

  • Type changed from enhancement to feature request

how about this:

Hm. Sounds like a good idea, Basically the same thing api.wordpress.org does behind the scene..

But if thats the case, Might also need a checkbox "This is an upgrade for ...." or something. Eitherway, Its a potential candidate for a future release.

(Marking as feature request until a patch comes in)

#10 @Denis-de-Bernardy
11 years ago

I mostly see it as a means to easily update a theme or a plugin that is *not* in the repository. :-)

#11 @DD32
11 years ago

I mostly see it as a means to easily update a theme or a plugin that is *not* in the repository. :-)

Yeah, Thats what i meant, BUT my point was that what your idea is, Is exactly what the API does for wordpress hosted plugins.

#12 @hakre
11 years ago

Additionally it would be nice that it's possible to upgarde the whole WordPress installation by providing a zipfile. Is somehow related to the original suggestion. Just overwriting every file that already exists to get a clean start-over.

#14 @Denis-de-Bernardy
11 years ago

dup with patch: #11915

#15 @cyberhobo
11 years ago

  • Cc cyberhobo@… added
  • Keywords dev-feedback has-patch added; needs-patch removed

The dup was mine. I updated my patch after reading this discussion to incorporate some extra checks. But more important, I think, it's an illustration with a bare minimum UI that I find helpful for getting a feel for the possible pros and cons of this feature.

My one addition to the need for the feature is related to #11850. Briefly, a user with a ZIP who fails to upgrade a plugin via upload might be tempted to delete the existing plugin, which might have an uninstaller that deletes their data.

So, without getting into the implementation, I wonder if this illustration can help us decide for or against this feature? I myself am for it, but I'd just like the bare minimum to address my above concern.

#16 @scribu
10 years ago

@cyberhobo: As long as the UX is identical to the normal update process, I don't see the problem.

+1 for this feature.

#17 @scribu
10 years ago

By the way, I think it wouldn't be that hard to allow multiple plugin/theme upgrades through a single .zip file.

Here's how it could work (for plugins):

  1. Unzip archive.
  2. Scan all extracted files for plugin headers.
  3. Match any found plugins with the currently installed plugins.
  4. Show a confirmation screen to the user about which plugins would be upgraded.
  5. Do normal upgrade procedure for matched plugins.

#18 follow-up: @cyberhobo
10 years ago

I'll see if I can find some time to look at this again.

Re: Step 4, I recall thinking it would be harder to add an extra exchange like a confirmation (in addition to being a UX change). My compromise was to add an "upgrade existing plugin(s)" checkbox to the initial upload form instead (also a UX change).

The other question I recall having is that I needed data like get_plugins() returns, but get_plugins() won't run on the unzipped archive in the working directory. I'm not entirely clear why that is, or if it would be worth adapting that code to this purpose rather than repeating much of it.

#19 in reply to: ↑ 18 @scribu
10 years ago

Replying to cyberhobo:

The other question I recall having is that I needed data like get_plugins() returns, but get_plugins() won't run on the unzipped archive in the working directory. I'm not entirely clear why that is, or if it would be worth adapting that code to this purpose rather than repeating much of it.

Better to make get_plugins() more general than have duplicate code.

#20 @hakre
10 years ago

Related: #16923

@cyberhobo
9 years ago

Refresh, improvements

#21 @cyberhobo
9 years ago

Got some work done at WordCamp Reno today:

  • updated to current trunk - still works!
  • modified get_plugins() to take an alternate plugin root. I skip caching in that case because it doesn't seem likely to be done more than once.
  • doesn't do multiple upgrades from a zip, but that still seems within reach

#22 @SergeyBiryukov
7 years ago

  • Description modified (diff)

Related: #27196

This ticket was mentioned in Slack in #core by clorith. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by clorith. View the logs.


6 years ago

#26 @countrymusicchicago
5 years ago

This would be a huge time saver.

#27 @Doobeedoo
4 years ago

7 years ago, it's still impossible to update a plugin with the .zip ?

#28 @pingram3541
4 years ago

+1 on this. Still very much a painful process to update plugins not in repo or don't have their own update mechanism, not every site has wp-cli and its just that much more work added to our lives to move from already being logged into the back end to using a third party client to update plugins. I'm actually surprised a plugin for this doesn't already exist =)

Last edited 4 years ago by pingram3541 (previous) (diff)

#29 follow-up: @bfintal
4 years ago

Perhaps we can merge parts of this plugin into core: https://wordpress.org/plugins/easy-theme-and-plugin-upgrades/

#30 @ocean90
4 years ago

#38139 was marked as a duplicate.

#31 @kda406
4 years ago

I cannot believe this is not already in the core, and hasn't been added for all these years. This would be a huge time and bandwidth saver. No need to download the same plugins for every one of my sites over and over.

#32 @nimmolo
4 years ago

+1 for add to core. The plugin mentioned by @bfintal works great, but this seems like a necessary core feature. Sites hosted on servers without FTP access are too much of a chore to update.

#33 @lukecavanagh
4 years ago

Seems like a solid core feature for 4.8.

#34 @rinkuyadav999
4 years ago

+1 for add to core.

#35 @pcpro178
4 years ago

+1 for add to core.

#36 @zubaka
4 years ago

+1 to add this feature to core

#37 @swissspidy
3 years ago

#40561 was marked as a duplicate.

#38 @folletto
3 years ago

+1 to this.

I'd also add that the lack of this feature makes impossible to update an active theme without disabling it first and deleting it, and disabling a theme means creating a gap for the site, plus it might lose customizations settings (widgets dropped / moved to a different sidebar).

If a user doesn't have SSH/SFTP access and wants to update an active theme, the lack of this feature makes it impossible without disrupting the site.

#39 @ahortin
3 years ago

Good to see this getting some traction after 8 years. This would be a huge time-saver, not to mention how much more convenient it would make updating themes & plugins. It's ridiculous that it's not already in core.

#40 @theMikeD
3 years ago

+1 to this. Is anyone "owning" this feature?

#41 @nimmolo
3 years ago

@rockwell15 PLEASE can we schedule this for the next point version. (Plugin and theme updates from .zip upload)

#42 @swissspidy
3 years ago

  • Keywords needs-refresh added

@nimmolo Unfortunately it's not that easy.

This ticket definitely needs a refreshed patch and lots of testing (perhaps even a unit test).

If anyone wants to work on this, feel free. Right now, this doesn't have priority though as it's not one of the current focuses of the project.

#43 @kda406
3 years ago

@swissspidy You are missing the point. Many people need this and have been requesting it for 8 years now. Enhancements - no I'm going to call this a fix because it should have been in there from the beginning. Fixes with wide spread need which have been long awaited, should have developer precedence.

When a new version of WordPress comes out, I always read to see if they have fixed this problem yet. Then I am disappointed when I learn they have wasted precious coding time and resources for unimportant "improvements".

Security related patches are the only things that should have higher priority over widely needed, log awaited fixes like this one.

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.


3 years ago

#46 @melchoyce
3 years ago

Proposed workflow:

  • Drag and drop an updated zip file for the theme you're updated directly onto themes.php. Get an Shiny Updates notice on the theme being updated. Boom, updated!
  • Also, add some sort of "update theme from zip" button to the theme details modal.

I'll sketch out a flow for this.

Last edited 3 years ago by melchoyce (previous) (diff)

#47 @pingram3541
3 years ago

So excited this is finally getting traction! I just wanted to add that there is also great value in the ability to also roll back a version via zip upload as well, not just update so maybe this can be considered as well?

Also, should there be any consideration for retention of what is overwritten? A download link?

Last edited 3 years ago by pingram3541 (previous) (diff)

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


3 years ago

#49 @FolioVision
3 years ago

+1

Reduces dependency on WordPress.org plugin directory and at this point that's a good thing. As plugin developers in an area of constant change (video), we are regularly asking customers to delete the existing plugin to install a custom beta version. Zip updates would definitely make customers lives easier.

As swisspidy notes, it's harder than it looks although the code to do updates right must exist on WP.org's server and could be made to work locally.

#50 @ronalfy
3 years ago

Definitely a +1 here.

We recently ran into this situation with a client site and the only workaround was renaming the folder and deleting the old theme.

#51 @StephenCronin
3 years ago

+1 for this.

It seems to be quite common that real users try to update using the zip upload (install) feature and therefore get the "destination folder already exists" message.

If you do a Google search, you'll see there are a bunch of articles about this, including FAQs from theme and plugin authors, which shows they get lots of users asking their support teams about this. Likewise there are a bunch of topics on the Support Forums.

At minimum we should warn users that this won't work before they attempt it. Ideally, we'd make it work, or come up with a solution such as the one Mel proposed.

All those articles tell users to update via FTP and although that will work, I'm sure core can offer a better user experience than that.

This ticket was mentioned in Slack in #design by melchoyce. View the logs.


3 years ago

#53 @melchoyce
3 years ago

Relevant Twitter conversation:

https://twitter.com/andrea_r/status/879779357878886401

https://twitter.com/andrea_r/status/879841515010445313

(I wish these embedded...)

I definitely think we should allow both uploading new themes, and updating existing themes, via drag-and-drop (#24579), but maybe we don't need to add new UI for updating — maybe having people go through the existing upload flow is fine.

#54 @rinkuyadav999
3 years ago

Too much +1 for this :)

This ticket was mentioned in Slack in #design by folletto. View the logs.


3 years ago

#56 @folletto
3 years ago

I agree, the drag'n'drop flow is going to be useful regardless of the specific flow as it's a speed/advanced feature, and we can start by allowing update with the same UI for the rest, and iterate later if we prefer to improve the design further.

#57 @ahortin
3 years ago

The ability to upload/update an existing theme & plugin, even using the existing workflow would be a million times better than not being able to do it, which is what we have now.

So excited that this is finally getting some traction! *Squeeeee!*

#58 @alex-ye
3 years ago

It would be nice feature to add into the core.
I had built those two simple plugins 1 year ago, hope it helps. :)
https://wordpress.org/plugins/reinstall-themes/
https://wordpress.org/plugins/reinstall-plugins/

#59 @pento
3 years ago

  • Milestone changed from Future Release to 4.9

Soooo... this ticket hasn't made much progress this cycle. :-)

Unfortunately, the people who would usually work on these things (@dd32 and I, for example) are unavailable until late September, and that's way too late to be starting work on this feature. There are a lot of hidden complexities and edge cases to consider, it requires a deep dive into the update code, and I'd really like to see masses of unit tests (and maybe even some integration tests).

If someone can pick this up and run with it for the next 4 weeks, I can absolutely provide reviews and feedback until I'm available to join you more regularly. We have just under 6 weeks until the feature merge deadline, that should be sufficient to build this, and give it time to soak. If doesn't have any movement until late September, it almost certainly won't make it into 4.9.

If you're thinking about jumping in, but aren't sure where to start, here are some steps to think about.

  • Go through the install/upgrade related screens many times, so you know exactly how it currently works.
  • Take screen shots, post them to make/test, link the post here.
  • Write about exactly how you think the UX flow should work. We have plenty of UX experts subscribed to this ticket who can give you feedback.
  • Iterate on the flow. Think about the edge cases - what will make it weird? What will make it bad? What will make it break?
  • Write a prototype. A plugin would be easier to test, but harder to merge - don't worry about that now.
  • Post more screenshots and video to make/test.
  • Iterate.

If any of these aren't your strong point, don't sweat it - there are a bunch of people here who will help you. If you can only dedicate time to part of it, that's cool! There are a bunch of people here who can join you to keep things moving.

If the ticket gets this far, it's on track to get it merged in 4.9! Good work, friends! You're awesome people. :-D

#60 @dd32
3 years ago

As @pento said above, I'll review any attempts made here - but with the caveat that I can't do so until the last week of September at the earliest.
If this were a super-simple thing to add, I'd have added it a long time ago, unfortunately there are a lot of edge-cases that are not immediately obvious - IIRC it's not merely 'connector' code required, but something a lot deeper.

The update code also differs significantly from the install code - Install handles various archive structures and is far more forgiving than the update handler.
For example, currently updates require the ZIP file structure match *exactly* what W.org produces - however most non-W.org ZIP files do not match in every respect, and causes problems for 3rd-party updates occasionally (until they get the structure "right"). The installer code handles the various archive structures a lot better, but still gets complaints sometimes (for example, ending up with a directory of wp-content/plugins/plugin-name.0.1.2.3.tmp/ under certain circumstances)

IMHO: Any change here will need to touch on the main update handlers to avoid code duplication, and the changes there will need deep testing, as there's no current unit testing, integration testing, or any other kind of testing in place there for any install/update actions.

(Some of the comments from 4+ years here will be completely irrelevant to any effort too, take them with a grain of salt, but verify any limitations mentioned)

This ticket was mentioned in Slack in #design by melchoyce. View the logs.


3 years ago

#62 @JoshuaWold
3 years ago

In order to catch myself up on the problem that's happening here, I've created a user flow with screenshots: https://make.wordpress.org/test/2017/08/27/updating-plugins-with-zip-uploads/.

It took a while to understand the issue, so hopefully these screenshots will help others.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


3 years ago

#64 @dnavarrojr
3 years ago

Ugh, getting punted again...

#65 @psykro
3 years ago

@pento @dd32 I'm keen to see if I can either own this, or do as much as possible in the next four weeks to push it forward. At this point I have some questions

  1. Does it make sense to start with only making this work if the user uploads an updated plugin zip from the Plugin update screen? There has been some discussion around implementing drag/drop functionality but that seems to me to be a good feature for a future release.
  2. The current process only checks for a matching folder structure, is this enough of a check? My thought is that it could do the following to determine whether to upgrade or install
    • check for a matching folder
    • if not ground check for an installed plugin with the same Plugin Name
  3. Does the upgrade process need to be more complicated than
    • delete old plugin files
    • extract new plugins files

#66 @Clorith
3 years ago

It should be more complicated than that, yes.

One thing is if you hit an "invalid" structure, as mentioned in a previous comment.

My concern that needs ot be addressed is when you take into consideration things such as like folder name (slug) matching. Someone made my-plugin as the slug for My first plugin, but there's already a my-plugin used by the Mittens by York Plugincompany who prefixed their plugin with my followed by plugin as the slug.

What happens if I accidentally grab an older version and upload it, should it automatically replace everything, should I be notified that I'm about to perform a downgrade (I think I should be).

#67 @psykro
3 years ago

@Clorith thanks for the feedback, noted regarding uploading older versions.

My question around the actual upgrade process was more along the lines of 'WordPress has already determined (by means to still be discussed) that this plugin can be upgraded'.

So once the something like 'can_plugin_be_updated()' returns true, does the physical action of upgrading the plugin need to be more complicated than something like

  • rename current plugin directory
  • upload and unzip new plugin
  • delete renamed plugin directory

I'm still going to follow @pento's suggestions of diving deep into the code and taking screenshots, but I'm just clarifying some initial questions/suggestions that pop into my head.

In my mind the checks for upgrading a plugin should be a combination of the plugin slug and Plugin header meta info for example

  • Plugin name
  • Version
  • Text Domain

However I realise that the only field that a plugin actually requires is the Plugin Name

My initial feeling is that if the plugin directory name and Plugin Name meta match, consider the plugin to be upgraded. If the plugin has a valid Version field, check against that, if not warn the user of a possible 'downgrade'.

#68 in reply to: ↑ 29 ; follow-up: @joyously
3 years ago

Replying to bfintal:

Perhaps we can merge parts of this plugin into core: https://wordpress.org/plugins/easy-theme-and-plugin-upgrades/

As stated a year ago, this plugin has existed a long time and works just great. Why not use what's already been tested?

#69 @psykro
3 years ago

@joyously thanks, I will install that as well and consider it based on the other concerns.

Out of interest sake, what does it take for someone to 'own' a ticket, does it require a chunk of work/discussion to be completed first, or is it just a case of someone making me the owner?

#70 follow-up: @Clorith
3 years ago

A ticket owner is really just someone who drives the ticket, you don't need to own it to work on it.

#71 in reply to: ↑ 70 @psykro
3 years ago

Replying to Clorith:

A ticket owner is really just someone who drives the ticket, you don't need to own it to work on it.

Gotcha, thanks @Clorith.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


3 years ago

#73 @psykro
3 years ago

Right, first attempt at suggesting a possible way forward. To my mind, the simplest way would be to check the zip being installed against any existing possible matches, before the actual file is uploaded.

https://jonathanbossenger.com/wp-content/uploads/2017/09/Add-Plugins-Step-2-Confirmation.png

So on adding a new plugin and clicking the Install Now button

  1. Make a WP Rest API call to check if a plugin with either a full or partial slug match exists
  2. Display the 'This plugin you are attempting to install seems to exist' message to the user
  3. Display the matching plugin details (name, description and version number)
  4. Move the Install Now button under the 'confirmation' message and change it to 'Accept and install'
  5. On clicking the install button the usual upload and install process proceeds

Things to consider

  1. During the upgrade process, the plugin being overridden should probably be renamed/moved to a temporary location, in case the plugin install fails and a rollback is required (sometime like rename to _plugin_name
  2. Once the new plugin has successfully been installed, clean up the temporary location at step 1 above.

This ticket was mentioned in Slack in #core by jon_bossenger. View the logs.


3 years ago

#75 in reply to: ↑ 68 @psykro
3 years ago

Replying to joyously:

Replying to bfintal:

Perhaps we can merge parts of this plugin into core: https://wordpress.org/plugins/easy-theme-and-plugin-upgrades/

As stated a year ago, this plugin has existed a long time and works just great. Why not use what's already been tested?

The plugin does handle the uploading of a plugin with the same (or similar) plugin slug, by moving and renaming the existing plugin and installing the new plugin.

So I suppose the question is, by adding the pre upload/install notification as outlined above, is that enough of a check against uploading a previous version of the same plugin or uploading a different plugin with the same or similar slug?

#76 @psykro
3 years ago

Further reading on the the current plugin suggests that it could simply be merged into core as is.

Attention: Version 2.0.0 changed the functionality of the plugin. You are no longer required to select “Yes” from a drop down before the theme or plugin can be upgraded. The need for an upgrade is now detected automatically. So, if you are used to the old functionality of the plugin, do not be concerned about the absence of upgrade details on the theme and plugin upload pages. Simply upload the theme or plugin as if you were installing it, and the plugin will automatically handle upgrading as needed.

However, setting up a real world test, with the same plugin slug but very different contents and the new plugin is uploaded over the old one. So the first plugin step check and verification by the user appears to be required.

Last edited 3 years ago by psykro (previous) (diff)

#77 @Clorith
3 years ago

Plugin slug check is definitely required, also remember that the name of a zip file may not be the plugin slug, the zips generally contain a folder with the plugin slug, so an API check before uploading won't be enough here.

#78 @psykro
3 years ago

Thanks @Clorith

If we uploaded the file asynchronously, using the same javascript library as the media library (which uses plupload as far as I know) this would give us the ability to perform the API check on the plugin slug.

So to update my proposal above, on clicking 'Install Now'

  1. Upload plugin zip asynchronously to a temporary location in the uploads directory, showing am 'upload' progress meter
  2. Once uploaded, unzip the plugin to use the plugin slug (folder name) to make a WP Rest API call to check if a plugin with either a full or partial slug match exists
  3. Display the 'This plugin you are attempting to install seems to exist' message to the user
  4. Display the matching plugin details (name, description and version number)
  5. Move the Install Now button under the 'confirmation' message and change it to 'Accept and install'
  6. On clicking the Accept and install button the plugin is installed

Things to consider remains the same.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

#80 @westonruter
3 years ago

  • Priority changed from low to high

Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

#82 @johnbillion
3 years ago

  • Keywords early added
  • Milestone changed from 4.9 to Future Release

This is making good progress, but it needs to go into a release cycle earlier on for testing purposes. Punting to 5.0.

@imath
3 years ago

#83 @imath
3 years ago

  • Keywords needs-refresh removed

Hi,

As i'm currently working on this feature for one of my plugin, i'm suggesting this alternative approach. I agree it would be interesting to upload / unzip / explore the uploaded plugin into another temporary folder then ask the user if he wants to update a matching existing plugin with the uploaded one, delete the old plugin, move the temporary folder etc... It would probably allow to check if the version is upper etc..

On the other hand, I think it's a lot of file manipulation and probably edits into the Plugin Upgrader class. The alternative approach is to consider a replace process instead of an update process. There can be cases when you updated a plugin, something's going wrong with your site's configuration and you need to come back to a previous version of the updated plugin to fix the issue with your site.

In 9757.1.patch, i'm using JavaScript to get the Zip filename and use this as the plugin's slug to check for existing plugin thanks to a Rest API request. If there is no match, it doesn't change anything to the process, if there's one, then it adds some information before the "Install Now" button and a "Cancel" button after it to reset the form, see screenshot beneath.

https://cldup.com/lZ4aB9e_0W.png

When the user clicks on "Install Now" the process is very similar to an update, and there is very few edits to Plugin_Upgrader::upgrade(). As the process is a bit different, strings need to be a bit different in this case though, because we don't know if the user is upgrading or downgrading the plugin. That's why the patch is taking this in account as you can see beneath :

https://cldup.com/HcneJO4PUj.png

"Replacing the installed version" & "Plugin replaced successfully" are the new strings.

NB: the patch is using the wp.apiRequest (introduced in 4.9, btw thanks a lot and congrats to people who worked on it 👍) And the script where is located this API is not loaded on multisite configs. There's probably a good reason for this but at the same time as some widgets are using it, i guess they could fail on multisite configs..)

If you think it's an interesting approach, I can give a hand during 5.0 dev cycle to carry on improving the patch and adding Themes support. Else, i'll be happy to give my feedbacks on @psykro 's approach.

#84 @ahortin
3 years ago

@imath Using the zip filename as the slug is not a great idea. A lot of 'premium' plugins typically have the version number as part of the zip filename. As an example, two very popular plugins Elementor Pro and Gravity Forms, have the following filenames for their most recent versions, elementor-pro-1.9.3.zip and gravityforms_2.2.5.8.zip, respectively.

Using these filenames as the slug would mean that when you checked for an existing version, it would be looking for the wrong slug as the actual folder names within each of those zip files I mentioned, are elementor-pro and gravityforms.

#85 @imath
3 years ago

@ahortin

Thanks for your feedback, yes i know WordPress & GitHub are also doing so. I trust site admins and i think they are smart people :)

  • They can rename a package,
  • Plugin Authors can add a line in their description/documentation "If you want to update manually rename the package to elementor-pro.zip"
  • The help tab of the administration screen can also include a section to explain how to rename a zip file containing the plugin version.

We could also sanitize the filename from the JavaScript file. But if you think it's not a great experience and it's best to do all the file manipulations on the server, i'm fine with it and i totally understand.

#86 @Clorith
3 years ago

We can't expect users to rename their files to do an update, that's a bad experience for the average user just looking to update their theme or plugin like they were told to do.

Your concern for reverting a plugin if an update is bad is legitimate though, we could possibly replicate the new code editor behavior of making a call to the site and checking if it fails, at least you won't end up with a white screened site then?

@imath
3 years ago

#87 @imath
3 years ago

@Clorith

Ok then we could use the FileReader API with the external JSZip library as i'm suggesting in 9757.2.patch. This way the real file structure contained into the zip package could be checked, no matter the name of the zip file. In the following test I've renammed preferred-languages.zip to RandomName.zip :

https://cldup.com/thgYNoxDiR.png

#88 @ahortin
3 years ago

I don't think renaming the zip file is a very good user experience. If someone selects preferred-languages.zip to upload, and then you go and rename this to RandomName.zip, it's going to cause confusion. The user isn't going to be expecting their filename to get changed and if they see a different filename being displayed and uploaded, from the one they selected, it's going to cause them concern.

#89 @imath
3 years ago

@ahortin

It was an example to show that the real zip content was taking in account so of course it works with preferred-languages-1.2.4.zip etc... The patch doesn’t rename the file at all. Feel free to test it btw :)

Sorry i caused a confusion here.

#90 @bloggingthemes
3 years ago

I certainly hope this ticket will keep progressing. One of the biggest user experience one could hope for is to give them the method of updating an existing theme and/or plugin simply by re-uploading using the installer. Most users are scared of FTP (I don't blame them), but using a third party plugin is not ideal because who knows how long it will be kept up-to-date and supported.

This is definitely important to exist within the core and should have been there since the beginning. I get so many users say to me:

  • How come I can't update my theme using the installer?
  • How come I can update with 1-click themes and plugins I get from wordpress but not yours?
  • Or they use the installer and get that "already exists" message and get frustrated.

Overall, I hope this gains ground for an upcoming WordPress release because people need this.

I've been using Joomla for 10 years and they've always had this capability. It also doesn't matter what the zip file name is either because it looks at the file's xml info. In the case of WordPress, I would assume this would be a similar equivelent of the text-domain and version number of the theme or plugin. Ultimately, it shouldn't matter if the theme or plugin was installed from .org or elsewhere, the fact is that for a much better user experience, we need this.

Hopefully this makes it in an upcoming release.

#91 @pento
21 months ago

#46223 was marked as a duplicate.

#92 @psykro
14 months ago

  • Owner set to psykro
  • Status changed from new to accepted

#93 @galbaras
11 months ago

From what I can see, there are at least 110,000 active installs of plugins overriding the install function in the quick and dirty manner. The above proposal will give them a better solution.

Also, why not leave the install alone and ADD a function to upload a file for the purpose of updating the plugin/theme? Then, WordPress can assume this is an update and act accordingly, and there's no confusion anywhere.

There can be a new page for selecting a plugin to update and a file to upload, but the easiest way would be to add an "update from file" link on the Installed Plugins page, making it clear in advance what's being updated.

This ticket was mentioned in PR #267 on WordPress/wordpress-develop by mariovalney.


5 months ago

<!--
Hi there! Thanks for contributing to WordPress!

Pull Requests in this GitHub repository must be linked to a ticket in the WordPress Core Trac instance (https://core.trac.wordpress.org), and are only used for code review. No pull requests will be merged on GitHub.

See the WordPress Handbook page on using PRs for Code Review more information: https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/

If this is your first time contributing, you may also find reviewing these guides first to be helpful:

-->

I would like to propose a simple solution to most use cases: allow user to overwrite plugins/themes.

Some points to discussion:

  • It's not automatic so is a user decision and probably will be checked after a error from default case.
  • I love the "Decisions, not Options", but I guess this is a acceptable decision to be taken by anyone uploading a plugin by zip.
  • Maybe add a tooltip description? I'll search a example in admin to keep UX.

With this, we:

  • Fix layout for big "Install now" translations (like the Brazilian "Instalar agora").
  • Fix layout for mobile help text.
  • Accept only zip as plugin upload is not able to handle only-file plugins.

Trac ticket: https://core.trac.wordpress.org/ticket/9757

#95 @mariovalney
5 months ago

Hey :)

I want to help on this ticket.

By now I would like to propose a simple solution: allow user to overwrite plugins/themes.

Details are in the PR comment above.

Some screenshots:

https://i.imgur.com/D3tqklp.png
The overwrite option.

https://i.imgur.com/A0emYWE.png
The layout problem.

Btw: too cool the new trac feature.

Last edited 5 months ago by mariovalney (previous) (diff)

#96 @folletto
5 months ago

I think that regardless of all the intricacies of the years old discussions there's a "first step" approach we can move forward here, which is allowing an overwrite with explicit confirmation.

In short:

  1. If the theme/plugin doesn't exist, just do as it does today
  2. If the theme/plugin does exist, show a screen asking "Are you sure you want to overwrite?"

There are some complexities in the second case (reading the zip, cleanup of folders, etc) for sure. From a UX perspective, the ideal scenario is to ask, and show at least the version number, name, and general metadata of both so the user can take a decision.

#97 @mariovalney
5 months ago

I agree.

Actually my first thinking was to add a "override anyway" after the error message in the uploading page. But it was too complicated to a first purpose without discussion haha

If we agree on follow this path I'll would be happy to refact the patch/PR.

About UI, I guess we can use the output we already have, just adding a link to allow the overwrite and a simple table with the comparison you suggested (at least to this first see).

#98 @folletto
5 months ago

If you've already an idea just post the screenshots of the new flow and we can move from there :)

#99 follow-up: @mariovalney
5 months ago

It's just a "console editing" version. So I'm not sure about implementation, but I guess it's possible.

https://i.imgur.com/gozn9tf.png

Last edited 5 months ago by mariovalney (previous) (diff)

#100 in reply to: ↑ 99 ; follow-up: @pingram3541
5 months ago

Replying to mariovalney:

It's just a "console editing" version. So I'm not sure about implementation, but I guess it's possible.

https://i.imgur.com/gozn9tf.png

This is great. I'd just like to add that this would be convenient if it also allowed older versions to "Override anyway" making a roll-back just as easy.

#101 @folletto
5 months ago

That seems to me a good way forward by keeping it very grounded for a ticket this old that is easily sidetracked. :)

I would just suggest a slight change of language for the actions:

  • Red → "Overwrite with uploaded" on the right
  • Blue → "Cancel and go back" on the left

I'm suggesting to flip order so the two actions are order-wise associated with the columns right above, which makes it easier to understand: left column and left actions are both "current", while right column and right action are both "uploaded".

Also similarly added the same word inside the red button, so the label of the column matches the label on the button, and for the blue one it makes clear nothing happens ("cancel") and the outcome ("go back").

Also agreed to allow downgrades too.
It should show it in ANY override case (i.e. every time a folder with the same name exists).

That's a good direction!

#102 @pingram3541
5 months ago

+1 on that, makes sense that the order of presentation matches and the instruction is very clear.

#103 in reply to: ↑ 100 @mariovalney
5 months ago

Replying to pingram3541:

This is great. I'd just like to add that this would be convenient if it also allowed older versions to "Override anyway" making a roll-back just as easy.

I agree. There is no validation about version: that's why I'm using "override".

Great suggestions @folletto
People from design are in another level haha :)

I guess I'm able to finish this approach today. o/

Last edited 5 months ago by mariovalney (previous) (diff)

#104 follow-up: @organizedthemes
5 months ago

I would advise that you make it work like the Easy Theme and Plugin Updates plugin, which allows us to overwrite the current plugin/theme and then it actually adds the Older version of that plugin/theme as a .zip file in the Media section in WordPress. Smart and works well in case we need to revert. It's a backup.

#105 in reply to: ↑ 104 @mariovalney
5 months ago

I like your suggestion but I guess we can keep things simple by now.

Besides that user already have all the information to do a right choice. It's like add a backup option on deleting plugin.

Maybe we could possibly think about replicate the code editor behavior of making a call to the site and checking if it fails, like Clorith suggested?

Last edited 5 months ago by mariovalney (previous) (diff)

#106 follow-up: @mariovalney
5 months ago

OK. That was not trivial. hahaha

I would really like a core developer feedback, because I'm not sure implementing this in the "Install Skin" is the best approach. But I was not able to figure another way (at least, without change much more code).

1 - I really tried to keep this for "upload" only.

2 - Would we trigger any action like in upgrade?

I guess don't. Because we are not keeping this for upgrade only. In this case, let's deactivate/activate the plugin?

I added a action on overwriting completed to allow developers do whatever they want on "manual update".

3 - I added some CSS. Is that the best place?

4 - As said, we are not checking versions. But we are showing comparison table only if we found plugin data in existing directory and after validating the uploaded package is a plugin.

5 - Maybe implement a new action on update.php to check a new nonce for this package ID?

This will not allow user overwrite a package by adding parameter on URL.

How the table looks like:

https://i.imgur.com/iT2JNGe.png

Note: themes are very like plugins in this case, so let's do the theme part after eveything is considered about plugins.

Last edited 5 months ago by mariovalney (previous) (diff)

#107 @mariovalney
5 months ago

  • Keywords needs-screenshots added

#108 follow-up: @galbaras
5 months ago

The screenshot looks like the installation is attempted initially and then fails. However, if WordPress now allows overwriting, it's no longer a failure, so perhaps the wording should change there.

Also, since the versions are displayed in a table, why not insert the links into the table for clearer alignment of what is being selected?

Finally, I'd style the links as buttons, because they are actions, not navigations. Button classes can then be "button-primary" (white on blue) for cancelling and "button-secondary" (ghost) for overwriting.

#109 @psykro
5 months ago

  • Owner psykro deleted
  • Status changed from accepted to assigned

#110 @folletto
5 months ago

The screenshot looks like the installation is attempted initially and then fails. However, if WordPress now allows overwriting, it's no longer a failure, so perhaps the wording should change there.

I think it's important to show the attempt. I think it's good to show it tried, and found a conflict, and it's asking on how to resolve it.
Granted, I would love for the UI to be less "terminal like", but it's work that has to be done in general for all the upgrades, and shouldn't be in scope for this specific ticket.

Similar answer for the other bits of advice. I know the suggested UI is not perfect, but it's clear and aligned to what we have today. I feel this is especially important for this specific issue as it's 11 years old and got sidetracked multiple times with extensions of scope. If we have a chance to ship this, let's focus.

I'd suggest if you've ideas on how to improve the upgrade screen to open a separate ticket and work on that. :)

#111 @folletto
5 months ago

I'll leave the dev questions to devs, as for the others:

As said, we are not checking versions. But we are showing comparison table only if we found plugin data in existing directory and after validating the uploaded package is a plugin.

This seems ok to me... I mean, if it's not a plugin and it has no metadata, it shouldn't even try, no?

This will not allow user overwrite a package by adding parameter on URL.

This seems alright in general, but I'm not sure what this means exactly. Can you clarify?

#112 in reply to: ↑ 108 @mariovalney
5 months ago

Replying to galbaras:

The screenshot looks like the installation is attempted initially and then fails. However, if WordPress now allows overwriting, it's no longer a failure, so perhaps the wording should change there.

That's the idea: we had a failure.
Maybe we can change "more details" to something sounds like "you can fix this way". But I guess a "overwrite this" after a "destination already exists" error do the job.

Replying to folletto:

This seems alright in general, but I'm not sure what this means exactly. Can you clarify?

The "overwrite link" add a parameter to URL. If user add it by hand will do the same without the comparison table, but I guess we can ignore that because the user came from another page and "reload" doesn't work... Rethinking now I was overconcerned hahaha

#113 follow-up: @joyously
5 months ago

but I guess we can ignore that because the user came from another page and "reload" doesn't work.

What about with WP-CLI?

#114 in reply to: ↑ 113 @mariovalney
5 months ago

Replying to joyously:

What about with WP-CLI?

I'm not sure WP-CLI uses this same code, because it runs on "wp-admin/update.php".
But I'm not sure how WP_CLI works too... So I'll take a better look.

Thanks for point this.

Last edited 5 months ago by mariovalney (previous) (diff)

#115 @folletto
5 months ago

Maybe I'm missing something, but WP-CLI seems a different task from the one mentioned here.
Unless there's something simple to do to add support, I'd try to not expand scope further, and save it for later.

#116 in reply to: ↑ 106 ; follow-ups: @azaozz
5 months ago

Replying to mariovalney:

The PR is looking good. Some quick notes:

The screenshot looks like the installation is attempted initially and then fails.

That's the idea: we had a failure.

Right, but instead of "failed" it may be better to ask if the user wants to update imho. Then it would go: Installing... The plugin is already installed, do you want to update it? Then the compare table.

I added some CSS. Is that the best place?

Yes, seems okay.

As said, we are not checking versions. But we are showing comparison table only if we found plugin data in existing directory and after validating the uploaded package is a plugin.

Wondering if it should have some sort of warning when "reverting" to an earlier version? Also, it's pretty rare, but there may be plugins with matching names (where one is in the plugins dir and the other is not). Perhaps trying to match more of the plugin meta/headers may be good?

Maybe implement a new action on update.php to check a new nonce for this package ID?

This will not allow user overwrite a package by adding parameter on URL.

May be nice to have but not strictly necessary imho. The plugin-upload nonce is already there so the only way for this to happen would be to manually add the URL query arg. Perhaps can make it a bit more unique, something like add_query_arg( 'overwrite', 'ovewrite-uploaded-plugin', $this->url ).

#117 in reply to: ↑ 116 @galbaras
5 months ago

Right, but instead of "failed" it may be better to ask if the user wants to update imho. Then it would go: Installing... The plugin is already installed, do you want to update it? Then the compare table.

Totally agree. That's what I meant earlier, in case this wasn't clear.

Wondering if it should have some sort of warning when "reverting" to an earlier version? Also, it's pretty rare, but there may be plugins with matching names (where one is in the plugins dir and the other is not). Perhaps trying to match more of the plugin meta/headers may be good?

There has to be a limit to how much WordPress will do to prevent absolutely every possible issue. Let's assume that if someone hasn't paid attention at first and tried to install a conflicting plugin, they're going to pay attention when the page tells them there's an issue.

What are the chances that a plugin will have both the same name/title AND the same directory name?

As for downgrading, I think that's the main reason for this thread. That, and recovering from an incomplete/corrupt installation.

#118 in reply to: ↑ 116 @mariovalney
5 months ago

Replying to azaozz:

Right, but instead of "failed" it may be better to ask if the user wants to update imho. Then it would go: Installing... The plugin is already installed, do you want to update it? Then the compare table.

The "folder_exists" and "process_failed" feedback are printed on general upgrader.
It's a lot of changes to achieve that.

I guess it's not a problem an error followed by an retry, so what about changing the UI we can control on specific case (Plugin Install Skin)?

Wondering if it should have some sort of warning when "reverting" to an earlier version? Also, it's pretty rare, but there may be plugins with matching names (where one is in the plugins dir and the other is not). Perhaps trying to match more of the plugin meta/headers may be good?

I was thinking about "overriding" instead "upgrading/downgrading".
It's very plausible after a "folder exists" error... But I agree we should go with the general case (upgrade/downgrade)...

So I improved the feedbacks and the UI.

Now we have a diff alert for all fields:

https://i.imgur.com/F9At0Ox.png

Except for Version, like upgrade is the secure case:

https://i.imgur.com/95qSMLF.png

But we alert about downgrade:

https://i.imgur.com/ZcJFk82.png

And a text about reuploading (same value for the 5 main fields):

https://i.imgur.com/KmHIv64.png

And thanks for the coomments on PR. Already solved :)

@folletto, I've changed the "Overwrite with uploaded" label to match the changing.

And for finish, with this changes we're able to show a better feedback on ovewritting UI:

https://i.imgur.com/qrb6WU5.png

https://i.imgur.com/UNYCnLJ.png

#119 @folletto
5 months ago

Sounds good to me. Nicely done.

#120 @mariovalney
5 months ago

I'm back for themes \o/

A regular update:
https://i.imgur.com/YfLuGyu.png

And with warnings:
https://i.imgur.com/7vb1BqS.png

And the same check for "reuploading package" like plugin:
https://i.imgur.com/cESjljY.png

But there's a note about errors on current theme:
https://i.imgur.com/gGuQBfE.png

And a check for parent theme for uploaded:
https://i.imgur.com/XVueSc7.png


I removed the "activate" action on already active plugins/themes too.

And there was a cache on theme_info that we need to clear too. No performance problem as it's expected on uploading process.

@azaozz a new code review would be appreciated.

Thanks

#121 @folletto
5 months ago

Design wise, I think it's good and we can move forward.
Well done.

In reference to earlier comments, I acknowledge this page will need more work at some point in the future (which applies to all updates in general). This is specifically to keep things simple and ship a basic feature that has been missing for 11 years. Once it ships, we can have further discussion and improve it.

#122 follow-up: @joyously
5 months ago

This looks great, but I was a little confused by the text "Remove current and install the old version", since in my mind the existing (current) one is the old one, even though the version numbers might say otherwise. I think the text used for all the other cases applies equally to the case of uploading an older version.

#123 @folletto
5 months ago

I see your point, it could trigger some confusion.

Maybe we can be even more explicit? Instead of "old" we say: "Remove current and install version 1.0.0" (the version number from above), only in the case of an "older" version being uploaded.

Would be difficult to introduce the dynamic version in the "old" scenario?

Otherwise, we can just go back to specify "uploaded", without a special case for "old".
It might work regardless given the highlight in red just above.

#124 in reply to: ↑ 122 @mariovalney
5 months ago

Done:

https://i.imgur.com/S5iW6Wl.png

https://i.imgur.com/e6b0eBN.png

Thanks for pointing this.

Last edited 5 months ago by mariovalney (previous) (diff)

#125 @folletto
5 months ago

You're a star. :)

Let's wait for the code review now! :D

#126 @galbaras
5 months ago

@mariovalney I agree. Very good work.

Can I ask that you make significant text bold? For example "Destination folder already exists" and "The parent theme is missing". The same applies to plugins.

While we're at it, the missing parent theme message can be a lot shorter, while still conveying the same message. No need for the "The current theme has the follow error". It should be enough to say "The parent theme THEME is missing. Please install it first, and then retry".

#127 @Doobeedoo
5 months ago

Hi, how to not receive email notifications every time someone comments this ticket ?

I tried :

  • Trac notifications : everything in unchecked
  • My favorites tickets : I do not have any favorite
  • Delete my comment on this ticket : I can edit it but not delete it
  • Watch this ticket : it's not enabled
  • Unsubscribe / do not follow this ticket link in email : no link like this in the email

Many thanks !

#128 @galbaras
5 months ago

Good question, albeit unrelated. Closest I can find is tweaking https://core.trac.wordpress.org/prefs/notification

#129 follow-up: @azaozz
5 months ago

  • Focuses ui-copy added
  • Keywords 2nd-opinion added

Yes, the workflow/UI is looking good. Perhaps the text strings (copy) can have another look, hoping somebody may have better ideas for there:

  • Compare before retry doesn't sound quite right imho. The users can make a decision to upgrade/downgrade at this point. They are not retrying to install again.
  • Seems like you are trying to upload a [plugin|theme] already installed.. Something like that could probably be used instead of "Compare before retry"? It comes after "Destination exists...", "Installation failed...", see the screenshot in comment 120.
  • Would be good to show "Backup your site before upgrading" warning imho.

Also not sure if this is particularly useful: https://github.com/WordPress/wordpress-develop/pull/267/files#diff-84f206c9515d9ad843b821ac8d8d526cR287-R290. As far as I see it shows if there in an error with the currently installed theme. That might be good, but is part of Site Health. Far more useful would be to know if the uploaded plugin/theme has obvious errors.

Speaking of errors, thinking it would be good to check whether the requirements for the plugin/theme are met? Like minimum WP version and PHP version. Discrepancies may get highlighted in the compare table, but seems the upgrade would still be allowed even if not compatible.

Going to ping @dd32 for a look as this patch uses install() to do upgrade, see https://core.trac.wordpress.org/ticket/9757#comment:60. Using install() is pretty much like using FTP to replace the plugin's directory but is missing several steps that are normally performed when upgrading, see https://core.trac.wordpress.org/browser/tags/5.4.1/src/wp-admin/includes/class-plugin-upgrader.php#L169.

Last edited 5 months ago by azaozz (previous) (diff)

#130 @prbot
5 months ago

mariovalney commented on PR #267:

TODO:

  • [ ] Replace "Compare and retry" with the "same plugin/theme" message.

#131 in reply to: ↑ 129 @galbaras
5 months ago

Replying to azaozz:

Speaking of errors, thinking it would be good to check whether the requirements for the plugin/theme are met? Like minimum WP version and PHP version. Discrepancies may get highlighted in the compare table, but seems the upgrade would still be allowed even if not compatible.

When downgrading or reinstalling something that's already been active, these checks should not prevent the installation from happening, even if the prerequisites aren't met, although it's good to list the issues on the screen and let the administrator make an informed decision.

#132 @mariovalney
5 months ago

Thank you for all the points.

Compare before retry doesn't sound quite right imho. The users can make a decision to upgrade/downgrade at this point. They are not retrying to install again.

User is retring the install (upgrade/downgrade) because it already failed.

Seems like you are trying to upload a [plugin|theme] already installed.. Something like that could probably be used instead of "Compare before retry"?

Nice. Added a todo.
Ooh... PR comments appears here...

Would be good to show "Backup your site before upgrading" warning imho.

Agreed! We don't show this to regular upload, but as this patches allow users upgrade/downgrade without much validation (because I thinks there's not way to garantee this is a upgrade/downgrade) would be good.

Just like core ones?


Important: Before upgrade/downgrade your plugin[theme], please back up your database and files.


Also not sure if this is particularly useful: https://github.com/WordPress/wordpress-develop/pull/267/files#diff-84f206c9515d9ad843b821ac8d8d526cR287-R290.

I was not able to be sure about which part of code are you talking about.

Maybe about the current theme error? I added it because WP treats invalid themes in another way of valid themes (another UI on list theme, for example, and not shows it on "add theme" page). So maybe makes sense to highlight that.

That might be good, but is part of Site Health. Far more useful would be to know if the uploaded plugin/theme has obvious errors.

We can add the check Clorith talked about.

Actually would be great to add this to regular install too. Is it possible? Any direction to point me?

Speaking of errors, thinking it would be good to check whether the requirements for the plugin/theme are met? Like minimum WP version and PHP version. Discrepancies may get highlighted in the compare table, but seems the upgrade would still be allowed even if not compatible.

Good point. Let's add this on "regular install" (check_package method) too?
This way we improve it and will not fall on "ovewrite install" in first place.

Last edited 5 months ago by mariovalney (previous) (diff)

#133 follow-ups: @karmatosed
5 months ago

This seems like a potentially nice adjustment to get into the next release. What is holding up the commit right now and how can we get that happening? I see ui-copy as a keyword, is this just a copy issue now?

#134 @Iamhere
4 months ago

Hello - came across this thread after find the very useful plugin "https://wordpress.org/plugins/update-theme-and-plugins-from-zip-file/"

My two cents worth would be

1) Allow us to upload multiple ZIPs at once (e.g. selecting more than just the one zip when clicking "upload from computer" - just queue them up ;-)

2) Remove the blue button after completion that says "activate plugin" as that's confusing (assuming it's already active and that this is just an update :-)

(EDIT re point 2 looks like I didn't see that when scrolling thru the above thread - it seems it's been dealt with.)

Good work team looking forward to this being core!

Best regards

Last edited 4 months ago by Iamhere (previous) (diff)

#135 in reply to: ↑ 133 @mariovalney
4 months ago

Hi!

I reviewed the "Compare before retry" change and I guess "reuploading_plugin" string is not the best approach, as this is a "after-error" procedure. Maybe: "You can check info and retry" ?

Here is the warning (I'm not sure it's the best UI):

https://i.imgur.com/9Pj0eZh.png

And waiting for feedback on:

Let's add this on "regular install" (check_package method) too?
This way we improve it and will not fall on "ovewrite install" in first place.

#136 in reply to: ↑ 133 @mariovalney
4 months ago

Replying to karmatosed:

This seems like a potentially nice adjustment to get into the next release. What is holding up the commit right now and how can we get that happening?

Hi! I guess we are just waiting for @dd32 feedback (#129) and maybe @azaozz can check we can add the "requirements for the plugin/theme" on regular install too.

I see ui-copy as a keyword, is this just a copy issue now?

I was not able to find reference on handbook... just about keyworks. For curiosity: is there any docs?

P.S.: Sorry the ping, people. Just trying to finish this.

Last edited 4 months ago by mariovalney (previous) (diff)

#137 @galbaras
4 months ago

@mariovalney The screen capture shows "You can check both plugins info and retry". I'd change this to "Please check the information below and choose how to proceed".

Also, I feel that the action links should be more closely related to the table, which refers to "current" and "uploaded". Therefore, I suggest "Keep current version (2.0.0)" and "Install uploaded version (1.0.0)".

Why is the version comparison row highlighted in red? Are you highlighting rows with different values, i.e. if the name is different, that would be highlighted too, or just the version?

Honestly, it's triggering my "fight or flight" response. How about a thin red border instead of the solid background?

@azaozz
4 months ago

#138 @azaozz
4 months ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Future Release to 5.5

In 9757.3.patch:

Thinking this is about ready. Please test! :)

@azaozz
4 months ago

Uploading - same version.

@azaozz
4 months ago

Updating to newer version.

@azaozz
4 months ago

Downgrading.

#139 @azaozz
4 months ago

I see ui-copy as a keyword

Yeah, this refers to the text that's shown on the screen, including error messages, action links, etc. Still may need some tweaks to make it as best as possible.

Why is the version comparison row highlighted in red?

It's only when downgrading, to add emphasis/draw attention.

Thinking more about it, installing or updating a plugin or theme from a zip is pretty much the same as uploading them through FTP, i.e. the site owner is in full control and can upload anything they want.

However when done through WP it can look at Requires at least and Requires PHP and give more feedback to the user before they decide to go ahead. Perhaps it should check whether the current install (WP version and PHP version) is compatible with the uploaded plugin/theme, and if not, prevent updating? This is already the case when updating from the wp.org API.

To implement this it just needs to look for Requires at least and Requires PHP in the uploaded plugin/theme readme.txt (if not present in the plugin's main file), and do a version_compare() when one or both of these are known. Then show an error and prevent updating if requirements are not met.

Also the Requires at least and Requires PHP text in the UI should probably be replaced with Requires WordPress version and Requires PHP version. Will fix.

Last edited 4 months ago by azaozz (previous) (diff)

@azaozz
4 months ago

In this case the backup warning should be replaced with "required WP/PHP version not met" error and the "Remove current and install the uploaded version" link should not be outputted?

@azaozz
4 months ago

#140 @azaozz
4 months ago

  • Keywords needs-dev-note added

In 9757.4.patch:

  • Add error messages when the uploaded theme/plugin requirements are not met.
  • Show error and prevent updating when the uploaded theme/plugin requires newer WP or PHP version.
  • Tweak few function/filter/arg names for (hopefully) better self-documenting.

@azaozz
4 months ago

Prevent update when required WP version is not met.

@azaozz
4 months ago

Both WP and PHP "requirement not met" error messages for a plugin.

@azaozz
4 months ago

Both WP and PHP "requirement not met" error messages for a theme.

#141 follow-up: @galbaras
4 months ago

@azaozz Consistency questions: I've never tried to install anything that wasn't compatible with the versions of PHP or WP I had (without overwriting, that is), so I'm not sure what happens in this case. Does WordPress block the installation?

If not, either make WordPress always check (i.e. never trust the admin), or leave these checks out of the overwriting case (i.e. always trust the admin).

#142 in reply to: ↑ 141 @azaozz
4 months ago

  • Focuses ui added

Replying to galbaras:

I've never tried to install anything that wasn't compatible with the versions of PHP or WP I had (without overwriting, that is), so I'm not sure what happens in this case. Does WordPress block the installation?

No, it doesn't block it, but there are several warnings. However when installing through the wp.org API there is quite a bit more info about the theme/plugin. As far as I remember installation is not blocked for "historical reasons", i.e. to support super old plugins that still work despite not being updated for a long time, even before installing plugins from WP Admin was possible. If we were to add this now, I'd think installing would be blocked when something is marked incompatible :)

Can unblock updating by uploading of themes and plugins marked as incompatible if need be, but thinking it's better to keep it. Think WP should respect the minimum requirements set by the theme/plugin authors.

Last edited 4 months ago by azaozz (previous) (diff)

#143 @folletto
4 months ago

Overall, I agree.

Just for recap, given this is a long thread, I'd remind this is a new feature to enable updating something existing without breaking the site. This is currently not possible, and makes people without FTP access either stuck, or forced to uninstall a theme/plugin before installing it again. This means losing configurations, settings, etc, on a live site, which makes any update experience miserable and makes people less likely to update if they are in this specific situation.

I think that the feature functionality is preserved even with the safety locks included above, even more considering these locks can be worked around fairly easily by editing the themes/plugin. If a plugin declares explicitly a minimum version, would be a mistake to allow it given what we know.


As far as the UI design goes: I think it's aligned enough with how this part of WP Admin works, and we can move forward.

I'd encourage further copy edits and refinement to be done once the feature is merged.

This ticket was mentioned in Slack in #themereview by poena. View the logs.


4 months ago

@shaunandrews
4 months ago

theme-errors

@shaunandrews
4 months ago

plugin-errors

#145 @shaunandrews
4 months ago

This seems to work as expected. I have some UI requests that I think would help focus and make the screen easier to understand — but I understand I might be asking for too much, and don't want to block to issue from merging.

  1. Can we split up the filename from the first heading? Its hard to read right now, and at a glance its not obvious what file I'm uploading.
  2. Can we maybe replace the "Plugin/Theme installation failed." line with something a little more descriptive, like "This plugin/theme is already installed."
  3. Can we right-align the first column in the table? And maybe adjust the highlighting — no need to highlight _everything_, but just the bits that are important to show the problem.
  4. I'm not sure about the use of Notice's for backing up and other issues. My mockups just use normal text.
  5. I know this flow currently uses links for the actions, but maybe we could/should switch to buttons?

https://core.trac.wordpress.org/raw-attachment/ticket/9757/Theme.png

https://core.trac.wordpress.org/raw-attachment/ticket/9757/plugin.png

This ticket was mentioned in Slack in #design by estelaris. View the logs.


4 months ago

#147 @poena
4 months ago

Are these buttons and the highlight with the red background used anywhere else in the admin?

Warnings and errors can not be indicated using color only.

I agree that table can use some more spacing, but I do not think it should prevent the feature from being added.

#148 @folletto
4 months ago

Warnings and errors can not be indicated using color only.

That's correct in general, but doesn't apply here as they aren't: the color is just highlighting the mismatch in numbers, which is textual and readable. The coloring is just an extra addition.

Also, there's extra copy both in text and on the button action.

#149 @poena
4 months ago

I understand I was not clear: I meant the red text "This plugin is already installed" and the removal of the admin notice.

The tone that is set is that the first message, in the red bold text, is more important.

Last edited 4 months ago by poena (previous) (diff)

#150 @desrosj
4 months ago

Reviewing this now. Going to stream of thought below:

  • "Remove current and install the uploaded version" seems a bit confusing to me. It makes more sense after I re-read the table of information above, but wondering if this can be improved more.
  • Should these links be buttons? If not, are these links meeting accessibility?
  • The red text for the proceed option is throwing me off. Updating is a good thing, the red makes it feel like the wrong option to click.
  • Should there be a call out that any modifications made to the files directly will be erased by proceeding?
  • Clicking cancel leaves the zip file in the media library. Should this be cleaned up (probably a follow up discussion)?

I haven't reviewed the code in depth, but it looks good at first glance, and I was unable to find a scenario that broke the process.

#151 @desrosj
4 months ago

Also, I am in favor of blocking an update when a site does not meet the PHP or WordPress requirements as specified by the plugin or theme. Allowing the update to go through will just risk breaking the user's site, or breaking it due to compatibility issues.

#152 @mariovalney
4 months ago

Hi. So happy to see this ticket going on.
Let me know if can still help somehow.

About the concern of blocking invalid PHP version, we can add this check to check_package method which validate plugin file.

This will work for both upload and overwrite.

https://i.imgur.com/N2tZuIH.png

@mariovalney
4 months ago

Avoid install plugin with wrong PHP requirement.

@azaozz
4 months ago

#153 follow-up: @azaozz
4 months ago

  • Keywords commit added

In 9757.6.patch: add the changes suggested by @shaunandrews, @poena and @desrosj. Only things I wasn't sure about are the font-size and color for the h2 (This plugin/theme is already installed), and the color/style for the "Replace current with uploaded button. (The colors, font-sizes, button style and placement can be tweaked after commit too.)

Please test as much as possible :) Not just by uploading a zip, also updating core, plugins and themes through the wp.org API, etc. Ideally auto-updates will also be tested, for core, plugins, and themes. Also, feel free to commit :)

we can add this check to check_package method which validate plugin file.

@mariovalney sure, but lets do this from another ticket (after this is committed). This is a bug in the "Install from a zip file" functionality, worth fixing even during beta imho.

Last edited 4 months ago by azaozz (previous) (diff)

@azaozz
4 months ago

@azaozz
4 months ago

@azaozz
4 months ago

@azaozz
4 months ago

@azaozz
4 months ago

@whyisjake
4 months ago

composer format updates

#154 @whyisjake
4 months ago

While testing, I am getting three new errors:

1) WP_REST_Plugins_Controller_Test::test_create_item
Undefined property: WP_Ajax_Upgrader_Skin::$overwrite

/build/wp-admin/includes/class-plugin-upgrader.php:89
/build/wp-admin/includes/class-plugin-upgrader.php:125
/build/wp-includes/rest-api/endpoints/class-wp-rest-plugins-controller.php:306
/build/wp-includes/rest-api/class-wp-rest-server.php:1027
/tests/phpunit/includes/spy-rest-server.php:57
/build/wp-includes/rest-api.php:447
/tests/phpunit/tests/rest-api/rest-plugins-controller.php:379

2) WP_REST_Plugins_Controller_Test::test_create_item_and_activate
Undefined property: WP_Ajax_Upgrader_Skin::$overwrite

/build/wp-admin/includes/class-plugin-upgrader.php:89
/build/wp-admin/includes/class-plugin-upgrader.php:125
/build/wp-includes/rest-api/endpoints/class-wp-rest-plugins-controller.php:306
/build/wp-includes/rest-api/class-wp-rest-server.php:1027
/tests/phpunit/includes/spy-rest-server.php:57
/build/wp-includes/rest-api.php:447
/tests/phpunit/tests/rest-api/rest-plugins-controller.php:404

3) WP_REST_Plugins_Controller_Test::test_create_item_and_activate_errors_if_no_permission_to_activate_plugin
Undefined property: WP_Ajax_Upgrader_Skin::$overwrite

/build/wp-admin/includes/class-plugin-upgrader.php:89
/build/wp-admin/includes/class-plugin-upgrader.php:125
/build/wp-includes/rest-api/endpoints/class-wp-rest-plugins-controller.php:306
/build/wp-includes/rest-api/class-wp-rest-server.php:1027
/tests/phpunit/includes/spy-rest-server.php:57
/build/wp-includes/rest-api.php:447
/tests/phpunit/tests/rest-api/rest-plugins-controller.php:431

#155 @poena
4 months ago

I got this error earlier but I believed it was because I had not applied the patch correctly. -Did you apply the changes made to src/wp-admin/update.php ?

#156 @estelaris
4 months ago

  • Keywords needs-screenshots removed

@earnjam
4 months ago

Fixes unit test errors and PHPCS errors from 9757.6.patch

#157 @earnjam
4 months ago

Seems to be working well for me in all the various ways I've tested it.

9757.7.patch Fixes the unit test errors mentioned by @whyisjake

@desrosj
4 months ago

#159 follow-up: @desrosj
4 months ago

This latest revision looks awesome! Really excited about this.

I created and linked a new PR to make reviewing the code easier and to confirm the tests are passing on Travis.

Because the `install()` method is used, I don't think that the current implementation would place the site in maintenance mode while the upgrade is applied. If a plugin or theme is active, this could cause some problems.

Placing this before and after the $upgrader->install() call (toggling the boolean) should solve that problem and prevent a user from stumbling on a broken site at during an update.

if ( is_plugin_active() ) {
    $upgrader->maintenance_mode( true );
}

Few more small thoughts, but I don't think any of these are a blocker for committing to include in beta:

Updated in the latest patch 9757.2.diff:

  • This action hook is a worded a bit weird: upgrader_overwrited_package. The patch replaces that with upgrader_overwrote_package (also toyed with upgrader_overwritten_package). Also, I clarified the docs to state the hook is only fired when the upgrader successfully overwrites the old package.
Last edited 4 months ago by desrosj (previous) (diff)

#160 @SergeyBiryukov
4 months ago

Noticed a typo in one string in 9757.2.diff:

  • The current theme has the follow error: "%s".

Should probably be "the following".

#161 @shaunandrews
3 months ago

Whoa, cool. I didn't expect half of my suggestions to become reality. Nice work.

I have two very minor requests:

  • In the main heading, can we make "Plugin" and "Theme" lowercase? I could see the argument for capitalizing them, but a quick glance shows me that we don't always do it.
  • Similar, the "Name" label in the table (like "Plugin Name" or "Theme Name") should also be lowercase.

#162 in reply to: ↑ 153 @mariovalney
3 months ago

Replying to azaozz:

But lets do this from another ticket (after this is committed). This is a bug in the "Install from a zip file" functionality, worth fixing even during beta imho.

Perfect! Here is: 50593 :)

#163 @audrasjb
3 months ago

Thanks for the great work here!

Reviewing the changes, I also noticed a small typo in upgrader_overwrote_package action DocBlock, located in wp-admin/includes/class-theme-upgrader.php:

@param string $package_type The package type (theme or theme).

Should be (plugin or theme). Probably a search and replace issue :)

#164 @azaozz
3 months ago

Replying to shaunandrews:

I have two very minor requests:

  • In the main heading, can we make "Plugin" and "Theme" lowercase? I could see the argument for capitalizing them, but a quick glance shows me that we don't always do it.
  • Similar, the "Name" label in the table (like "Plugin Name" or "Theme Name") should also be lowercase.

Sure, the first string is in the (pre-existing) code for installing from a zip. Can change it now though :)

Should be (plugin or theme). Probably a search and replace issue :)

Yep, thanks, fixing :)

#165 @azaozz
3 months ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from assigned to closed

In 48390:

Upgrade/install: Allow plugin and theme updates from a uploaded .zip file.

Props mariovalney, cyberhobo, imath, shaunandrews, mariovalney, earnjam, desrosj, dd32, folletto, swissspidy, melchoyce, pento, joshuawold, psykro, clorith, ahortin, galbaras, pingram3541, joyously, doobeedoo, karmatosed, poena, whyisjake, earnjam, sergeybiryukov, audrasjb, azaozz.

Fixes #9757.

#166 in reply to: ↑ 159 @azaozz
3 months ago

Replying to desrosj:

Because the `install()` method is used, I don't think that the current implementation would place the site in maintenance mode while the upgrade is applied. If a plugin or theme is active, this could cause some problems.

Placing this before and after the $upgrader->install() call (toggling the boolean) should solve that problem and prevent a user from stumbling on a broken site at during an update.

if ( is_plugin_active() ) {
    $upgrader->maintenance_mode( true );
}

Few more small thoughts, but I don't think any of these are a blocker for committing to include in beta:

Lets handle these in a new ticket.

#167 @afragen
3 months ago

Just a couple of comments on [48390], there are a couple of non-Yoda conditionals and I think the use of is_wp_version_compatible() and is_php_version_compatible() would make a couple version_compare() conditions simpler.

Last edited 3 months ago by SergeyBiryukov (previous) (diff)

#168 follow-up: @joyously
3 months ago

I tried this in the 5.5beta, and it shows me the HTML tags for the author link.

https://i.postimg.cc/7P35xt6y/Upload-Plugin-html-tags.jpg

#169 @joyously
3 months ago

Having the uploaded zip in the Media Library doesn't really make sense. It isn't mentioned to the user.
If I cancel or click away from the screen, the zip remains in the library. I can't use it from there, though.
If I successfully replace a plugin, neither one is saved in the Media Library. The plugin I've used in the past for this would save the one being replaced, which seems more useful.

#170 @SergeyBiryukov
3 months ago

In 48447:

I18N: Move the "WordPress Backups" support URL to its own translatable string.

Follow-up to [48390].

See #9757.

#171 in reply to: ↑ 168 @SergeyBiryukov
3 months ago

Replying to joyously:

I tried this in the 5.5beta, and it shows me the HTML tags for the author link.

Thanks, moved to #50633.

Having the uploaded zip in the Media Library doesn't really make sense. It isn't mentioned to the user.
If I cancel or click away from the screen, the zip remains in the library. I can't use it from there, though.

I think this was addressed in [48417] / #50612.

#172 follow-up: @SergeyBiryukov
3 months ago

In 48448:

Upgrade/Install: Use is_php_version_compatible() and is_wp_version_compatible() in plugin and theme requirement checks.

Follow-up to [48390].

Props afragen.
See #9757.

#173 follow-up: @azaozz
3 months ago

@SergeyBiryukov [48448] looks good but it changes the conditionals to be "less readable, more inconsistent", see https://make.wordpress.org/core/2020/03/20/updating-the-coding-standards-for-modern-php/#comment-38291 :) (that formatting is technically not wrong, the coding standards proposal in that post seems "stuck"..).

Having the uploaded zip in the Media Library doesn't really make sense.

As Sergey mentioned, this is the way installing/upgrading works. Was fixed in [48417], #50612.

Will have to add wp_strip_all_tags() for "Name" and "Author" though.

Last edited 3 months ago by azaozz (previous) (diff)

#174 @azaozz
3 months ago

In 48453:

Upgrade/Install: Use wp_strip_all_tags() for the fields in the compare table on the "Update theme/plugin from uploaded zip" screen. Some may contain HTML.

See #9757.

#175 in reply to: ↑ 172 ; follow-up: @afragen
3 months ago

Replying to SergeyBiryukov:

In 48448:

Upgrade/Install: Use is_php_version_compatible() and is_wp_version_compatible() in plugin and theme requirement checks.

Follow-up to [48390].

Props afragen.
See #9757.

Both of the compatibility functions include a test for the parameter as empty. It should be sufficient to simply use the compatibility function in the conditional.

If the parameter is empty the condition evaluates to true. It also improves the readability.

Last edited 3 months ago by afragen (previous) (diff)

#176 in reply to: ↑ 175 ; follow-up: @SergeyBiryukov
3 months ago

Replying to afragen:

Both of the compatibility functions include a test for the parameter as empty. It should be sufficient to simply use the compatibility function in the conditional.

They do, but omitting the check would cause a notice if the plugin data doesn't have the RequiresPHP key:

$new_plugin_data = array();
var_dump( is_php_version_compatible( $new_plugin_data['RequiresPHP'] ) );

Notice: Undefined index: RequiresPHP

It might be that the key always exists at that point, but that doesn't seem to be the case at a glance, so I think including the ! empty() check is a safer option.

#177 in reply to: ↑ 176 ; follow-up: @afragen
3 months ago

Replying to SergeyBiryukov:

Replying to afragen:

Both of the compatibility functions include a test for the parameter as empty. It should be sufficient to simply use the compatibility function in the conditional.

They do, but omitting the check would cause a notice if the plugin data doesn't have the RequiresPHP key:

$new_plugin_data = array();
var_dump( is_php_version_compatible( $new_plugin_data['RequiresPHP'] ) );

Notice: Undefined index: RequiresPHP

It might be that the key always exists at that point, but that doesn't seem to be the case at a glance, so I think including the ! empty() check is a safer option.

In that case, wouldn’t it make more sense to use isset()?

#178 in reply to: ↑ 173 ; follow-up: @SergeyBiryukov
3 months ago

Replying to azaozz:

[48448] looks good but it changes the conditionals to be "less readable, more inconsistent", see https://make.wordpress.org/core/2020/03/20/updating-the-coding-standards-for-modern-php/#comment-38291 :) (that formatting is technically not wrong, the coding standards proposal in that post seems "stuck"..).

Personally, I find @jrf's proposal more readable than the alternative, and tend to use the proposed formatting in my commits since the post was published. It's already used in quite a few places in core :)

#179 in reply to: ↑ 178 ; follow-up: @azaozz
3 months ago

Replying to SergeyBiryukov:

Personally, I find @jrf's proposal more readable than the alternative, and tend to use the proposed formatting in my commits since the post was published. It's already used in quite a few places in core :)

I don't have any (strong) preferences, but if it's used for if() then it should also be used for sprintf(), array(), etc. The inconsistency is no good imho :)

Last edited 3 months ago by azaozz (previous) (diff)

#180 in reply to: ↑ 177 ; follow-up: @azaozz
3 months ago

Replying to afragen:

In that case, wouldn’t it make more sense to use isset()?

Guess that's more of a "personal coding habits"? :)

isset() is good in some cases, but if you are expecting a value and doing something with it, it's better to "move on" straight away with ! empty() instead of running more code, calling functions, etc.

Last edited 3 months ago by azaozz (previous) (diff)

#181 in reply to: ↑ 180 @afragen
3 months ago

Replying to azaozz:

Replying to afragen:

In that case, wouldn’t it make more sense to use isset()?

Guess that's more of a "personal coding habits"? :)

isset() is good in some cases, but if you are expecting a value and doing something with it, it's better to "move on" straight away with ! empty() instead of running more code, calling functions, etc.

In this case the is_*_version_compatible() also does a check for empty().

#182 @SergeyBiryukov
3 months ago

In 48455:

Upgrade/Install: Simplify compatibility checks for uploaded plugins and themes for better readability.

Use $new_plugin_data and $new_theme_data as a shorthand for the corresponding $this->upgrader properties.

Follow-up to [48390], [48448].

Props afragen.
See #9757.

#183 in reply to: ↑ 179 @SergeyBiryukov
3 months ago

Replying to azaozz:

Replying to SergeyBiryukov:

Personally, I find @jrf's proposal more readable than the alternative, and tend to use the proposed formatting in my commits since the post was published. It's already used in quite a few places in core :)

I don't have any (strong) preferences, but if it's used for if() then it should also be used for sprintf(), array(), etc. The inconsistency is no good imho :)

To clarify a bit, I don't have a strong preference on if being on its own line, that just seemed more consistent with what's already in core. I do, however, have a strong preference on the boolean/logical operators between the statements being placed at the start of the line, rather than at the end. That's what I find more readable :)

#184 follow-up: @mariovalney
3 months ago

From the tavern maybe we can replace the string:

If you have a plugin in a .zip format, you may install it by uploading it here.

by

If you have a plugin in a .zip format, you may upload it here.

If you have a plugin in a .zip format, you may install or update it by uploading it here.

On install_plugins_upload (plugin-install.php) and install_themes_upload (theme-install.php).

#185 in reply to: ↑ 184 @azaozz
3 months ago

Replying to mariovalney:

Yeah, makes sense. Thinking the second string is a bit clearer? Fixing.

#186 @azaozz
3 months ago

In 48509:

Upgrade/install: Fix/clarify the "Upload in a zip format" string.

Props greenshady, mariovalney.
See #9757.

#188 @desrosj
3 months ago

In 48676:

Upgrade/Install: Correct spelling of “overwrite” in new hooks and array indexes.

See #9757.

#189 @desrosj
3 months ago

In 48677:

Upgrade/Install: Clarify the descriptions for install_(plugin|theme)_overwrite_actions.

See #9757.

#190 @afragen
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There seems to be a difference between https://core.trac.wordpress.org/changeset/48390 and what is currently in core, https://core.trac.wordpress.org/changeset/48390#file4

In install() what is currently in core in $this-run( ... 'clear_destination' => $args['overwrite_package']..).

It should be $parsed_args['overwrite_package'] as $args['overwrite_package'] doesn't exist.

I have no idea how this happened I can't trace the commit.

This ticket was mentioned in Slack in #core by afragen. View the logs.


3 months ago

#192 @sabernhardt
3 months ago

  • Keywords commit removed

@afragen I think you're noticing a difference between the plugin and theme upgrader files.

class-plugin-upgrader.php:
'clear_destination' => $parsed_args['overwrite_package'],

class-theme-upgrader.php:
'clear_destination' => $args['overwrite_package'],

For testing, I'll upload a patch to edit the theme upgrader.

@sabernhardt
3 months ago

changing $args to $parsed_args in class-theme-upgrader.php

#194 @SergeyBiryukov
3 months ago

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

In 48685:

Upgrade/Install: Pass correct argument to clear_destination in Theme_Upgrader::install().

Follow-up to [48390].

Props afragen, sabernhardt.
Fixes #9757.

#195 @SergeyBiryukov
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting [48685] to the 5.5 branch after a second committer's review.

#196 @desrosj
3 months ago

  • Keywords commit dev-reviewed added; dev-feedback removed

Looks good @SergeyBiryukov!

#197 @SergeyBiryukov
3 months ago

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

In 48687:

Upgrade/Install: Pass correct argument to clear_destination in Theme_Upgrader::install().

Follow-up to [48390].

Props afragen, sabernhardt.
Reviewed by desrosj, SergeyBiryukov.
Merges [48685] to the 5.5 branch.
Fixes #9757.

Note: See TracTickets for help on using tickets.