WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 6 days ago

#54351 new defect (bug)

Checking for temp update directories may throw warnings

Reported by: Clorith Owned by:
Milestone: 5.9 Priority: normal
Severity: normal Version: trunk
Component: Site Health Keywords: needs-patch needs-testing
Focuses: Cc:

Description

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 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:

$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.

Change History (6)

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


3 weeks ago

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


3 weeks ago

#3 @audrasjb
3 weeks ago

  • Keywords needs-patch needs-testing added

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


3 weeks ago

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


8 days ago

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


6 days ago

Note: See TracTickets for help on using tickets.