WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 7 weeks ago

#51383 reviewing defect (bug)

PHP error body class added for suppressed errors

Reported by: desrosj Owned by: SergeyBiryukov
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.5
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

When a PHP error occurs in the admin that is suppressed using the @ operator, the .php-error class is still added to the <body> even though the error will not be output.

To reproduce, use error suppression on code that invokes an E_WARNING level error. I discovered this with code calling the mkdir() method within the WP_Filesystem_Direct class for a pre-existing directory.

@mkdir( ABSPATH . 'wp-content/themes/' ); should reproduce the issue after ensuring nothing else is actively capturing and processing PHP errors (such as Query Monitor).

Follow up to #35155 in 5.5 and #51073 in 5.5.1.

Attachments (2)

51383.diff (987 bytes) - added by stoyangeorgiev 7 months ago.
51383.patch (1004 bytes) - added by mukesh27 6 months ago.
Added space between condition

Download all attachments as: .zip

Change History (17)

#1 @desrosj
7 months ago

Placing this in the 5.5.2 milestone as this would be a good to fix to backport.

Also, I forgot to include why this is undesirable. The .php-error class ensures PHP errors are not hidden behind the admin bar. But since the error is suppressed, it just results in a blank space at the top of the page.

Last edited 7 months ago by desrosj (previous) (diff)

#2 @stoyangeorgiev
7 months ago

  • Keywords has-patch added; needs-patch removed

Hello there, I have added the E_WARNING to the condition, and when the error is silenced, the empty space is not present however, if the error is not silenced part of the message appears under the menu. I will tweak it a bit more, but if anyone has another idea, I am open to suggestions. I have tried using,

! in_array( $error_get_last['type'], array( E_NOTICE, E_WARNING )

, but the second condition causes the empty space to be present if the error is silenced elsewhere.

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


6 months ago

#4 @SergeyBiryukov
6 months ago

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

#5 @audrasjb
6 months ago

During today’s bug scrub, @SergeyBiryukov shared some concerns about the proposed approach. By the way, I tested the patch and it looks like it fixes the issue.

@mukesh27
6 months ago

Added space between condition

#6 @mukesh27
6 months ago

It seems patch not consider last space in the code

&& ( E_NOTICE !== $error_get_last['type'] || 'wp-config.php' !== wp_basename( $error_get_last['file'] ) ) 

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


6 months ago

#8 @whyisjake
6 months ago

  • Keywords commit added

@SergeyBiryukov, any other thoughts on this one? Looks good to me.

#9 @SergeyBiryukov
6 months ago

  • Milestone changed from 5.5.2 to 5.6

Would like some more time to review this, moving to 5.6 for now.

#10 @SergeyBiryukov
5 months ago

  • Keywords commit removed
  • Milestone changed from 5.6 to 5.7

Some history here:

Previously:

See also: #28376.

With 5.6 RC in a few days, moving to the next release for now for a deeper review.

#11 @audrasjb
3 months ago

I just encountered the issue in the real world.

I applied the patch and it worked fine on my side. @SergeyBiryukov not marking this for commit as you wanted to take the time to deeply review the patch, but it looks good to go on my side :)

#12 @audrasjb
3 months ago

For what it's worth, the patch still applies cleanly against trunk :)

#13 @lukecarbis
2 months ago

Would love to get this in 5.7, but it would need to be merged inside a week. Think we can make that happen @SergeyBiryukov ?

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


7 weeks ago

#15 @hellofromTonya
7 weeks ago

  • Milestone changed from 5.7 to 5.8

5.7 RC1 is tomorrow. Per scrub discussion today with @SergeyBiryukov:

I think the patch might fix the issue at hand, but I'm not sure that code still does what it's intended to do then :slightly_smiling_face: Since it's not specific to 5.7, I think it can be punted to 5.8.

Moving this ticket to 5.8.

Note: See TracTickets for help on using tickets.