Shiny Updates: FTP Support

[pento]

Branched from #29820.

This ticket is for tracking the creation of FTP support in Shiny Updates.

[ericlewis]

attachment:31528.diff​ is a work in progress.

It *should* work for updating a plugin and inputting FTP/SSH credentials. I haven't tested it yet because I don't have an FTP server in my Varying Vagrant Vagrants setup I use for core development (as far as I know).

Other notes:

• Before page load, do some intelligent things to decide whether we need to request filesystem credentials. When the user makes an action (e.g. upgrade plugin) that requires credentials, offer them a form to fill out to do this.
• Going with a modal for the moment. We discussed a "terrestrial" form at the top of the page in case the user doesn't have direct as the filesystem method. This would be awkward if the user is going to the Plugin list table and we proffer a form at the top when their goal is to deactivate a plugin, but we put it there in case they want to upgrade a plugin.
• Rewrote some of the update plugin endpoint logic to return an error if filesystem connection failed. I wonder if we should repeatedly request credentials if this failure occurs (i.e. if the user is messing up while inputting their FTP credentials). We probably should.
• No styling at the moment, Functionality First™.

[DrewAPicture]

It pains me to say that despite the recent surge in activity we aren't going to be able to get this one over the finish line for 4.2. We needed to have the big pieces done in time for Beta 1 and it just isn't going to happen. Good effort, folks.

Maybe we can get it done in 4.3.

[DrewAPicture]

Consider my hand slapped. Let's talk about it at the devchat tomorrow and decide if anything is salvageable.

[jorbin]

In 31749:

Request FTP and SSH credentials when needed during shiny updates

This is a first pass at requesting FTP and SSH credentials when needed during shiny updates. Styling and some UX improvements are still needed, but we do show the prompt and use the passed data when doing plugin installs and updates for shiny updates. There are also a couple of areas that we could improve code wise such how we create the requestFilesystemCredentials part of the localized _wpUpdatesSettings. Over the past half century, we've split the atom, we've spliced the gene and we've roamed Tranquility Base. We've reached for the stars and never have we been closer to having them in our grasp. That has nothing to do with shiny updates.

Props ericlewis, jorbin, and drewapicture for testing
Fixes #31528

[johnbillion]

r31749 is a concern.

script-loader.php is a file which gets included in load-scripts.php and load-styles.php, our script and style concatenators.

Since r31749 script-loader.php now calls request_filesystem_credentials() directly, which in turns calls get_filesystem_method(), a function which performs a write, read, close, and delete action on a temporary file (unless you've manually defined WP_FS_METHOD).

We need a better way of determining whether we need to request filesystem credentials. My suggestion would be to trigger an AJAX request which returns the result of request_filesystem_credentials(), rather than calling request_filesystem_credentials() in script-loader.php just to populate a JS boolean.

I don't think beta 1 should ship with the current code.

[ocean90]

Replying to johnbillion:

I don't think beta 1 should ship with the current code.

I agree, current it doesn work because of plugin-install.php:439 Uncaught ReferenceError: jQuery is not defined.

This is the idea while I was working/thinking about this for the language installer:

1. Perform a regular install request.
2. The AJAX callback tries to perfom the action.
3. If it fails because of missing credentials, send an error code back.
4. The response needs to be checked for this error code, if it exists, ask the user for the credentials and perform the same install request again, with the credentials.

[ocean90]

In 31755:

Revert [31749], see #31528.

[ocean90]

Replying to ocean90:

In 31755:

Revert [31749], see #31528.

Reason:

E_ERROR: /wp-admin/includes/file.php:1188 - Call to undefined function submit_button()
Call to undefined function submit_button()
File: /wp-admin/includes/file.php

[jorbin]

The latest patch contains the original, but now reverted, changes along with a fix. Instead of going the ajax route, I went with moving the form generation to be dependent on the need for the form and checking for that instead of doing it inside the script loader.

If you want to see just change from the previous code, https://core.trac.wordpress.org/attachment/ticket/31528/31528.9.diff contains that code.

[jorbin]

For testing, I've been using the ​core-control plugin to force a filesystem credential method.

In order to have a local FTP server, I'm using VVV and running the following command to install and activate an FTP server:
sudo apt-get install vsftpd && sudo sed -i -e "s/\#write_enable=YES/write_enable=YES/" /etc/vsftpd.conf && sudo restart vsftpd

VVV has the username vagrant and the password of vagrant available by default.

[DrewAPicture]

For those following along at home, we're going to wait to roll in FTP support until after Beta 1 ships. This should give us much more time to come up with a solid solution.

[ericlewis]

In attachment:31528.11.diff​, a continuation of @jorbin's work in attachment:31528.10.diff​.

• Set wp.updates.shouldRequestFilesystemCredentials on document.ready; any earlier the element #request-filesystem-credentials-dialog definitely won't exist.
• Change wp.updates.shouldRequestFilesystemCredentials conditionals to basic truthiness, since we're setting it to true or false now. This avoids some rampant plugin update/install requests.
• Remove wp.updates.updateAlways. This also creates rampant request attempts because when every request comes back, it runs the first update on the queue. If an update is failing because of bad filesystem credentials, the lock should actually be kept and the queue not invoked. The queue will be invoked when the user submits the credentials form. For successful updates, remove the lock and hit the queue at the end of the success callback.
• Spell out docs for wp.updates.showErrorInCredentialsForm.

[ericlewis]

In attachment:31528.12.diff​, continuing on attachment:31528.11.diff​:

• update the version number in the list table when a plugin is updated. Since the plugin meta data can be filtered (plugin_row_meta), I opted for some intelligent string replacement (with l10n support) rather than creating new targetable HTML wrappers.
• return an error in case updates are made in rapid succession, the update_plugins site transient has been flushed, and WP doesn't think the plugin needs to be updated. This is a stopgap, and we should look into fixing that.

[jipmoors]

Please see #31616 for a deconstruction of the request_filesystem_credentials function.

[jipmoors]

Questions about wp_print_request_filesystem_credentials_modal considering the filter present internally.

Considering this comment:

 * Plugins may override this form by returning true|false via the
 * {@see 'request_filesystem_credentials'} filter.

Inconsistent URL use
Since the URL is passed to the filter inside the function, results may differ on it's content.

1204 $filesystem_credentials_are_stored = request_filesystem_credentials( self_admin_url() );

versus

1215 <?php request_filesystem_credentials( site_url() ); ?> 

The filter does not guarantee for a 100% that this will result in a credentials request form.

If the filter is implemented somewhere and returning 'true' then the result of this call will be 'true' and not a form.

It would seem more logical to me to capture the output and test if it's not empty (i.e. we have a form) instead of using the return value.

ob_start(); 
$filesystem_credentials_are_stored = request_filesystem_credentials( self_admin_url() ); 
ob_end_clean(); 

That output only has to be captured once (and the function only run once) so it can be used on the apropriate place to request the credentials.

[jorbin]

attachment:31528.12.diff​ is ready to go. Anyone else that wants to put there eyes on it before it lands should do so.

[jorbin]

Summarizing some conversation in slack today:

Helen greped through the plugins and found a couple of plugins using the filter @jipmoore pointed out. I looked at all of them and this is what I found:

plugins/appcastlenet-api/
plugins/iremotewp/
plugins/wpcommand/
plugins/wpremote/

• It looks like they have the ability to override the credentials with something stored on the remote servers. All four are using essentiallyy the same code. This has no affect on things from what I can tell. I've asked Human Made about the wpremote version. ​https://github.com/humanmade/WP-Remote-WordPress-Plugin/commit/ce7fe2fdbe111cf719b501421239d8f36d3aba97#commitcomment-10221682

plugins/jetpack/

• jetpack is bypassing credential checks for it's check to see if plugins need to be updated. Should have no impact on ssh.

plugins/site-manager

• May cause an issue if direct isn't working. I'll ping Marko who is an author of this plugin.

plugins/ssh-sftp-updater-support/

• Appears fine. It is modifying the form a bit. Will also ping plugin author once this lands asking that they make sure to test with beta.

I also tested having the filter return true when it shouldn't and the experience with shiny updates is the same as without shiny updates (aka, broken). This would absolutely appear to be an issue with a plugin and not one we need to worry about.

[jorbin]

attachment:31528.13.diff​ contains a few changes from attachment:31528.12.diff​, related to installs and handling errors in this. This fixes a bug where if a user entered the wrong credentials, they were unable to try again (it would just hang). Additionally, it fixes an issue where if you clicked "install" while an install was running, the second install wouldn't be kicked off automatically.

Unless someone has reasons not to, I'm going to land this version in trunk tonight.

[jorbin]

In 31811:

Request FTP and SSH credentials when needed during shiny updates

This is a restoration of [31749] which was reverted in [31755].

It includes a number of enhancements from the original version. Namely:

• Not doing a credential check in src/wp-includes/script-loader.php
• Add new function wp_print_request_filesystem_credentials_modal
• update the version number in the list table when a plugin is updated

UI still needs further work, but this basic version should enable more testing

Props ericlewis, jorbin
See #31528

[jorbin]

Calling this complete. UI work will continue in #31608. If there are any issues, please open up a new ticket.