Make WordPress Core

Opened 15 years ago

Closed 15 years ago

#11987 closed defect (bug) (fixed)

Incorrect handling of WP_DEBUG_DISPLAY

Reported by: ocean90's profile ocean90 Owned by: nacin's profile nacin
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Inline Docs Keywords:
Focuses: Cc:

Description

In wp-settings.php exists

// Add define('WP_DEBUG_DISPLAY', false); to wp-config.php to use the globally configured setting for display_errors and not force it to On
if ( ! defined('WP_DEBUG_DISPLAY') || WP_DEBUG_DISPLAY )
   ini_set('display_errors', 1);

But it should be a 0, so that it should look as follow

// Add define('WP_DEBUG_DISPLAY', false); to wp-config.php to use the globally configured setting for display_errors and not force it to On
if ( ! defined('WP_DEBUG_DISPLAY') || WP_DEBUG_DISPLAY )
   ini_set('display_errors', 0);

Problem:
If I add define('WP_DEBUG', true); to wp-config.php it shows me the erros as default. If I add then define('WP_DEBUG_DISPLAY', false); it makes no sense.

Attachments (1)

11987.diff (443 bytes) - added by sivel 15 years ago.
Allow user to override php.ini to disable display_errors when WP_DEBUG is enabled.

Download all attachments as: .zip

Change History (17)

#1 follow-up: @nacin
15 years ago

  • Milestone 2.9.2 deleted
  • Resolution set to invalid
  • Status changed from new to closed

This code was split up significantly in #11881 but the logic remains the same.

The functionality of define('WP_DEBUG', true); has never changed -- it sets a more inclusive error_reporting level and the errors are shown. Months ago WP_DEBUG_DISPLAY was introduced as a way to then hide errors but still set the error_reporting level to include E_NOTICE. Because errors are by default shown and the constant is called *_DISPLAY, it needs to be defined as false to hide them.

#2 in reply to: ↑ 1 @ocean90
15 years ago

Thats right, but in 2.9.1/2.9.1.1 it's a bug, so I post it here.
In the current trunk it's fixed, I forgot to look at it, sry.

#3 @nacin
15 years ago

Not a problem, I was just pointing out for reference that wp-settings.php was severely chopped up in trunk.

Here's how it works. If you define WP_DEBUG as true, then the error_reporting level is set to E_ALL (minus E_DEPRECATED and E_STRICT if PHP 5.3). Additionally, display_errors is forced on.

To prevent display_errors from being forced on, you must define WP_DEBUG_DISPLAY as false, which is why this code works. (The statement checks if WP_DEBUG_DISPLAY isn't defined or if it is true, which leaves it being defined && is false as the override.)

WordPress leaves it to the server for whether display_errors should be on otherwise. But when WP_DEBUG is on, it overrides the setting except when WP_DEBUG_DISPLAY is defined as false.

Make sense? I tried to make it a little more clear in in trunk's default-constants.php, such that it is defined by WP as true when not defined, and in how they are handled in wp_debug_mode() in wp-includes/load.php.

#4 @ocean90
15 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Summary changed from Use ini_set('display_errors', 0) instead of ini_set('display_errors', 1); in wp-settings.php to Incorrect handling of WP_DEBUG_DISPLAY
  • Version changed from 2.9.1 to 3.0

Sry, but please check it again.

in wp-load.php

if ( WP_DEBUG_DISPLAY )
   ini_set( 'display_errors', 0 );

And now I should use define('WP_DEBUG_DISPLAY', false); to hide the errors. But it didn't make sense, because in this context if(WP_DEBUG_DISPLAY) must be true to set display_errors 0.

In my opinion their is an ! missing:

if ( !WP_DEBUG_DISPLAY )
   ini_set( 'display_errors', 0 );

PS: in wp-load.php line 221 is an e too much (eerrors to debug.log)

#5 follow-ups: @nacin
15 years ago

No, in wp-includes/load.php (not wp-load.php):

if ( WP_DEBUG_DISPLAY )
   ini_set('display_errors', 1);

If you define('WP_DEBUG_DISPLAY', false);, then the default server setting will not be overridden. At no time does WP ever override display_errors off. It only forces it on when WP_DEBUG unless specified otherwise.

Thanks for the note on the typo in the inline docs, I'll take care of that in #11881.

#6 @ocean90
15 years ago

  • Resolution set to invalid
  • Status changed from reopened to closed
  • Version 3.0 deleted

I do not know how I came to wp-load.php, I mean wp-includes/load.php.

At no time does WP ever override display_errors off.

Ahhh, and I thought, that define('WP_DEBUG_DISPLAY', false); should do this...

So we should closed it and forget it....

#7 in reply to: ↑ 5 ; follow-up: @adam.spiers
15 years ago

  • Cc adam.spiers added
  • Resolution invalid deleted
  • Status changed from closed to reopened

Hi guys, hope you don't mind if I reopen this:

Replying to nacin:

If you define('WP_DEBUG_DISPLAY', false);, then the default server setting will not be overridden. At no time does WP ever override display_errors off. It only forces it on when WP_DEBUG unless specified otherwise.

Can I suggest that it would be more useful if WP *did* allow you to override display_errors to off? After all, the default PHP value is 1 (on), so on a modern vanilla PHP install, WP_DEBUG_DISPLAY is completely ineffective: regardless of whether you leave it undefined or set it to true or false, the end result is that errors will get displayed.

It's worth reviewing the text from the original ticket for this variable (#10202):

Often enough when users enable WP_DEBUG they still do not get any good information because most hosts have display_errors set to Off and some times do not have access to the error logs.

Enabling display_errors when WP_DEBUG is true would make troubleshooting much easier.

We can add a constant for disabling display_errors for those people that would rather just have the errors sent to the globally defined error log file.

So the original intent was to

  • (a) enable display_errors by default when WP_DEBUG is true, but also
  • (b) allow disabling of display_errors.

The current trunk code only satisfies (a).

Instead, I propose the following behaviour:

  • undefined and WP_DEBUG false -> don't override server setting of display_errors
  • undefined and WP_DEBUG true -> override server setting to 1
  • 0 -> override server setting to 0
  • 1 -> override server setting to 1

This follows the Principle of Least Surprise, since if a user makes the effort to set WP_DEBUG_DISPLAY, they will expect it to have an effect.

In other words:

function wp_debug_mode() {
        ...

        if ( defined( 'WP_DEBUG_DISPLAY' ) ) {
                ini_set( 'display_errors', WP_DEBUG_DISPLAY );
        }

        if ( WP_DEBUG ) {
                ...
                if ( ! defined( 'WP_DEBUG_DISPLAY' ) ) {
                        /* Default to displaying errors as per #10202 */
                        ini_set( 'display_errors', 1 );
                }
                ...
        }
        ...
}

This gives the WP admin full flexibility in their choice of setting for display_errors without requiring that they have access to the server settings (often they don't).

Thoughts?

#8 in reply to: ↑ 5 @miqrogroove
15 years ago

Replying to nacin:

No, in wp-includes/load.php (not wp-load.php):

if ( WP_DEBUG_DISPLAY )
   ini_set('display_errors', 1);

If you define('WP_DEBUG_DISPLAY', false);, then the default server setting will not be overridden. At no time does WP ever override display_errors off. It only forces it on when WP_DEBUG unless specified otherwise.

Thanks for the note on the typo in the inline docs, I'll take care of that in #11881.

I have to agree with Adam, I think nacin was asleep at the wheel when he wrote that reply. Maybe he can revise his comments?

#9 @sivel
15 years ago

  • Keywords has-patch dev-feedback added; WP_DEBUG removed

Here is the current function:

function wp_debug_mode() {
    if ( WP_DEBUG ) {
        if ( defined( 'E_DEPRECATED' ) )
            error_reporting( E_ALL & ~E_DEPRECATED & ~E_STRICT );
        else
            error_reporting( E_ALL );

        if ( WP_DEBUG_DISPLAY )
            ini_set( 'display_errors', 1 );

        if ( WP_DEBUG_LOG ) {
            ini_set( 'log_errors', 1 );
            ini_set( 'error_log', WP_CONTENT_DIR . '/debug.log' );
        }
    } else {
        if ( defined( 'E_RECOVERABLE_ERROR' ) )
            error_reporting( E_ERROR | E_WARNING | E_PARSE | E_USER_ERROR | E_USER_WARNING | E_RECOVERABLE_ERROR );
        else
            error_reporting( E_ERROR | E_WARNING | E_PARSE | E_USER_ERROR | E_USER_WARNING );
    }
}

You are correct, WordPress makes no attempt at disabling display_errors. When it was written the only intention was to give people a way to enable it when their host had it disabled. Setting to allows for you to make WP not "force" display_errors. The intention was not to allow people to disable it when their host explicitly had enabled it.

This came from a conversation on IRC with Ryan, in which he had display_errors disabled in php.ini and did not want display_errors enabled when WP_DEBUG was set to true.

If you really want the functionality to actually override php.ini and disable display_errors when WP_DEBUG is set, that should be do able. I'll attach a patch in a moment. Keep in mind that these settings only affect display_errors when WP_DEBUG is enabled.

@sivel
15 years ago

Allow user to override php.ini to disable display_errors when WP_DEBUG is enabled.

#10 in reply to: ↑ 7 @miqrogroove
15 years ago

Replying to adam.spiers:

So the original intent was to

  • (a) enable display_errors by default when WP_DEBUG is true, but also
  • (b) allow disabling of display_errors.

This is not correct either. The WP_DEBUG_DISPLAY constant allows you to use the server's default display_errors setting in WordPress. That is its only purpose. If you set it to TRUE or do not define it, then WordPress will always turn on display_errors, as you pointed out.

#11 @nacin
15 years ago

miqrogroove thought I was sleeping because of "At no time does WP ever override display_errors off." What I meant was that at no time will WP ever override display_errors to off, not when off.

Anyway, I can improve the inline documentation for these constants in wp_debug_mode() and default-constants.php, but that's all that's needed. Really we need three settings, force on, server setting, force off. I don't think changing the functionality of WP_DEBUG_DISPLAY is a good idea or necessary (defining it as true, remember, pushes it from force on to server setting). I also don't find it necessary to introduce a new WP_DEBUG_HIDE constant as force off.

This gives the WP admin full flexibility in their choice of setting for display_errors without requiring that they have access to the server settings (often they don't).

You don't need access to the server settings, you can do exactly what WP would do and use ini_set(). This will turn on WP_DEBUG and force it to off:

define('WP_DEBUG', true);
define('WP_DEBUG_DISPLAY', true); // don't mess with server value
ini_set('display_errors', 0); // only time the server value will be messed with

I'm going to leave this open because I'm going to commit that typo fix and also some clearer documentation ("Add <code>define('WP_DEBUG_DISPLAY', false);</code> to wp-config.php to disable the display of errors," as evidenced here, is inaccurate.)

#12 @nacin
15 years ago

  • Component changed from General to Inline Docs
  • Keywords has-patch dev-feedback removed
  • Milestone set to 3.0
  • Owner set to nacin
  • Status changed from reopened to accepted

#13 follow-up: @nacin
15 years ago

Will need some more coffee.

"defining it as true, remember, pushes it from force on to

server setting" -- s/true/false/. So:

define('WP_DEBUG_DISPLAY', true); // default setting. FORCE ON
define('WP_DEBUG_DISPLAY', false); // don't force. use server setting

Additionally my three-liner for FORCE OFF was wrong:

efine('WP_DEBUG', true);
define('WP_DEBUG_DISPLAY', false); // don't mess with server value
ini_set('display_errors', 0); // only time the server value will be messed with

I'm not going to lie, it's confusing. :)

#14 in reply to: ↑ 13 ; follow-up: @miqrogroove
15 years ago

Replying to nacin:

Additionally my three-liner for FORCE OFF was wrong:

It should work? The current version should override log_errors, not display_errors.

#15 in reply to: ↑ 14 @nacin
15 years ago

Replying to miqrogroove:

It should work? The current version should override log_errors, not display_errors.

From wp_debug_mode() in wp-includes/load.php:

	if ( WP_DEBUG_DISPLAY )
		ini_set( 'display_errors', 1 );

	if ( WP_DEBUG_LOG ) {
		ini_set( 'log_errors', 1 );
		ini_set( 'error_log', WP_CONTENT_DIR . '/debug.log' );
	}

WP_DEBUG_LOG is by default false. WP_DEBUG_DISPLAY is by default true.

#16 @nacin
15 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [13176]) Better inline documentation for WP_DEBUG, WP_DEBUG_DISPLAY, and WP_DEBUG_LOG. Fixes #11987

Note: See TracTickets for help on using tickets.