Plugin installation failure not reflected in the UI

[AronMS]

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.

[ocean90]

Confirmed. Ajax response is success: false and in Chrome it freezes the screen, requires a hard refresh to continue.

[ocean90]

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."
  }
}

[flixos90]

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().

[flixos90]

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().

[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

[flixos90]

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.

[swissspidy]

@flixos90 The wp.updates.shouldRequestFilesystemCredentials check definitely makes sense, but we should really try to fix the root cause, not paper over it :-)

[obenland]

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.

[ocean90]

37531.5.patch​:

• Includes 37531.3.diff​ without class-wp-upgrader.php change
• Includes 37531.4.diff​
• Introduces WP_Ajax_Upgrader_Skin which extends Automatic_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().
• Note: The current call_user_func_array() works in all supported PHP versions, see ​https://3v4l.org/15dEY.

[ocean90]

@dd32 Also interested in your feedback about WP_Ajax_Upgrader_Skin.

[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?

[ocean90]

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().

[obenland]

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.

[ocean90]

37531.5.diff​ looks good to me. There is some repeating code but that's mostly caused by the underlying upgrade functions.

[ocean90]

refresh of .5

[pento]

37531.6.diff​ looks good to me.

[ocean90]

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.

[ocean90]

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.

[DrewAPicture]

37531.7.diff​ Looks good. Slightly tweaked the docs only in 37531.8.diff​.

[ocean90]

In 38199:

Upgrade/Install: Make some install/update failures more verbose.

An upgrader class is used in conjunction with an upgrader skin class. A skin class handles the logging for an upgrade and informs a user about the progress and failures.
The current Ajax install/update handlers are using the Automatic_Upgrader_Skin class because during an Ajax request no output is intended. The difference between Ajax updates and automatic updates is that you will see the full log (usually by email) while Ajax updates focus only on success or failure. For that Automatic_Upgrader_Skin has one disadvantage: It doesn't provide a way to retrieve failure messages which were passed through WP_Upgrader_Skin::error() by the upgrader.
To solve this issue a new skin WP_Ajax_Upgrader_Skin has been introduced. The skin extends Automatic_Upgrader_Skin and overrides the error() and feedback() methods to intercept all errors, which can be a WP_Error object or a string.

This updates all four Ajax handler for installing/updating themes/plugins to use the new skin. They now also check the skin for any intercepted errors and pass them on to the user.

Props flixos90, obenland, ocean90.
Props DrewAPicture, pento for review.
Fixes #37531.

[ocean90]

In 38200:

Upgrade/Install: Make some install/update failures more verbose.

An upgrader class is used in conjunction with an upgrader skin class. A skin class handles the logging for an upgrade and informs a user about the progress and failures.
The current Ajax install/update handlers are using the Automatic_Upgrader_Skin class because during an Ajax request no output is intended. The difference between Ajax updates and automatic updates is that you will see the full log (usually by email) while Ajax updates focus only on success or failure. For that Automatic_Upgrader_Skin has one disadvantage: It doesn't provide a way to retrieve failure messages which were passed through WP_Upgrader_Skin::error() by the upgrader.
To solve this issue a new skin WP_Ajax_Upgrader_Skin has been introduced. The skin extends Automatic_Upgrader_Skin and overrides the error() and feedback() methods to intercept all errors, which can be a WP_Error object or a string.

This updates all four Ajax handler for installing/updating themes/plugins to use the new skin. They now also check the skin for any intercepted errors and pass them on to the user.

Merge of [38199] to the 4.6 branch.

Props flixos90, obenland, ocean90.
Props DrewAPicture, pento for review.
See #37531.