wp_die() may generate doing_it_wrong notices, confusing other WP error handling

[ohiosierra]

When wp_die() runs, it checks to see if a more specific die handler should be run.

When checking if the XML die handler should run, it calls is_feed(), is_comment_feed(), and is_trackback(). If wp_die() was triggered outside of the loop and WP_DEBUG is enabled, these calls will generate "doing_it_wrong" error notices.

In other WordPress functions, such as wp_finalize_scraping_edited_file_errors(), WordPress relies on the PHP function, error_get_last() to determine if a fatal error has occurred. Generally if wp_die() has been triggered by an error, the last error will be the one that triggered it.

However, if a doing_it_wrong notice has been triggered it will take the place of the fatal error.

The net effect is that if WP_DEBUG is enabled, certain protections like the plugin editor rollback feature will not function, because the doing_it_wrong notice hides the fatal error from WordPress.

Expected: wp_die() will not trigger any new errors or notices that may obscure an error that caused wp_die() to run.

[SergeyBiryukov]

Introduced in [45016].

[tmdesigned]

Check for $wp_query before trying is_feed, is_comment_feed, and is_trackback

[tmdesigned]

(I am the bug reporter; @ohiosierra is an older handle of mine).

This issue came up with troubleshooting why #46045 wasn't behaving the same way for me, and as it turns out WP_DEBUG was causing the behavior noted here. That ticket now has a patch that skirts this issue, but I still believe it is better for wp_die() to avoid creating additional warnings / notices.

I've submitted a patch that performs the same check the query functions perform (i.e. is_feed) to determine if it is a _doing_it_wrong condition. Performing the check here (silently) prevents the additional notice being thrown.

The conditional has gotten a little long and could be grouped, but I kept the isset( $wp_query ) checks on each line for readability.

[TimothyBlynJacobs]

This approach makes sense to me and did fix the issue in my testing.

The patch doesn't apply for me, I think because the patch wasn't generated from the root of the checkout? So the file path is just functions.php

[spacedmonkey]

I personally don’t like the current patch from @ohiosierra as it doesn’t fix the underlying issue, that calling the doing_it_wrong function anywhere in the shutdown handler is going to have this issue.

I think we should use the doing_it_wrong_trigger_error filter and return false at the start of the wp_die function. This would fix the larger issue and cleaner.

[tmdesigned]

@spacedmonkey that is definitely a cleaner approach. I've updated a new patch (and this time from the correct dir?)

Thoughts on the priority for the filter? Since we are disabling doing_it_wrong, I think it's unlikely other code would be specifically turning it on anywhere else (other than its default setting).

I was planning on writing a test for this later today.

[tmdesigned]

using filter to disable _doing_it_wrong

[spacedmonkey]

@tmdesigned patch looks good. Couple of piece of feedback.

  add_filter( 'doing_it_wrong_trigger_error', __return_false );

The function call needs single quotes around it.
I could put default priority and arg number. So this

  add_filter( 'doing_it_wrong_trigger_error', '__return_false', 10, 1 );

[tmdesigned]

quoted function call, added default priority & args

[tmdesigned]

Added unit test

[tmdesigned]

I've added a unit test, but I will note that this is my first unit test contribution so I would appreciate feedback.

It was also a little tricky because the unit test environment is already manipulating the error handling, so I had to back pedal that a little bit.

The test throws an error before the patch, and no error after the patch.

[TimothyBlynJacobs]

The original error still needs to be fixed anyways. Calling wp_die() shouldn't cause warnings.

[TimothyBlynJacobs]

I also don't think we should mask all _doing_it_wrongs in wp_die(). The notice is there for a reason, just because you are using wp_die() shouldn't mean that the warning is suppressed, particularly since it is a WP_DEBUG only feature _and_ wp_die() is no longer guaranteed to die().

[tmdesigned]

So we want to stop wp_die() from spawning new error messages, but (if it's not necessarily going to die) not disable doing_it_wrong outside of that context?

In that case could we temporarily add that filter silencing _doing_it_wrong, then remove it?

Back to your first post, I don't see much difference between checking for the global wp_query in wp_die and then not calling those functions (i.e. is_feed) versus calling those, which then triggers doing_it_wrong, which then does the same check. The filter (latter) approach seems a little more roundabout but also more clean in code.

[TimothyBlynJacobs]

I don't think it is cleaner code because it is still causing the notices, the output is just suppressed. The doing_it_wrong_run action will still fire, for instance. Suppressing it by adding a filter also doesn't guarantee the output will not appear, other filters may cause the output to still echo. The cleanest would be making sure core code doesn't cause _doing_it_wrong to be called at all IMO, which is why I personally prefer your initial patch.

I think making the error scraping code more resilient to other errors in the shutdown process should be explored, but I don't think it is necessary to solve this ticket and needs more research. Applying it globally to wp_die() is both not targeted enough ( wp_die() is called far more often in non-error scraping contexts ) and not broad enough to catch the various ways in which the last error can be polluted ( just in core we have doing it wrongs, and all the deprecated triggers ).

[tmdesigned]

Original approach of directly checking for $wp_query, with unit tests

[tmdesigned]

Well I'm really okay with either. I've uploaded the original approach with a matching unit test.

So as a summary for others, there are two approaches here to prevent the last PHP error from being polluted with _doing_it_wrong notices that may be triggered within wp_die().

46813.4 works by using the doing_it_wrong_trigger_error filter to stop _doing_it_wrong from fully executing.

46813.5 works by directly checking for the $wp_query global inside of wp_die() before calling the query functions.

In either case, I would appreciate a second pair of eyes on the related unit test. I will note again that it's my first for WordPress core.

(There are some slight differences between the tests for the 2 approaches, with the tests for 46813.4 being more complicated as it had to disable the _doing_it_wrong handling built into the unit tests. It works well enough, but on failure produces an error instead of a failure which I imagine is more ideal.)

[spacedmonkey]

Looping in @flixos90 . Both me and @TimothyBlynJacobs have our thoughts on this.

[afragen]

I agree with @TimothyBlynJacobs. Core shouldn't generate _doing_it_wrong() notices. We should fix the issue and not hide it.

[tmdesigned]

Just for clarity, neither approach allows the notices to be generated. 46813.4 does allow the _doing_it_wrong function to start, but doesn't allow it to run long enough to create the actual notices via trigger_error (unless the filter is modified elsewhere with higher priority).

[schlessera]

I agree with the above sentiment that Core should never throw doing_it_wrong notices in the first place. It just seems to send out a message of incompetence if Core is misusing itself...

Also, filtering out all notices like that might cloak more serious issues down the road.

Patch 46813.5.diff seems like the way to go after some WPCS clean-up and an explicit declaration of the global (props @TimothyBlynJacobs for the review).

[tmdesigned]

Added global tag in phpdoc and more targeted unit test

[tmdesigned]

Cleaned indentation and removed extra line in comment

[SergeyBiryukov]

In 45206:

Bootstrap/Load: After [45016], make sure wp_die() does not cause _doing_it_wrong() notices if called before $wp_query global is set.

Props tmdesigned, TimothyBlynJacobs.
Fixes #46813.