WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#36708 closed defect (bug) (fixed)

Silence ini_set() in wp_debug_mode() if WP_DEBUG is off

Reported by: SergeyBiryukov Owned by: swissspidy
Milestone: 4.5.3 Priority: normal
Severity: normal Version: 4.5
Component: Bootstrap/Load Keywords: has-patch commit fixed-major
Focuses: Cc:
PR Number:

Description

In [24564], we've added ini_set( 'display_errors', 0 ) for XML-RPC requests.

In [36571], we did the same for AJAX requests. Apparently this caused issues on sites where ini_set() has been disabled for security reasons, see comment:24:ticket:26262.

As this line fires regardless of WP_DEBUG value and can lead to unexpected errors in production, we should add error suppression here (at least if WP_DEBUG is disabled), as we already do in every other instance of ini_set() in core.

Attachments (2)

36708.patch (414 bytes) - added by SergeyBiryukov 3 years ago.
36708.2.patch (683 bytes) - added by avagoldin 3 years ago.
Added WP_DEBUG check.

Download all attachments as: .zip

Change History (12)

#1 follow-up: @avagoldin
3 years ago

#36692
Maybe ini_set( 'display_errors', 0 ); only if WP_DEBUG is false?


#2 in reply to: ↑ 1 ; follow-up: @adamsilverstein
3 years ago

Replying to avagoldin:

#36692
Maybe ini_set( 'display_errors', 0 ); only if WP_DEBUG is false?

I agree with this suggestion: logging can be useful here to debug a failing ajax or xml-rpc callback. Even though displaying errors breaks the calls, it also exposes the errors more directly for developers who may not have file/log file access. If I had WP_DEBUG true I would expect to see errors regardless of the context.

As a side note, it would be good to check wp-api calls to see if we follow the same logic of suppressing errors there.

@avagoldin
3 years ago

Added WP_DEBUG check.

#3 in reply to: ↑ 2 @avagoldin
3 years ago

Replying to adamsilverstein:

I agree with this suggestion: logging can be useful here to debug a failing ajax or xml-rpc callback. Even though displaying errors breaks the calls, it also exposes the errors more directly for developers who may not have file/log file access. If I had WP_DEBUG true I would expect to see errors regardless of the context.

I added a patch with a check for WP_DEBUG.

As a side note, it would be good to check wp-api calls to see if we follow the same logic of suppressing errors there.

if ( ! WP_DEBUG && ( defined( 'XMLRPC_REQUEST' ) || defined( 'REST_REQUEST' ) || ( defined( 'DOING_AJAX' ) && DOING_AJAX ) ) ) {
	@ini_set( 'display_errors', 0 );
}

defined( 'REST_REQUEST' ) is in the test if it is what you mean.

#4 @ocean90
3 years ago

#36777 was marked as a duplicate.

#5 @ocean90
3 years ago

  • Keywords commit added

For 4.5.3 we should go with 36708.patch. Changing the value based on WP_DEBUG should be handled in a new ticket.

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


3 years ago

#7 @swissspidy
3 years ago

  • Owner set to swissspidy
  • Resolution set to fixed
  • Status changed from new to closed

In 37448:

Bootstrap/Load: Silence ini_set() in wp_debug_mode().

Props SergeyBiryukov.
Fixes #36708 for trunk.

#8 @swissspidy
3 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#9 @ocean90
3 years ago

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

In 37451:

Bootstrap/Load: Silence ini_set() in wp_debug_mode().

Merge of [37448] to the 4.5 branch.

Props SergeyBiryukov.
Fixes #36708.

This ticket was mentioned in Slack in #core-restapi by borekb. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.