Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#37531 closed defect (bug) (fixed)

Plugin installation failure not reflected in the UI

Reported by: aronms's profile AronMS Owned by: ocean90's profile 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)

37531.diff (1.9 KB) - added by flixos90 8 years ago.
37531.2.diff (3.0 KB) - added by flixos90 8 years ago.
37531.3.diff (3.3 KB) - added by flixos90 8 years ago.
37531.4.diff (545 bytes) - added by obenland 8 years ago.
37531.patch (5.7 KB) - added by ocean90 8 years ago.
37531.2.patch (7.6 KB) - added by ocean90 8 years ago.
37531.3.patch (12.2 KB) - added by ocean90 8 years ago.
37531.4.patch (11.1 KB) - added by ocean90 8 years ago.
37531.5.patch (13.0 KB) - added by ocean90 8 years ago.
37531.5.diff (13.4 KB) - added by obenland 8 years ago.
37531.6.diff (8.7 KB) - added by ocean90 8 years ago.
refresh of .5
37531.7.diff (11.1 KB) - added by ocean90 8 years ago.
37531.8.diff (11.1 KB) - added by DrewAPicture 8 years ago.

Download all attachments as: .zip

Change History (47)

#1 @swissspidy
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 @SergeyBiryukov
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: @ocean90
8 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.6

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

#4 in reply to: ↑ 3 @ocean90
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

#6 @ocean90
8 years ago

  • Priority changed from normal to high
  • Severity changed from normal to major

@flixos90
8 years ago

@flixos90
8 years ago

#7 @flixos90
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().

Last edited 8 years ago by flixos90 (previous) (diff)

@flixos90
8 years ago

#8 @flixos90
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().

Last edited 8 years ago by flixos90 (previous) (diff)

#9 @flixos90
8 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#10 follow-up: @ocean90
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 @flixos90
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 @swissspidy
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 @obenland
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.

@obenland
8 years ago

@ocean90
8 years ago

@ocean90
8 years ago

@ocean90
8 years ago

@ocean90
8 years ago

#15 @ocean90
8 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

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.
Last edited 8 years ago by ocean90 (previous) (diff)

This ticket was mentioned in Slack in #feature-shinyupdates by ocean90. View the logs.


8 years ago

@ocean90
8 years ago

#17 @ocean90
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: @obenland
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 @ocean90
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

@obenland
8 years ago

#22 @obenland
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 @ocean90
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.

#24 @ocean90
8 years ago

  • Owner obenland deleted

@ocean90
8 years ago

refresh of .5

#25 @ocean90
8 years ago

  • Owner set to pento

#26 @pento
8 years ago

  • Status changed from reviewing to accepted

#27 @pento
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 @ocean90
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.

@ocean90
8 years ago

#29 @ocean90
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.

#30 @ocean90
8 years ago

  • Keywords commit added; needs-refresh removed

@DrewAPicture
8 years ago

#31 @DrewAPicture
8 years ago

  • Keywords dev-reviewed added

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

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


8 years ago

#33 @ocean90
8 years ago

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

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.

#34 @ocean90
8 years ago

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.

Note: See TracTickets for help on using tickets.