Site Health false positive: WP_DEBUG_LOG warning when debug.log is outside wp-content - Ticket #64071#10684
Site Health false positive: WP_DEBUG_LOG warning when debug.log is outside wp-content - Ticket #64071#10684hbhalodia wants to merge 54 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Hi Team, Can you please help review and update the wordings if needed? Thank You, |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…t false positives
westonruter
left a comment
There was a problem hiding this comment.
If we wanted to go the extra mile, there could be a loopback request to try to actually request the file over HTTP to see if it returns anything. This may not be helpful in the end, however, as the file may not be present if nothing has been written to the log yet. Just an idea.
Thanks @westonruter, I thought of an edge case as well, what if there is no any log file present, then |
Good point. Well, in that case you could always check to see if wordpress-develop/src/wp-includes/load.php Lines 622 to 623 in 5a53b94 If, however, they do something like: define( 'WP_DEBUG_LOG`, ABSPATH . 'debug.log' );Then checking |
There was a problem hiding this comment.
Pull request overview
This PR fixes a false positive in WordPress Site Health where a warning about WP_DEBUG_LOG being set to a potentially public file is shown even when the debug log is configured to be outside the wp-content directory (and thus not publicly accessible).
Changes:
- Modified the debug mode check to determine if error log files are within or outside the WordPress document root
- Added different status levels (critical vs good) and messages based on whether the log file is publicly accessible
- Differentiated messaging between WP_DEBUG_LOG constant usage and direct PHP error_log configuration
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (5)
tests/phpunit/tests/admin/wpSiteHealth.php:792
- The expected public-log result hardcodes translatable strings (label/description) instead of using
__(). SinceWP_Site_Health::get_test_is_in_debug_mode()builds these messages with__(), using the same in tests avoids coupling to the untranslated English strings.
$result = array(
'status' => 'critical',
'label' => 'Your site is set to log errors to a potentially public file',
'description' => 'The constant, <code>WP_DEBUG_LOG</code>, has been added to this website’s configuration file. This means any errors on the site will be written to a file which is likely publicly accessible.',
'test' => 'is_in_debug_mode',
tests/phpunit/tests/admin/wpSiteHealth.php:814
- The expected private-log result hardcodes translatable strings (label/description). Align this with the rest of the suite by wrapping user-facing strings in
__()so the test isn’t tied to the untranslated English output.
$result = array(
'status' => 'good',
'label' => 'Your site is set to log errors to a file outside the document root',
'description' => 'The configuration constant, <code>WP_DEBUG_LOG</code>, is enabled. In addition, your site is set to write errors to a file outside the WordPress directory, which is a good practice as the log file should not be publicly accessible.',
'test' => 'is_in_debug_mode',
tests/phpunit/tests/admin/wpSiteHealth.php:836
- The expected “unable to determine” result hardcodes translatable strings (label/description). Using
__()here keeps the expectations consistent withWP_Site_Health::get_test_is_in_debug_mode()and the rest of this test class.
$result = array(
'status' => 'critical',
'label' => 'Unable to determine error log file location',
'description' => 'The configuration constant, <code>WP_DEBUG_LOG</code>, is enabled, but the log file location could not be determined.',
'test' => 'is_in_debug_mode',
tests/phpunit/tests/admin/wpSiteHealth.php:1098
- These assertions compare the label against a hardcoded English string. Use the translated string via
__()(as other tests in this file do) to avoid making the test dependent on the default locale.
$this->assertSame( 'recommended', $actual_result['status'], 'Status should be "recommended" when WP_DEBUG_DISPLAY is enabled in development.' );
$this->assertSame( 'Your site is set to display errors to site visitors', $actual_result['label'], 'Label should indicate that errors are displayed to visitors.' );
$this->assertStringContainsString( 'WP_DEBUG_DISPLAY', $actual_result['description'], 'Description should contain WP_DEBUG_DISPLAY.' );
tests/phpunit/tests/admin/wpSiteHealth.php:962
- Same portability issue as the previous test:
/var/logmay not exist, which would makerealpath()fail and change the code path under test. Use an existing directory likesys_get_temp_dir()to keep the test reliable across environments.
$site_health = $this->setup_site_health_with_debug_properties( true, false, null );
$private_log_path = '/var/log/php-error.log';
ini_set( 'error_log', $private_log_path );
The assignments in test_is_in_debug_mode_error_log_private() were not aligned, causing the Coding standards / PHP checks job to fail with Generic.Formatting.MultipleStatementAlignment.NotSameWarning. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (7)
src/wp-admin/includes/class-wp-site-health.php:1493
- The “outside the document root” label is misleading here because the code is only checking whether the resolved error_log directory is inside
realpath( ABSPATH )(the WordPress installation directory), not the web server document root. This could cause confusing guidance for users (and mismatches the wording used in the description).
} elseif ( 'private' === $log_path_status ) {
$result['label'] = __( 'Your site is set to log errors to a file outside the document root' );
$result['status'] = 'good';
tests/phpstan/bootstrap.php:29
- This conditional uses an undefined
$_variable and results inWP_DEBUG_LOGalways being falsey (falseor''), which doesn’t help PHPStan explore code paths where WP_DEBUG_LOG is enabled. Consider defining it from an environment variable so PHPStan sees astring|falseunion that can be truthy.
if ( isset( $_ ) ) {
define( 'WP_DEBUG_LOG', false );
} else {
define( 'WP_DEBUG_LOG', '' );
}
tests/phpunit/tests/admin/wpSiteHealth.php:940
- This test hard-codes a Unix-specific
/var/log/...path. On Windows (and some restricted CI environments)realpath( dirname( ini_get( 'error_log' ) ) )will returnfalse, causing the code under test to take the “unable to determine” branch and making the test fail. Use an existing directory that’s reliably outsideABSPATH, such assys_get_temp_dir().
public function test_is_in_debug_mode_error_log_private(): void {
$site_health = $this->setup_site_health_with_debug_properties( true, true, null );
$private_log_path = '/var/log/php-error.log';
ini_set( 'error_log', $private_log_path );
tests/phpunit/tests/admin/wpSiteHealth.php:962
- Same portability issue as the previous test:
/var/log/...is not guaranteed to exist across platforms, and can makerealpath()fail, changing the expected status. Usesys_get_temp_dir()to ensure the directory exists and is outsideABSPATH.
public function test_is_in_debug_mode_error_log_private_without_wp_debug_log(): void {
$site_health = $this->setup_site_health_with_debug_properties( true, false, null );
$private_log_path = '/var/log/php-error.log';
ini_set( 'error_log', $private_log_path );
tests/phpunit/tests/admin/wpSiteHealth.php:773
- These expected values are hard-coded English strings, while the implementation returns translated strings via
__(). That makes the tests locale-dependent and inconsistent with other assertions in this file (which generally compare against__()). Use__()for the expected label/badge strings to keep the test stable under non-default locales.
return array(
'status' => 'good',
'label' => 'Your site is not set to output debug information',
'test' => 'is_in_debug_mode',
'badge' => array(
tests/phpunit/tests/admin/wpSiteHealth.php:793
- This helper hard-codes expected label/description strings instead of using
__(), which makes the tests locale-dependent. Align the expected values with the implementation (translated strings) by wrapping them in__().
$result = array(
'status' => 'critical',
'label' => 'Your site is set to log errors to a potentially public file',
'description' => 'The constant, <code>WP_DEBUG_LOG</code>, has been added to this website’s configuration file. This means any errors on the site will be written to a file which is likely publicly accessible.',
'test' => 'is_in_debug_mode',
);
tests/phpunit/tests/admin/wpSiteHealth.php:837
- This helper hard-codes expected label/description strings instead of using
__(), which makes the tests locale-dependent. Wrap these expected values in__()to match the production code’s translated strings.
$result = array(
'status' => 'critical',
'label' => 'Unable to determine error log file location',
'description' => 'The configuration constant, <code>WP_DEBUG_LOG</code>, is enabled, but the log file location could not be determined.',
'test' => 'is_in_debug_mode',
);
|
Hi, @hbhalodia, can you review the Copilot suggestions and update this PR accordingly if they are reasonable? |
I would check those and update the PR. Thanks, |
…lop into fix/issue-64071
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/wp-admin/includes/class-wp-site-health.php:1469
ini_get( 'error_log' )can return relative paths or special values (e.g.syslog/stderr). Usingrealpath( dirname( ini_get( 'error_log' ) ) )will resolve these to the current working directory and can incorrectly mark the log as being inside the WordPress directory ("public"), producing false positives/regressions. Consider treating non-absoluteerror_logvalues as "unable to determine" instead of resolving them.
if ( ! empty( ini_get( 'error_log' ) ) ) {
$debug_log_dir = realpath( dirname( ini_get( 'error_log' ) ) );
$absolute_path = realpath( ABSPATH ) . DIRECTORY_SEPARATOR;
if ( false === $debug_log_dir ) {
$log_path_status = 'error';
} elseif ( str_starts_with( $debug_log_dir . DIRECTORY_SEPARATOR, $absolute_path ) ) {
$log_path_status = 'public';
} else {
$log_path_status = 'private';
src/wp-admin/includes/class-wp-site-health.php:1493
- The "private" branch is determined by the log directory being outside
ABSPATH, but the label says "outside the document root". Being outside the WordPress directory does not guarantee being outside the web server document root (e.g. WordPress in a subdirectory of the docroot), so this label is misleading. Consider aligning the label with what’s actually checked.
} elseif ( 'private' === $log_path_status ) {
$result['label'] = __( 'Your site is set to log errors to a file outside the document root' );
$result['status'] = 'good';
tests/phpunit/tests/admin/wpSiteHealth.php:813
- This expected label repeats the same mismatch as the production code: the logic checks for paths outside the WordPress directory (
ABSPATH), not necessarily outside the server document root. If the label inWP_Site_Health::get_test_is_in_debug_mode()is updated to reflect what’s actually checked, the test helper should be updated to match.
$result = array(
'status' => 'good',
'label' => 'Your site is set to log errors to a file outside the document root',
'description' => 'The configuration constant, <code>WP_DEBUG_LOG</code>, is enabled. In addition, your site is set to write errors to a file outside the WordPress directory, which is a good practice as the log file should not be publicly accessible.',
tests/phpstan/bootstrap.php:29
- This conditional uses an undefined variable (
$_) and both branches define falsey values (false/''), which makes the intent hard to understand and doesn’t represent the fact thatWP_DEBUG_LOGcan also be a truthy string path. Unless there is a specific PHPStan issue requiring this, it would be clearer to define the constant directly (as before).
define( 'WP_DEBUG', false );
define( 'WP_DEBUG_DISPLAY', false );
if ( isset( $_ ) ) {
define( 'WP_DEBUG_LOG', false );
} else {
define( 'WP_DEBUG_LOG', '' );
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
tests/phpstan/bootstrap.php:29
- The conditional
if ( isset( $_ ) )is opaque here and (because the bootstrap is executed) will deterministically defineWP_DEBUG_LOGas an empty string. That value is not a typical core configuration and makes the intent of this PHPStan bootstrap harder to understand. Prefer defining a single, explicit default value.
if ( isset( $_ ) ) {
define( 'WP_DEBUG_LOG', false );
} else {
define( 'WP_DEBUG_LOG', '' );
}
src/wp-admin/includes/class-wp-site-health.php:1494
- This branch marks the log location as "private" and sets status to
goodbased only on the log directory being outsideABSPATH. That does not necessarily mean it is outside the document root (e.g. WordPress installed in a subdirectory), so the label "outside the document root" and the "good" status can be misleading / a false negative for publicly accessible log files. Consider rewording the label to match what’s actually checked (outside the WordPress directory), and consider whether the status should remainrecommendedunless you can reliably determine it is outside the web root.
} elseif ( 'private' === $log_path_status ) {
$result['label'] = __( 'Your site is set to log errors to a file outside the document root' );
$result['status'] = 'good';
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
src/wp-admin/includes/class-wp-site-health.php:84
- The constructor’s handling of WP_DEBUG_LOG and WP_DEBUG_DISPLAY is stricter than core’s wp_debug_mode() behavior (wp-includes/load.php:613+). For example, WP_DEBUG_LOG set to 1 or '1' enables logging in wp_debug_mode(), but this code treats it as disabled, which can make Site Health report the wrong messaging/status.
$this->wp_debug = defined( 'WP_DEBUG' ) && WP_DEBUG;
if ( defined( 'WP_DEBUG_LOG' ) && ( is_bool( WP_DEBUG_LOG ) || is_string( WP_DEBUG_LOG ) ) ) {
$this->wp_debug_log = WP_DEBUG_LOG;
} else {
$this->wp_debug_log = false;
}
if ( defined( 'WP_DEBUG_DISPLAY' ) && is_bool( WP_DEBUG_DISPLAY ) ) {
$this->wp_debug_display = WP_DEBUG_DISPLAY;
} else {
$this->wp_debug_display = null;
}
src/wp-admin/includes/class-wp-site-health.php:1469
- ini_get('error_log') may legally be a non-filesystem target (e.g. 'syslog') or a relative path. Using realpath(dirname(...)) will resolve those to the current working directory (e.g. '.') and can incorrectly classify the log as “public” when WordPress is installed in that directory. Consider treating non-absolute paths as “unknown/error” before applying realpath().
if ( ! empty( ini_get( 'error_log' ) ) ) {
$debug_log_dir = realpath( dirname( ini_get( 'error_log' ) ) );
$absolute_path = realpath( ABSPATH ) . DIRECTORY_SEPARATOR;
if ( false === $debug_log_dir ) {
$log_path_status = 'error';
} elseif ( str_starts_with( $debug_log_dir . DIRECTORY_SEPARATOR, $absolute_path ) ) {
$log_path_status = 'public';
} else {
$log_path_status = 'private';
src/wp-admin/includes/class-wp-site-health.php:1493
- The “private” branch is determined by checking whether the error log directory is outside ABSPATH (the WordPress install directory), but the label says it’s outside the “document root”. That wording is inaccurate and could overstate safety when WordPress is installed in a subdirectory of the document root.
} elseif ( 'private' === $log_path_status ) {
$result['label'] = __( 'Your site is set to log errors to a file outside the document root' );
$result['status'] = 'good';
tests/phpunit/tests/admin/wpSiteHealth.php:940
- This test hard-codes a “private” error log path under /var/log, which won’t exist on Windows and can cause realpath(dirname(...)) to fail (making the Site Health result “error/critical” instead of “private/good”). Using sys_get_temp_dir() keeps the test portable while still being outside ABSPATH in typical CI setups.
$site_health = $this->setup_site_health_with_debug_properties( true, true, null );
$private_log_path = '/var/log/php-error.log';
ini_set( 'error_log', $private_log_path );
tests/phpunit/tests/admin/wpSiteHealth.php:962
- This test hard-codes a “private” error log path under /var/log, which can break on platforms where that path doesn’t exist (e.g. Windows), causing realpath(dirname(...)) to return false and the Site Health result to become “error/critical”. Use sys_get_temp_dir() to keep the test portable.
$site_health = $this->setup_site_health_with_debug_properties( true, false, null );
$private_log_path = '/var/log/php-error.log';
ini_set( 'error_log', $private_log_path );
Trac ticket: https://core.trac.wordpress.org/ticket/64071
Screenshots
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.