[wp-trac] [WordPress Trac] #51170: FTP automatic updates are not RFC 959 compliant for NLST command

[WordPress Trac]

#51170: FTP automatic updates are not RFC 959 compliant for NLST command
------------------------------+-----------------------
 Reporter:  giox069           |       Owner:  afragen
     Type:  defect (bug)      |      Status:  assigned
 Priority:  normal            |   Milestone:  6.0
Component:  Filesystem API    |     Version:  3.7
 Severity:  normal            |  Resolution:
 Keywords:  has-patch commit  |     Focuses:
------------------------------+-----------------------
Changes (by costdev):

 * keywords:  dev-feedback has-patch needs-testing => has-patch commit


Comment:

 This ticket proposes making `WP_Filesystem_FTPext::exists()` and
 `WP_Filesystem_ftpsockets` compliant with RFC 959 by only using NLST on
 directories, rather than on both directories and files.

 -----

 Using `ftp_nlist()` requires checking an array. As the number of files in
 the directory are unknown, this approach has inconsistent performance.

 Using `ftp_size()` looks like a better option. While the
 [https://www.php.net/manual/en/function.ftp-size.php PHP Manual] says that
 it may not be available on all servers, I spoke to @rawrly in the
 #hosting-community channel who believes that this may be because the FTP
 `SIZE` command was introduced in 2002 and in the years that followed, this
 may not have been enabled by some. However, this is believed to be less
 and less likely as things move towards sFTP.

 -----

 [https://github.com/WordPress/wordpress-develop/pull/2460 PR 2460] uses:
 - `WP_Filesystem_FTPext::is_dir()` and
 `WP_Filesystem_FTPsockets::is_dir()` to check for a directory.
 - `WP_Filesystem_FTPext::size()` (which uses `ftp_size()` internally) to
 check for a file.
 - `WP_Filesystem_ftpsockets::size()` (which uses
 [https://core.trac.wordpress.org/browser/tags/5.9/src/wp-admin/includes
 /class-ftp.php#L420 ftp_base::filesize] internally) to check for a file.

 This approach seems sound to me, as the respective `::is_dir()` and
 `::size()` should be returning accurate results.

 -----

 I've tested [https://github.com/WordPress/wordpress-develop/pull/2460 PR
 2460] with `FS_METHOD` set to `ftpext` and `ftpsockets`.

 In both cases, these were the results when using the respective
 `::exists()` method:
 ||= **Test** =||= **Result** =||
 || A directory that exists || true ||
 || An empty hidden file || true ||
 || A non-empty hidden file || true ||
 || An empty file || true ||
 || A non-empty file || true ||
 || A directory that does not exist || false ||
 || A file that does not exist || false ||

 -----

 In addition, the following Dashboard actions continue to work as expected
 for both `ftpext` and `ftpsockets`:

 **Plugins**
 - Install plugin from dot org
 - Install plugin from .zip
 - Delete plugin via Dashboard

 **Themes**
 - Install theme from dot org
 - Install theme from .zip
 - Delete theme via Dashboard

 **Media**
 - Upload image via Media Library
 - Delete image via Media Library

 -----

 Nominating [https://github.com/WordPress/wordpress-develop/pull/2460 PR
 2460] as a `commit` candidate.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/51170#comment:17>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform