[wp-trac] [WordPress Trac] #54351: Checking for temp update directories may throw warnings

[WordPress Trac]

#54351: Checking for temp update directories may throw warnings
--------------------------+--------------------
 Reporter:  Clorith       |      Owner:  (none)
     Type:  defect (bug)  |     Status:  new
 Priority:  normal        |  Milestone:  5.9
Component:  Site Health   |    Version:  trunk
 Severity:  normal        |   Keywords:
  Focuses:                |
--------------------------+--------------------
 in [51815] the Site Health function `get_test_update_temp_backup_writable`
 was introduced, which is meant to check if upgrade directories exist, and
 are writable.

 When visiting the Site Health screen in a scenario where `WP_Filesystem`
 uses `ftpext` for manipulating the filesystem, this causes multiple
 warnings as the various calls to check for directories, and if they are
 writable, are causing PHP's `ftp_*` functions to fire, when there may not
 be a valid FTP connection available. (see [https://github.com/WordPress
 /wordpress-develop/blob/07ad6efdf7157d22424496d39d8c5635f28ecfbb/src/wp-
 admin/includes/class-wp-site-health.php#L1968-L1978 class-wp-site-
 health.php:1968-1678]

 I wonder if the best solution here might be to inject a connection call,
 which could then be used to determine if any other fields should be
 checked, or revert to a default value, I'm thinking along these lines:

 {{{#!php
 $filesystem_is_connected = $wp_filesystem->connect();

 $wp_content = ( $filesystem_is_connected ?
 $wp_filesystem->wp_content_dir() : false );

 $upgrade_dir_exists      = ( $filesystem_is_connected ?
 $wp_filesystem->is_dir( "$wp_content/upgrade" ) : false );
 $upgrade_dir_is_writable = ( $filesystem_is_connected ?
 $wp_filesystem->is_writable( "$wp_content/upgrade" ) : false );
 $backup_dir_exists       = ( $filesystem_is_connected ?
 $wp_filesystem->is_dir( "$wp_content/upgrade/temp-backup" ) : false );
 $backup_dir_is_writable  = ( $filesystem_is_connected ?
 $wp_filesystem->is_writable( "$wp_content/upgrade/temp-backup" ) : false
 );

 $plugins_dir_exists      = ( $filesystem_is_connected ?
 $wp_filesystem->is_dir( "$wp_content/upgrade/temp-backup/plugins" ) :
 false );
 $plugins_dir_is_writable = ( $filesystem_is_connected ?
 $wp_filesystem->is_writable( "$wp_content/upgrade/temp-backup/plugins" ) :
 false );
 $themes_dir_exists       = ( $filesystem_is_connected ?
 $wp_filesystem->is_dir( "$wp_content/upgrade/temp-backup/themes" ) : false
 );
 $themes_dir_is_writable  = ( $filesystem_is_connected ?
 $wp_filesystem->is_writable( "$wp_content/upgrade/temp-backup/themes" ) :
 false );
 }}}

 It should be noted that by providing `false` as the default value for all
 fields, we are essentially marking this check as valid, which may not be
 true at all, because if WordPress can't connect to the filesystem, it
 should instead be a failed test.
 The directories should probably be considered non-writable if they can't
 even be reached, this needs different logic further into the checks as
 well?

 This may still lead to a warning as well, if the `connect()` function is
 missing variables, in testing where no information is provided, it only
 complained about a missing hostname, we'll handle that in a separate
 ticket for the Filesystem API component.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/54351>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform