WordPress.org
Get WordPress

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#51383 reviewing defect (bug)

PHP error body class added for suppressed errors

Reported by: desrosj's profile desrosj Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.5
Component: Administration Keywords: has-patch has-unit-tests
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 (4)

51383.diff​ (987 bytes) - added by stoyangeorgiev 4 years ago.
51383.patch​ (1004 bytes) - added by mukesh27 4 years ago.
Added space between condition
51383.1.diff​ (687 bytes) - added by audrasjb 3 years ago.
Patch refreshed agains trunk
51383.2.diff​ (3.3 KB) - added by mcaskill 23 months ago.
Add filter to preempt printing the .php-error CSS class to make PHP errors visible

Download all attachments as: .zip

Change History (43)

#1 @desrosj
4 years 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 4 years ago by desrosj (previous) (diff)

@stoyangeorgiev
4 years ago

#2 @stoyangeorgiev
4 years 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.
4 years ago

#4 @SergeyBiryukov
4 years ago

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

#5 @audrasjb
4 years 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
4 years ago

Added space between condition

#6 @mukesh27
4 years 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.
4 years ago

#8 @whyisjake
4 years ago

  • Keywords commit added

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

#9 @SergeyBiryukov
4 years 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
4 years 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
4 years 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
4 years ago

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

#13 @lukecarbis
4 years 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.
4 years ago

#15 @hellofromTonya
4 years 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.

This ticket was mentioned in ​Slack in #core by chaion07. ​View the logs.
4 years ago

#17 @hellofromTonya
3 years ago

  • Milestone changed from 5.8 to 5.9

I agree with Sergey that a deeper review is needed to ensure the original intent remains. Also should revisit his suggestions here:

In a future release, we could try something like:

  • Saving the value of error_reporting() before it's affected by wp_debug_mode().

. - Using that to only print the class if we determine that the error was displayed according to that setting.

As the issue is not due to 5.7 or 5.8 and today is 5.8 Beta 1, punting to 5.9.

Adding to my tasks list to help out for resolution landing in 5.9.

@audrasjb
3 years ago

Patch refreshed agains trunk

This ticket was mentioned in ​Slack in #core by audrasjb. ​View the logs.
3 years ago

#19 @hellofromTonya
3 years ago

  • Owner changed from SergeyBiryukov to hellofromTonya

Reassigning to me to ensure it gets into my review queue.

#20 @audrasjb
3 years ago

FWIW, I just checked the last proposed patch and it still applies cleanly against trunk @hellofromTonya :)

This ticket was mentioned in ​Slack in #core by audrasjb. ​View the logs.
3 years ago

#22 @hellofromTonya
3 years ago

  • Keywords needs-testing added
  • Milestone changed from 5.9 to 6.0

Likely won't get time to review with 5.9 Beta 1 in 5 days. Though this fix is straightforward, there's history that needs to be considered to ensure the fix retains the expected behavior and fixes the reported issue. It'll testing.

This ticket was mentioned in ​Slack in #core by costdev. ​View the logs.
3 years ago

#24 @costdev
3 years ago

Per the discussion in the bug scrub, pinging @hellofromTonya to see if you've had time/have time to do the review on this?

#25 @audrasjb
3 years ago

  • Keywords needs-refresh added

The patch doesn't apply anymore against trunk.
I'm adding a PR right now :)

This ticket was mentioned in ​PR #2490 on ​WordPress/wordpress-develop by ​audrasjb.
3 years ago #26

  • Keywords needs-refresh removed

Trac ticket: https://core.trac.wordpress.org/ticket/51383

This ticket was mentioned in ​Slack in #core by chaion07. ​View the logs.
3 years ago

#28 @audrasjb
3 years ago

@SergeyBiryukov @hellofromTonya The change works on my side, but it looks like you both wanted to have a second pair on eyes on this. Are you available to double check this PR?

#29 @peterwilsoncc
3 years ago

  • Keywords needs-refresh added; needs-testing removed

I don't think the patch is correct.

The PHP class ought to be added if the warning is unsuppressed but with the ​linked pull request the class is never displayed.

As a proof of concept, apply the patch and add trigger_error( 'some warning', E_WARNING ); prior to the definition of $error_get_last.

The warning will be displayed but the class won't be added to the body element.

Developer types: remember to deactivate Query Monitor while testing this, it sets an error handler that may trigger a few minutes confusion.

#30 @hellofromTonya
3 years ago

  • Milestone changed from 6.0 to Future Release
  • Owner hellofromTonya deleted

With 6.0 RC1 tomorrow, it's too late for this ticket in the 6.0 cycle. As the ticket has been kicked down the road since 5.5.2 and there's still work pending to figure out a solution, moving this to Future Release. Also removing myself as owner.

#31 @peterwilsoncc
2 years ago

#56947 was marked as a duplicate.

#32 @peterwilsoncc
2 years ago

Committers: please give props to @hztyfoon

They added some testing notes to the duplicate ticket, #56947.

#33 @ironprogrammer
2 years ago

Quoting @peterwilsoncc's note as it pertains to reproducing this issue when following the test steps outlined in #56947:

As it depends on the WP_DEBUG & WP_DEBUG_DISPLAY constants in the wp-config.php file, it may not occur in all circumstances.

@mcaskill
23 months ago

Add filter to preempt printing the .php-error CSS class to make PHP errors visible

#34 follow-up: @mcaskill
23 months ago

Example related to 51383.2.diff:

<?php

// Ignore suppressed deprecation notices.
add_filter( 'pre_admin_php_error_class', function ( $print, $error ) {
        return ( $print && E_DEPRECATED !== $error['type'] )
}, 10, 2 );

#35 in reply to: ↑ 34 @mikinc860
18 months ago

@mcaskill The last provided solution is working fine and in the newer versions of WordPress the same class indicates to display the PHP errors in a proper way.

This ticket was mentioned in ​Slack in #core-test by oglekler. ​View the logs.
15 months ago

This ticket was mentioned in ​Slack in #core by oglekler. ​View the logs.
4 months ago

#38 @desrosj
4 months ago

#59751 was marked as a duplicate.

This ticket was mentioned in ​PR #7256 on ​WordPress/wordpress-develop by ​@akirk.
3 months ago #39

  • Keywords has-unit-tests added; needs-refresh removed

In PHP 8.1, the admin_header will gets the php-error CSS class added for the suppressed notice for @ini_set( 'auto_detect_line_endings', 1 );

This adds code so that we can do without the ini setting and still handle old-style \r line-terminated files. This also adds a unit test for this.

Trac ticket: https://core.trac.wordpress.org/ticket/51383

Note: See TracTickets for help on using tickets.