wp-cli icon indicating copy to clipboard operation
wp-cli copied to clipboard

Respect WP_DEBUG instead of overriding

Open iandunn opened this issue 7 years ago • 21 comments

Currently the value of display_errors is overridden by \Utils\wp_debug_mode(), but I would expect WP_CLI to respect the value set by WP_DEBUG and \wp_debug_mode() instead.

It sounds like there may have once been a reason for overriding it, but it's not known anymore.

In my experience, having it overridden is a hassle, because when I'm writing commands I don't have notice/warning/error messages easily available. In many cases, it might not even be obvious that an error occurred at all. So I have to manually check log files or use a fragile workaround to restore the original settings.

I'd be happy to work on a PR if it's decided that this behavior should be removed.

Background information:

php/utils-wp.php#L26 cb57cc0 #786

iandunn avatar Aug 01 '16 16:08 iandunn

Currently the value of display_errors is overridden by \Utils\wp_debug_mode(), but I would expect WP_CLI to respect the value set by WP_DEBUG and \wp_debug_mode() instead.

Sorry if I'm being a bit dense, but I'm still not sure what the problem is. Can you provide a more detailed example of how this is a problem, what the current behavior is, and what you'd expect the behavior to be?

danielbachhuber avatar Aug 01 '16 16:08 danielbachhuber

Sorry, I probably just didn't describe that clearly.

Steps to reproduce:

  1. define( 'WP_DEBUG', true); in your dev environment's wp-config.php

  2. Create test.php with these contents:

    <?php
    
    echo "hello";
    calling_function_that_doesnt_exist();
    
  3. Run wp eval-file test.php

Expected output:

> wp eval-file foo.php
hello
Fatal error: Call to undefined function calling_function_that_doesnt_exist() in test.php on line 3

Actual output:

> wp eval-file foo.php
hello

display_errors has been disabled by \Utils\wp_debug_mode(), so any notices/warnings/errors thrown by PHP are not directed to STDOUT.

iandunn avatar Aug 01 '16 17:08 iandunn

Running php test.php behaves as expected since display_errors is on by default in my dev environment:

> php foo.php 
helloPHP Fatal error:  Call to undefined function calling_function_that_doesnt_exist() in test.php on line 3

iandunn avatar Aug 01 '16 17:08 iandunn

What would you think about this?

  1. If WP_DEBUG=true, then WP-CLI defaults to displaying errors when --debug is and isn't included. If --no-debug is passed, then WP-CLI silences errors.
  2. If WP_DEBUG=false or isn't set, the WP-CLI defaults to not displaying errors. --debug would enable displaying errors.

It does seem somewhat erroneous for WP-CLI to quietly silence errors when WP_DEBUG is defined in wp-config.php.

danielbachhuber avatar Aug 01 '16 17:08 danielbachhuber

Yeah, that makes sense to me. I'll work on a PR :)

iandunn avatar Aug 01 '16 17:08 iandunn

Yeah, that makes sense to me. I'll work on a PR :)

👍

It'd be good to have explicit test coverage for each of the scenarios, where we don't already.

danielbachhuber avatar Aug 01 '16 22:08 danielbachhuber

Hello, it would be nice to document how to get the current debug verbosity.. For now I have found by myself: if(\WP_CLI::get_config('debug')) { Thanks for the very good WP_CLI!!

myselfhimself avatar Aug 05 '16 10:08 myselfhimself

Yeah, that makes sense to me. I'll work on a PR :)

@iandunn Do you have a timeframe you're willing to commit to for this? Otherwise, I think this issue should be free for the first person to submit a PR.

danielbachhuber avatar Aug 17 '16 12:08 danielbachhuber

Sorry about that, my schedule got more packed than I expected.

I think it's totally fine to consider it open, I didn't mean to call dibs or anything. I'm still planning to get to this, but if someone else wants to submit a PR first that's totally fine with me.

iandunn avatar Aug 17 '16 14:08 iandunn

@iandunn I added a testcase in #3372 for the behavior you described in https://github.com/wp-cli/wp-cli/issues/3220#issuecomment-236649813. The testcase passes, which isn't what I'd expect if the behavior was broken like you initially described.

Can you re-verify the original issue, and confirm there isn't additional code in your environment that might be affecting error reporting?

danielbachhuber avatar Sep 02 '16 16:09 danielbachhuber

Huh, that test passes for me too, and now I can't reproduce the problem in any of my environments. That's odd, since I've been experiencing it for years.

I've updated WP and WP-CLI since opening this issue, but can't think of any other changes that'd be relevant. IIRC, I tested with a fresh WP install before opening the issue, too, to rule out external causes.

Oh well, I guess I'll just consider this resolved unless I run into it again in the future. Sorry for the trouble.

iandunn avatar Sep 06 '16 19:09 iandunn

Oh well, I guess I'll just consider this resolved unless I run into it again in the future. Sorry for the trouble.

No worries! Happy to look into it again if you can track down more steps to reproduce.

danielbachhuber avatar Sep 06 '16 20:09 danielbachhuber

I'm re-opening this because I can reproduce it again, using the steps in https://github.com/wp-cli/wp-cli/issues/3220#issuecomment-236649813

I think there may be a few extra conditions necessary:

  • Xdebug must be enabled.
  • xdebug.force_display_errors must be disabled.

I'll send a PR that shows the test failing.

iandunn avatar Mar 22 '18 19:03 iandunn

Reopening due to the comment above and the open PR.

swissspidy avatar Aug 04 '18 09:08 swissspidy

Is --no-debug still a part of this PR? Because I don't want to get all the notices and warnings from the CLI just because I've set WP_DEBUG true for the site itself. Not sure if that should be its own issue.

bitwombat avatar Aug 26 '18 23:08 bitwombat

Hello I spent some hours on this issue and found that the issue is about the log_errors ini variable. If log_errors is On you can't disable errors reporting with ini_set( 'display_errors', 0); log_errors is set to Off by default in php. In Ubuntu is On for PHP CLI php.ini file /etc/php/7.2/cli/php.ini. WP CLI using Ubuntu for testing (Github actions). I forked and updated @iandunn branch #4740. Added ini_set( 'log_errors', 0 ); and now the scenario didn't pass https://github.com/ahberg/wp-cli/pull/2.

ahberg avatar Feb 18 '21 13:02 ahberg

Kinda related but there's an issue with Xdebug 3 - this line:

https://github.com/wp-cli/wp-cli/blob/master/php/utils-wp.php#L69

Forces the value of display_errors if that function exists but Xdebug can now have it's mode set to off via the xdebug.mode ini setting, in which case the function exists but Xdebug won't do anything.

https://xdebug.org/docs/all_settings#mode

We should check if Xdebug is active too.

roborourke avatar Apr 20 '21 13:04 roborourke

@roborourke I think we don't need to check if xdebug is active. My solution is simple check my PR https://github.com/wp-cli/wp-cli/pull/5501/files. Any feedback is welcome.

ahberg avatar Apr 25 '21 11:04 ahberg

This is still an issue. Even with xdebug.force_display_errors => On => On I'm unable to see any debug messages. Turning XDEBUG off shows the messages.

My current approach is to add <?php ini_set( 'display_errors', 1 ); to an mu-plugin/cli-debug.php file.

soulseekah avatar Mar 31 '22 16:03 soulseekah

Yes is still an issue. Can we label it as a confirmed bug?

ahberg avatar Apr 22 '22 14:04 ahberg