WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 6 months ago

#46813 closed defect (bug) (fixed)

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

Reported by: ohiosierra Owned by: SergeyBiryukov
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: servehappy has-patch has-unit-tests
Focuses: Cc:
PR Number:

Description

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.

Attachments (7)

46813.diff (1.0 KB) - added by tmdesigned 8 months ago.
Check for $wp_query before trying is_feed, is_comment_feed, and is_trackback
46813.2.diff (512 bytes) - added by tmdesigned 8 months ago.
using filter to disable _doing_it_wrong
46813.3.diff (521 bytes) - added by tmdesigned 8 months ago.
quoted function call, added default priority & args
46813.4.diff (1.8 KB) - added by tmdesigned 8 months ago.
Added unit test
46813.5.diff (2.2 KB) - added by tmdesigned 8 months ago.
Original approach of directly checking for $wp_query, with unit tests
46813.6.diff (2.5 KB) - added by tmdesigned 8 months ago.
Added global tag in phpdoc and more targeted unit test
46813.7.diff (2.3 KB) - added by tmdesigned 8 months ago.
Cleaned indentation and removed extra line in comment

Download all attachments as: .zip

Change History (30)

#1 @SergeyBiryukov
8 months ago

  • Component changed from General to Bootstrap/Load
  • Milestone changed from Awaiting Review to 5.2

Introduced in [45016].

@tmdesigned
8 months ago

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

#2 @tmdesigned
8 months ago

  • Keywords has-patch added

(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.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


8 months ago

#4 @TimothyBlynJacobs
8 months ago

  • Keywords needs-refresh added

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

#5 @spacedmonkey
8 months ago

  • Keywords serve-happy needs-patch added; has-patch removed

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.

#6 @spacedmonkey
8 months ago

  • Keywords servehappy added; serve-happy removed

#7 @tmdesigned
8 months ago

@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
8 months ago

using filter to disable _doing_it_wrong

#8 @spacedmonkey
8 months ago

  • Keywords has-patch added; needs-refresh needs-patch removed

@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
8 months ago

quoted function call, added default priority & args

@tmdesigned
8 months ago

Added unit test

#9 @tmdesigned
8 months ago

  • Keywords has-unit-tests added

#10 @tmdesigned
8 months ago

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.

#11 @TimothyBlynJacobs
8 months ago

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

#12 @TimothyBlynJacobs
8 months ago

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().

#13 @tmdesigned
8 months ago

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.

#14 @TimothyBlynJacobs
8 months ago

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
8 months ago

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

#15 @tmdesigned
8 months ago

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.)

#16 @spacedmonkey
8 months ago

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

#17 @afragen
8 months ago

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

#18 @tmdesigned
8 months ago

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).

This ticket was mentioned in Slack in #core-php by schlessera. View the logs.


8 months ago

#20 @schlessera
8 months ago

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).

#21 @SergeyBiryukov
8 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@tmdesigned
8 months ago

Added global tag in phpdoc and more targeted unit test

@tmdesigned
8 months ago

Cleaned indentation and removed extra line in comment

#22 @SergeyBiryukov
8 months ago

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

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.

#23 @spacedmonkey
6 months ago

  • Component changed from Bootstrap/Load to Site Health
Note: See TracTickets for help on using tickets.