#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 )
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)
Change History (223)
#1
@
16 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
#2
@
16 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
@
16 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)
#6
@
16 years ago
- Keywords needs-patch added; 2.8 removed
thoughts about setting clear_destination to true in this case?
#7
@
16 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
@
16 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
@
16 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
@
16 years ago
I mostly see it as a means to easily update a theme or a plugin that is *not* in the repository. :-)
#11
@
16 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
@
15 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.
#15
@
15 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
@
15 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
@
15 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):
- Unzip archive.
- Scan all extracted files for plugin headers.
- Match any found plugins with the currently installed plugins.
- Show a confirmation screen to the user about which plugins would be upgraded.
- Do normal upgrade procedure for matched plugins.
#18
follow-up:
↓ 19
@
15 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
@
15 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.
#21
@
14 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
This ticket was mentioned in Slack in #core by clorith. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by clorith. View the logs.
10 years ago
#28
@
9 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 =)
#29
follow-up:
↓ 68
@
8 years ago
Perhaps we can merge parts of this plugin into core: https://wordpress.org/plugins/easy-theme-and-plugin-upgrades/
#31
@
8 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
@
8 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.
#38
@
8 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
@
8 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.
#41
@
8 years ago
@rockwell15 PLEASE can we schedule this for the next point version. (Plugin and theme updates from .zip upload)
#42
@
8 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
@
7 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.
7 years ago
#46
@
7 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.
#47
@
7 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?
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
7 years ago
#49
@
7 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
@
7 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
@
7 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.
7 years ago
#53
@
7 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.
This ticket was mentioned in Slack in #design by folletto. View the logs.
7 years ago
#56
@
7 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
@
7 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
@
7 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
@
7 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
@
7 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.
7 years ago
#62
@
7 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.
7 years ago
#65
@
7 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
- 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.
- 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
- Does the upgrade process need to be more complicated than
- delete old plugin files
- extract new plugins files
#66
@
7 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
@
7 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:
↓ 75
@
7 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
@
7 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:
↓ 71
@
7 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
@
7 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.
7 years ago
#73
@
7 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.
So on adding a new plugin and clicking the Install Now button
- Make a WP Rest API call to check if a plugin with either a full or partial slug match exists
- Display the 'This plugin you are attempting to install seems to exist' message to the user
- Display the matching plugin details (name, description and version number)
- Move the Install Now button under the 'confirmation' message and change it to 'Accept and install'
- On clicking the install button the usual upload and install process proceeds
Things to consider
- 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
- 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.
7 years ago
#75
in reply to:
↑ 68
@
7 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
@
7 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.
#77
@
7 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
@
7 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'
- Upload plugin zip asynchronously to a temporary location in the uploads directory, showing am 'upload' progress meter
- 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
- Display the 'This plugin you are attempting to install seems to exist' message to the user
- Display the matching plugin details (name, description and version number)
- Move the Install Now button under the 'confirmation' message and change it to 'Accept and install'
- 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.
7 years ago
#80
@
7 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.
7 years ago
#82
@
7 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.
#83
@
7 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.
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 :
"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
@
7 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
@
7 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
@
7 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?
#87
@
7 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
:
#88
@
7 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
@
7 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
@
7 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.
#93
@
5 years 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 years ago
#94
<!--
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:
- FAQs for New Contributors: https://make.wordpress.org/core/handbook/tutorials/faq-for-new-contributors/
- Contributing with Code Guide: https://make.wordpress.org/core/handbook/contribute/
- WordPress Coding Standards: https://make.wordpress.org/core/handbook/best-practices/coding-standards/
- Inline Documentation Standards: https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/
- Browser Support Policies: https://make.wordpress.org/core/handbook/best-practices/browser-support/
- Proper spelling and grammar related best practices: https://make.wordpress.org/core/handbook/best-practices/spelling/
-->
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
@
5 years 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:
Btw: too cool the new trac feature.
#96
@
5 years 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:
- If the theme/plugin doesn't exist, just do as it does today
- 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
@
5 years 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
@
5 years ago
If you've already an idea just post the screenshots of the new flow and we can move from there :)
#99
follow-up:
↓ 100
@
5 years ago
It's just a "console editing" version. So I'm not sure about implementation, but I guess it's possible.
#100
in reply to:
↑ 99
;
follow-up:
↓ 103
@
5 years ago
Replying to mariovalney:
It's just a "console editing" version. So I'm not sure about implementation, but I guess it's possible.
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
@
5 years 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
@
5 years ago
+1 on that, makes sense that the order of presentation matches and the instruction is very clear.
#103
in reply to:
↑ 100
@
5 years 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/
#104
follow-up:
↓ 105
@
5 years 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
@
5 years 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?
#106
follow-up:
↓ 116
@
5 years 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:
Note: themes are very like plugins in this case, so let's do the theme part after eveything is considered about plugins.
#108
follow-up:
↓ 112
@
5 years 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.
#110
@
5 years 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
@
5 years 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
@
5 years 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:
↓ 114
@
5 years 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
@
5 years 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.
#115
@
5 years 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:
↓ 117
↓ 118
@
5 years 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
@
5 years 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
@
5 years 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:
Except for Version, like upgrade is the secure case:
But we alert about downgrade:
And a text about reuploading (same value for the 5 main fields):
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:
#120
@
5 years ago
I'm back for themes \o/
And the same check for "reuploading package" like plugin:
But there's a note about errors on current theme:
And a check for parent theme for uploaded:
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
@
5 years 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:
↓ 124
@
5 years 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
@
5 years 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.
#126
@
5 years 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
@
5 years 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
@
5 years ago
Good question, albeit unrelated. Closest I can find is tweaking https://core.trac.wordpress.org/prefs/notification
#129
follow-up:
↓ 131
@
5 years 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.
mariovalney commented on PR #267:
5 years ago
#130
TODO:
- [ ] Replace "Compare and retry" with the "same plugin/theme" message.
#131
in reply to:
↑ 129
@
5 years 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
@
5 years 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.
#133
follow-ups:
↓ 135
↓ 136
@
5 years 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
@
4 years 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
#135
in reply to:
↑ 133
@
4 years 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):
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
@
4 years 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.
#137
@
4 years 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?
#138
@
4 years ago
- Keywords 2nd-opinion removed
- Milestone changed from Future Release to 5.5
In 9757.3.patch:
- Based on https://github.com/WordPress/wordpress-develop/pull/267.diff,
- Tweaked the styling a bit, replaced the red backgrounds with red text (looks more like the rest of wp-admin).
- Updated some strings.
- Fixed few PHPCS errors.
- Moved the "backup now!" warning a bit up and added standard styling.
Thinking this is about ready. Please test! :)
#139
@
4 years 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.
@
4 years 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?
#140
@
4 years 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.
#141
follow-up:
↓ 142
@
4 years 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
@
4 years 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.
#143
@
4 years 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 years ago
#145
@
4 years 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.
- 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.
- Can we maybe replace the "Plugin/Theme installation failed." line with something a little more descriptive, like "This plugin/theme is already installed."
- 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.
- I'm not sure about the use of Notice's for backing up and other issues. My mockups just use normal text.
- I know this flow currently uses links for the actions, but maybe we could/should switch to buttons?
This ticket was mentioned in Slack in #design by estelaris. View the logs.
4 years ago
#147
@
4 years 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
@
4 years 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
@
4 years 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 is more important.
#150
@
4 years 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
@
4 years 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.
#153
follow-up:
↓ 162
@
4 years 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.
#154
@
4 years 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
@
4 years 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 ?
#157
@
4 years 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
This ticket was mentioned in PR #395 on WordPress/wordpress-develop by desrosj.
4 years ago
#158
Trac ticket: https://core.trac.wordpress.org/ticket/9757
#159
follow-up:
↓ 166
@
4 years 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:
- When the PHP or WordPress requirements are not met, can we link to the HelpHub so that the user has some direction: Maybe this page: https://wordpress.org/support/update-php/?
- Would it make more sense to create an array of data and pass that to this filter before creating the HTML output? If not, I think it makes sense to also pass the `$rows` variable to the hook (and maybe this should also be filterable?).
Updated in the latest patch 9757.2.diff:
- This action hook is a worded a bit weird:
upgrader_overwrited_package
. The patch replaces that withupgrader_overwrote_package
(also toyed withupgrader_overwritten_package
). Also, I clarified the docs to state the hook is only fired when the upgrader successfully overwrites the old package.
#160
@
4 years 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
@
4 years 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.
#163
@
4 years 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
@
4 years 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
@
4 years ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from assigned to closed
In 48390:
#166
in reply to:
↑ 159
@
4 years 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:
- When the PHP or WordPress requirements are not met, can we link to the HelpHub so that the user has some direction: Maybe this page: https://wordpress.org/support/update-php/?
- Would it make more sense to create an array of data and pass that to this filter before creating the HTML output? If not, I think it makes sense to also pass the `$rows` variable to the hook (and maybe this should also be filterable?).
Lets handle these in a new ticket.
#167
@
4 years 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.
#169
@
4 years 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.
#171
in reply to:
↑ 168
@
4 years 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.
#173
follow-up:
↓ 178
@
4 years 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.
#175
in reply to:
↑ 172
;
follow-up:
↓ 176
@
4 years ago
Replying to SergeyBiryukov:
In 48448:
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.
#176
in reply to:
↑ 175
;
follow-up:
↓ 177
@
4 years 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:
↓ 180
@
4 years 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: RequiresPHPIt 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:
↓ 179
@
4 years 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:
↓ 183
@
4 years 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 :)
#180
in reply to:
↑ 177
;
follow-up:
↓ 181
@
4 years 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.
#181
in reply to:
↑ 180
@
4 years 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()
.
#183
in reply to:
↑ 179
@
4 years 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 forsprintf()
,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:
↓ 185
@
4 years 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
@
4 years ago
Replying to mariovalney:
Yeah, makes sense. Thinking the second string is a bit clearer? Fixing.
4 years ago
#187
This was merged into core in https://core.trac.wordpress.org/changeset/48390.
#190
@
4 years 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.
4 years ago
#192
@
4 years 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.
#195
@
4 years 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.
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.