Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#54351 new defect (bug)

Checking for temp update directories may throw warnings

Reported by: clorith's profile Clorith Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 6.0
Component: Site Health Keywords: needs-patch needs-testing early dev-feedback
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 (12)

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


2 years ago

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


2 years ago

#3 @audrasjb
2 years ago

  • Keywords needs-patch needs-testing added

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


2 years ago

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


2 years ago

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


2 years ago

#7 @hellofromTonya
2 years ago

  • Keywords early added
  • Milestone changed from 5.9 to 6.0

[51815] and associated changesets were reverted and its ticket #51857 moved to 6.0 for continued refinement and extensive testing. Moving this ticket to 6.0 and marked as early.

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


2 years ago

#9 @costdev
2 years ago

  • Keywords dev-feedback added
  • Version changed from 5.9 to trunk

Per the discussion in the bug scrub, I'm adding the dev-feedback keyword to get as much input on this as possible.

See PR 2225, which touches this ticket as well as #54166, #54543 and #51857. As such, this would greatly benefit from additional testing, particularly on different filesystems.

@pbiron has extensively tested in WP_Filesystem_ftpext and may be able to expand more on results pertinent to this ticket.

Changing the version to trunk as [51815] was reverted in [52351] prior to 5.9's release.

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


2 years ago

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


2 years ago

#12 @peterwilsoncc
2 years ago

  • Milestone changed from 6.0 to Future Release

#51857 was moved to future release last week, so I'll move this alongside it.

If this is effectively a clone/sub-task of the other ticket then it might be possible to close it and continue working on the other ticket.

Note: See TracTickets for help on using tickets.