WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 7 days ago

#42184 closed defect (bug) (fixed)

Customizer theme install getting stuck on downloading

Reported by: helen Owned by:
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Customize Keywords: has-patch
Focuses: Cc:

Description

I've been getting into situations with the new theme browser/installer in the customizer where it gets stuck on the downloading spinner or the install button doesn't actually work at all - you can click it but nothing happens, and no errors in the console.

I *think* this has to do with the fact that I'm also messing around with forcing the FTP credential prompt for other purposes and toggling that on and off in PHP without reloading the page. This itself may (probably) be an edge case that doesn't need addressing, but I think the error handling could be better and may come up in other situations.

Steps to reproduce:

  1. Browse .org themes in the customizer.
  2. With forced FTP on (will add code for this in a followup comment), attempt to "install and preview" from the grid view.
  3. Observe that failure notice appears over the screenshot and button changes to Installation failed but remains primary and active
  4. Disable forcing FTP in PHP elsewhere; do not reload page.
  5. Button on the attempted theme is still clickable but does nothing, all other buttons will take you to the downloading spinner which never completes.

Attachments (6)

42184.0.diff (663 bytes) - added by westonruter 2 months ago.
credential-modal.png (303.0 KB) - added by westonruter 2 months ago.
after-credential-modal-cancel.png (224.3 KB) - added by westonruter 2 months ago.
42184.1.diff (1.5 KB) - added by celloexpressions 8 weeks ago.
Add handling for credential cancelling in the customize context.
42184.2.diff (2.0 KB) - added by celloexpressions 8 weeks ago.
Fix theme download error handling.
42184.disable.diff (6.3 KB) - added by westonruter 8 weeks ago.
Disable theme install and delete when FTP credentials required

Download all attachments as: .zip

Change History (34)

#1 @helen
2 months ago

@ocean90's plugin code for forcing FTP:

<?php
/**
 * Plugin Name: Force FTP
 * Description: Used to test FTP credentials screen.
 */

add_filter( 'filesystem_method', function() {
        return 'ftpext';
} );

add_filter( 'fs_ftp_connection_types', function() {
        return [
                'ftp'  => __( 'FTP' ),
                'ftps' => __( 'FTPS (SSL)' ),
                'ssh'  => __( 'SSH2' ),
        ];
} );
Last edited 2 months ago by westonruter (previous) (diff)

#2 @westonruter
2 months ago

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

I was able to reproduce this a couple times when I was hacking around trying to force an unable_to_connect_to_filesystem error when calling wp_ajax_install_theme(). I think it has something to do with wp.updates.maybeRequestFilesystemCredentials(), and the wp.updates.ajaxLocked and wp.updates.shouldRequestFilesystemCredentials flags. I think it assumes a prompt is being shown to enter credentials, but I've never seen this appear. Maybe it is actually being shown but it is hidden behind the Customizer UI thanks to z-index issues?

#3 @celloexpressions
2 months ago

My expectation has been that the calls to wp.updates in the themes panel JS (and also maybe in the section JS) would show the filesystem credentials and handle related errors (with the adjustments to updates.js in #37661). It would be most helpful if the people who worked on shiny updates (I think @obenland or @swissspidy, possibly others?) could take a look at how it's being applied to the customizer. My guess is that the required error handling and credential overlays are mostly implemented but require minor tweaking to be fully functional in this context.

#4 @obenland
2 months ago

This seems to be an issue of a missing wp_print_request_filesystem_credentials_modal(); call in wp-admin/customize.php to capture FTP credentials. Once that was added, the theme browser should also listen for when the modal closes prematurely and return to the selection instead of the loading spinner.

@westonruter
2 months ago

#5 @westonruter
2 months ago

  • Keywords has-patch added; needs-patch removed

@obenland thanks for that! That makes sense. @helen Would you test 42184.0.diff to see if it fixes the issue you reported?

Note that you'll need to have that plugin active to force FTP mode before loading the Customizer, since the template will skip rendering if it detects it has direct write access to the filesystem. You wouldn't be able test with dynamically enabling/disabling FTP mode while the Customizer is open.

#6 @westonruter
2 months ago

  • Owner set to helen
  • Status changed from new to reviewing

#7 @westonruter
2 months ago

I just noticed #42205 (FTP credentials dialog gets hidden behind things) which seems closely related.

#8 @westonruter
2 months ago

  • Keywords needs-patch added; has-patch removed
  • Owner changed from helen to celloexpressions

The problem is going deeper than just adding the credential modal template in 42184.0.diff. When I add it, I do get the modal as expected in credential-modal.png. But when I cancel out of that modal, I then get some weird behavior as can be seen in after-credential-modal-cancel.png.

I think it may have to do with this credential-modal-cancel event handler running in the unexpected context of the Customizer: https://github.com/WordPress/wordpress-develop/blob/850db3c6a05af31efdd4ec982536cae4c496ae61/src/wp-admin/js/updates.js#L1780-L1825

In addition the buttons both getting the “Install” label, then when I try clicking on them they do nothing.

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


2 months ago

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


2 months ago

@celloexpressions
8 weeks ago

Add handling for credential cancelling in the customize context.

#11 @celloexpressions
8 weeks ago

  • Keywords has-patch added; needs-patch removed

42184.1.diff should take care of most of the issues here, by correctly resetting labels and closing the downloading notification when the credentials modal is cancelled.

There are a couple of additional issues that I could not track down. The button you pressed breaks if you cancel out of the modal, but all of the other buttons still work. The labels can also be incorrectly assigned when the theme details modal is open (but not in the grid view). The aria-label is not reset properly, which may be because any aria-labels are handled differently in the admin context.

The best solution is probably to rethink the integration with updates.js entirely, not changing buttons or showing visible loading indicators from there in the customizer because the full-page notification is shown. Much of what was added in updates.js undoes what happens there for wp-admin. It's a bit late to do that for 4.9, but something to consider for the future. For now, 42184.1.diff makes most of this functional, and if anyone else can dig into the lingering issues further, that would be helpful.

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


8 weeks ago

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


8 weeks ago

#14 @helen
8 weeks ago

I'm not set up to test fully (actually entering credentials and having them work) but 42184.1.diff does at least allow the modal to appear the first time you press the install button for a given theme. As noted, the button ceases to work after that.

Also, just want to be clear that I doubt this is a release blocker, though it is pretty bad to have something that can make the user feel like they broke it.

#15 follow-up: @westonruter
8 weeks ago

  • Keywords needs-patch added; has-patch removed

After dismissing the file credentials modal, I'm still seeing the same thing as after-credential-modal-cancel.png, where both buttons say “Install”.

@celloexpressions
8 weeks ago

Fix theme download error handling.

#16 @celloexpressions
8 weeks ago

#42314 was marked as a duplicate.

#17 in reply to: ↑ 15 @celloexpressions
8 weeks ago

  • Keywords has-patch added; needs-patch removed

Replying to westonruter:

After dismissing the file credentials modal, I'm still seeing the same thing as after-credential-modal-cancel.png, where both buttons say “Install”.

Known issue noted in 11. The problem is that updates.js doesn't address multiple buttons with the same action; I don't see a minimally-hacky way to fix this without more broadly refactoring the integration. However, this only happens when a model is open, so it will be rarely encountered.

42184.2.diff fixes the failed theme download error handling following the new notifications API, which is something that users will run into much more frequently (for example, an intermittent internet connection or downtime on .org). The buttons still stop working for that, but they do show a notification failed error. It seems like it would be worth more broadly evaluating the error handling here, likely in a future (potentially minor) release so that there's time to consider all error cases to handle and refactor things at a higher level.

Summary of issues fixed in 42184.2.diff:

  • Theme download error handling restored following the use of a notifications API for the downloading overlay.
  • Introduce preliminary customize-specific logic for the modal credential cancel handler.
  • Ensure that scripts for the credentials modal are correctly enqueued.

Are there any other issues here that seem fixable for 4.9? Would be good to get the progress into beta 4, at least, for broader testing.

#18 @afercia
8 weeks ago

With 42184.2.diff applied I still see something not working correctly, to reproduce:

  • force FTP
  • customizer > change theme > .org themes
  • click on any theme "Install & Preview" button
  • the FTP credentials modal opens
  • close the modal
  • click on the same theme previously clicked "Install & Preview" button
  • nothing happens

#19 @westonruter
8 weeks ago

Since SFTP is broken I think what we should do is disable installation entirely when SFTP is required. We can disable the install buttons and a notification to the Themes panel in the same way we do when the user has drafted or scheduled a changeset.

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


8 weeks ago

@westonruter
8 weeks ago

Disable theme install and delete when FTP credentials required

#21 @westonruter
8 weeks ago

In 41997:

Customize: Prevent theme installation and deletion in Customizer while SFTP credentials need to be requested.

This is a temporary measure while we wait for credentials to be able to be supplied in the Customizer.

Amends [41788].
See #42184, #37661, #42126.

#22 @westonruter
8 weeks ago

  • Milestone changed from 4.9 to 4.9.1

Punting now that we temporarily disabled requesting for SFTP credentials in the Customizer.

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


8 weeks ago

#24 @westonruter
8 weeks ago

In 42018:

Customize: Remove theme_installing notification when installation fails.

Amends [41648].
Props celloexpressions.
See #42184, #37661.

#25 @celloexpressions
7 weeks ago

  • Owner celloexpressions deleted

Next steps:

  • Revert [41997]
  • Use 42184.2.diff
  • Fix the two known bugs: incorrect labels after cancelling the modal, and fix broken buttons after cancelling the modal

Note that with 42184.2.diff, SFTP appears to work as expected. The only issues are with canceling the modal, which happens much more in testing than in practice. [41997] seems like an overreaction to the minor bugs that we need to track down with that, which may seem worse than they are because they are more apparent in a testing workflow than in real usage. Either way, the fixes for that should not be particularly difficult. I'm not planning on working on this issue further, so someone else can take it on.

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


6 weeks ago

#27 @johnbillion
4 weeks ago

  • Milestone changed from 4.9.1 to 4.9.2

#28 @westonruter
7 days ago

  • Milestone changed from 4.9.2 to 4.9
  • Resolution set to fixed
  • Status changed from reviewing to closed

Remaining issues to be addressed in #42873.

Note: See TracTickets for help on using tickets.