#37531 closed defect (bug) (fixed)
Plugin installation failure not reflected in the UI
Reported by: | AronMS | Owned by: | ocean90 |
---|---|---|---|
Milestone: | 4.6 | Priority: | high |
Severity: | major | Version: | 4.6 |
Component: | Plugins | Keywords: | has-patch commit dev-reviewed |
Focuses: | ui, administration | Cc: |
Description
I am testing WordPress 4.6 RC-1. I have a directory in my plugins folder containing an old version of my plugin, the main php file has been renamed e.g. drafty-in-here/drafty-in-here.php.lock
.
When I use the WordPress plugin installer to install a new copy from worpress.org on the UI it just keeps spinning. However when I look in the JavaScript console I see the folowing output:
JQMIGRATE: Migrate is installed, version 1.4.1 Downloading install package from https://downloads.wordpress.org/plugin/drafty-in-here.1.2.0.zip… Unpacking the package… Installing the plugin… Destination folder already exists. C:/MAMP/htdocs/projects/wordpress-test-site-beta/wp-content/plugins/drafty-in-here/ Plugin install failed.
WordPress should show that the plugin install failed on the UI otherwise the user has no idea what's going on.
This behaviour is slightly better than WP 4.5 where installation would fail, but it would say that it was successful.
Attachments (13)
Change History (47)
#1
@
8 years ago
- Summary changed from Plugin Instilation Failuer Not Refelected in UI to Plugin Instilation Failure Not Refelected in UI
- Type changed from enhancement to defect (bug)
- Version set to trunk
#2
@
8 years ago
- Summary changed from Plugin Instilation Failure Not Refelected in UI to Plugin installation failure not reflected in the UI
#3
follow-up:
↓ 4
@
8 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to 4.6
#4
in reply to:
↑ 3
@
8 years ago
Replying to ocean90:
and in Chrome it freezes the screen, requires a hard refresh to continue.
No freeze, it's the .modal-open
class which gets added to the body element because the response is for missing credentials…
{ "success": false, "data": { "install": "plugin", "slug": "two-factor", "pluginName": "Two-Factor", "debug": [ "Downloading install package from https:\/\/downloads.wordpress.org\/plugin\/two-factor.zip…", "Unpacking the package…", "Installing the plugin…", "Destination folder already exists. \/srv\/www\/wp-shared\/develop-plugins\/two-factor\/", "Plugin install failed." ], "errorCode": "unable_to_connect_to_filesystem", "errorMessage": "Unable to connect to the filesystem. Please confirm your credentials." } }
This ticket was mentioned in Slack in #feature-shinyupdates by ocean90. View the logs.
8 years ago
#7
@
8 years ago
37531.diff fixes the modal-open
class to be used. If the filesystem credentials modal isn't printed out, do not attempt to show it. (sorry about the trailing spaces, I just noticed my code editor messed them up)
However, another problem here is that the directory already existing is actually not a filesystem credential error IMO. Even on a setup which allows direct filesystem access, this problem may appear - requesting filesystem credentials wouldn't fix it. Therefore, in 37531.2.diff I modified the upgrader class to store this specific error in the $result
property so that it is used by the AJAX function wp_ajax_install_plugin()
.
#8
@
8 years ago
This issue also affects the Theme Install screen. The problem is less likely to happen because WordPress assumes a theme is installed when seeing the directory of that name. To recreate the issue (without the patch of course): Go to the Theme Install screen, then create a directory that matches the slug of a theme, then back on the WordPress screen try to install - it will produce the exact same result. In 37531.3.diff I adjusted wp_ajax_install_theme()
to include the error code as well, similar as in wp_ajax_install_plugin()
.
#10
follow-up:
↓ 11
@
8 years ago
- Keywords needs-patch added; has-patch needs-testing removed
@flixos90 Thanks for the patch, but this requires some more work. I documented the current issues on Slack: https://wordpress.slack.com/archives/feature-shinyupdates/p1469830481000040
#11
in reply to:
↑ 10
@
8 years ago
Replying to ocean90:
@flixos90 Thanks for the patch, but this requires some more work. I documented the current issues on Slack: https://wordpress.slack.com/archives/feature-shinyupdates/p1469830481000040
Ah okay I see. Correct me if I'm wrong, but it looks to me that at least the first patch makes sense as is - as it prevents WordPress to try to show the filesystem credentials modal although it isn't printed out. This won't fix the more severe general problem, but will fix the install screen from freezing.
This ticket was mentioned in Slack in #feature-shinyupdates by ocean90. View the logs.
8 years ago
#13
@
8 years ago
@flixos90 The wp.updates.shouldRequestFilesystemCredentials
check definitely makes sense, but we should really try to fix the root cause, not paper over it :-)
#14
@
8 years ago
We could check if $updater->skin->result
is a WP_Error
and set $result
to that, but that wouldn't fix the underlying problem of WP_Upgrader::install_package()
not setting its $results
property correctly in some instances where errors occur. Like when the destination directory already exists.
#15
@
8 years ago
- Keywords has-patch needs-testing added; needs-patch removed
- Includes 37531.3.diff without
class-wp-upgrader.php
change - Includes 37531.4.diff
- Introduces
WP_Ajax_Upgrader_Skin
which extendsAutomatic_Upgrader_Skin
. This class stores all error messages/WP_Error objects in an$error
property. - All Ajax handlers are updated to use the new skin.
- Each Ajax handler now checks for the 3 error sources: 1) Result of the install/update method, 2)
$skin->result
and 3)$skin->get_errors()
.
This ticket was mentioned in Slack in #feature-shinyupdates by ocean90. View the logs.
8 years ago
#17
@
8 years ago
- Owner set to obenland
- Status changed from new to reviewing
@dd32 Also interested in your feedback about WP_Ajax_Upgrader_Skin
.
#18
follow-up:
↓ 19
@
8 years ago
Patch looks good. I noticed that when it checks for $skin->get_errors()->get_error_code()
in the ajax callbacks, it doesn't add the error code to the response, is that by design?
#19
in reply to:
↑ 18
@
8 years ago
Replying to obenland:
Patch looks good. I noticed that when it checks for
$skin->get_errors()->get_error_code()
in the ajax callbacks, it doesn't add the error code to the response, is that by design?
In this branch we may have multiple errors and get_error_code()
would only return the error code for the first error message. I'm fine with adding it there though because during tests I wasn't really able to trigger more than one error in the same process.
Patch needs an update because I copied the wrong DocBlock to __construct()
.
This ticket was mentioned in Slack in #core by ocean90. View the logs.
8 years ago
This ticket was mentioned in Slack in #feature-shinyupdates by obenland. View the logs.
8 years ago
#22
@
8 years ago
Updated patch with a more generic comment for the constructor and a logic change in ajaxAlways
to remove the lock and continue processing the queue when an error occurs.
#23
@
8 years ago
- Keywords commit added
37531.5.diff looks good to me. There is some repeating code but that's mostly caused by the underlying upgrade functions.
#27
@
8 years ago
- Keywords dev-reviewed added; needs-testing removed
- Owner changed from pento to ocean90
- Status changed from accepted to assigned
37531.6.diff looks good to me.
#28
@
8 years ago
- Keywords needs-refresh added; commit dev-reviewed removed
Patch needs a small change because errors for wrong FTP credentials are currently displayed in the list but they should be part of the modal.
#29
@
8 years ago
37531.7.diff removes $status['errorCode'] = $wp_filesystem->errors->get_error_code();
from all four Ajax handlers. Filesystem errors related to auth/connect ('no_ftp_ext', 'empty_hostname', 'empty_username', 'empty_password', 'connect', 'auth', 'no_ssh2_ext', 'ssh2_php_requirement') should always be visible in the modal which gets only opened when the error code is unable_to_connect_to_filesystem
.
This was added in 37531.patch but the change to wp-admin/js/updates.js
is the better fix for this.
#31
@
8 years ago
- Keywords dev-reviewed added
37531.7.diff Looks good. Slightly tweaked the docs only in 37531.8.diff.
Confirmed. Ajax response is
success: false
and in Chrome it freezes the screen, requires a hard refresh to continue.