WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 weeks ago

#36561 new enhancement

Deprecated notices should be classified as such.

Reported by: jrf Owned by:
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch 2nd-opinion
Focuses: coding-standards Cc:
PR Number:

Description

All errors triggers should be classified with the appropriate error level.

Most notably, the _deprecated_function()/_deprecated_constructor()/_deprecated_file()/_deprecated_argument()/_doing_it_wrong() function do not pass an appropriate error level to trigger_error().

For the deprecated function group, the most appropriate level seems to be E_USER_DEPRECATED which was introduced in PHP 5.3.0.

For _doing_it_wrong() an E_USER_NOTICE (or E_USER_WARNING) seems more appropriate.

Fixed in the accompanying patch.

For backward compatibility with PHP 5.2, a define for E_USER_DEPRECATED has been added to wp-includes/compat.php which follows the same logic as used in SimplePie for consistency:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-simplepie.php#L699

Attachments (4)

36561-classify-trigger_error-calls.patch (8.5 KB) - added by jrf 4 years ago.
36561-2-classify-trigger_error-calls.patch (9.9 KB) - added by jrf 3 years ago.
Added @since changelog tags
36561-3-classify-trigger_error-calls.patch (11.7 KB) - added by jrf 2 years ago.
Updated the patch + applied the same logic to the _deprecated_hook() function
36561-4-classify-trigger_error-calls.patch (13.5 KB) - added by jrf 4 weeks ago.
Refreshed patch: removed PHP 5.3 compat changes; updated the version mentioned in the @since entries; fixed up the trigger_error() function calls to multi-line for improved readability.

Download all attachments as: .zip

Change History (20)

#1 @DrewAPicture
3 years ago

  • Keywords has-patch needs-docs added

Interesting. @jrf: Thanks for the patch.

I think we're going to need changelog entries added for the change in behavior when calling trigger_error() in these functions.

@jrf
3 years ago

Added @since changelog tags

#2 @jrf
3 years ago

I think we're going to need changelog entries added for the change in behavior when calling trigger_error() in these functions.

@DrewAPicture Good point. Added for the deprecated functions. Not for the _doing_it_wrong() function as in effect the behaviour of that function does not change.
To be precise: no classification defaults to E_USER_NOTICE and the new classification is the same.
See: http://php.net/manual/en/function.trigger-error.php

For whomever is interested:
I've put in some work on - and will be sending in patches for - the Debug Bar plugin which will fix a number of issues with how the plugin logs and presents the error notices to devs.
This patch will not cause any conflicts in that respect and was in actual fact the result of my debugging efforts on the Debug Bar plugin.

For more info on that:
https://wordpress.org/support/topic/where-does-development-of-this-plugin-take-place
https://github.com/dd32/debug-bar/pulls and specifically: https://github.com/dd32/debug-bar/pull/17 and even more specifically: https://github.com/dd32/debug-bar/pull/17/commits/a4f5c64a742d8e44bd7d200d5f353d396210a3ee

#3 @jrf
3 years ago

  • Keywords needs-docs removed

#4 @jrf
3 years ago

What needs to be done to move this ticket forward ? (aside from rebasing it) @DrewAPicture ?

#5 @DrewAPicture
3 years ago

  • Keywords 4.8-early added

@jrf I apologize for the delay. Unfortunately at this point, we have once again missed the boat in getting this in pre-beta. So let's mark it for 4.8-early and be vigilant about jumping on it first thing.

#6 @jrf
2 years ago

As 4.8 is out, shall we try & get this in early for 4.9 ?

#7 @swissspidy
2 years ago

  • Keywords 4.8-early removed
  • Milestone changed from Awaiting Review to 4.9

I'm not opposed to doing so.

@jrf
2 years ago

Updated the patch + applied the same logic to the _deprecated_hook() function

#8 @jrf
2 years ago

@swissspidy Thanks for your response. I've updated the patch. Should be good to go now.

#9 @jrf
2 years ago

Oh and some love for the hook deprecation path in https://core.trac.wordpress.org/ticket/10441 might also be nice for 4.9 ;-)

#10 @jrf
2 years ago

Just realized a minor issue with this patch. There are a couple of places in core where error_reporting() is called like so:

<?php
error_reporting( E_CORE_ERROR | E_CORE_WARNING | E_COMPILE_ERROR | E_ERROR | E_WARNING | E_PARSE | E_USER_ERROR | E_USER_WARNING | E_RECOVERABLE_ERROR );

These calls do not include the PHP 5.3 E_DEPRECATED and E_USER_DEPRECATED and at least two of these calls are made before the wp-includes/compat.php file is loaded.

These are the places where I found the troublesome error_reporting() calls:

Anyone got any bright ideas how to handle this ? Or should the backfill define(s) for the PHP 5.3 error constants just be moved to the top of the wp-load.php file and the above four calls adjusted to include them ?

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


2 years ago

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


2 years ago

#13 @johnbillion
2 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from 4.9 to Future Release

I believe that the current error levels for deprecated calls, wrongdoings, etc is intentional. Note that the default error level used by error_log() is E_NOTICE.

We can't upgrade anything to an E_WARNING because they can become visible on the front end of a poorly configured server (eg one that has WP_DEBUG correctly set to false but which has display_errors enabled, and it'll also cause logs to fill up if they're configured to only log warnings.

If we downgrade the E_NOTICE triggered by deprecated calls to E_DEPRECATED, I'm wary of this change causing deprecated usage to go unnoticed when a server is configured to log notices but not deprecated notices. Is this a common configuration?

#14 @jrf
2 years ago

@johnbillion

I believe that the current error levels for deprecated calls, wrongdoings, etc is intentional. Note that the default error level used by error_log() is E_NOTICE.

I don't necessarily agree.
I think it's more a choice of convenience as E_(USER_)DEPRECATED only became available in PHP 5.3.

I also think that being explicit about the error level used would be better than relying on a silent default level.

In the above proposal and the associated patches:

  • For doing it wrong, the error level doesn't change, it's just being made explicit.
  • For deprecated notices, the error level does change to the more appropriate E_USER_DEPRECATED.

We can't upgrade anything to an E_WARNING ...

I'm not proposing to upgrade to E_WARNING anywhere as that would not be an appropriate choice anyway.

... because they can become visible on the front end of a poorly configured server (eg one that has WP_DEBUG correctly set to false but which has display_errors enabled, and it'll also cause logs to fill up if they're configured to only log warnings.

That has nothing to do with the error level chosen and is actually incorrect.

If display_errors is set to 1 (enabled) and WP_DEBUG is set to false, error will NOT display on the screen. Those messages will also NOT go into the error log either.

Both display_errors as well as log_errors respect the error_reporting setting and will not act on errors not covered by that.

Setting WP_DEBUG to false effectively sets error_reporting to E_CORE_ERROR | E_CORE_WARNING | E_COMPILE_ERROR | E_ERROR | E_WARNING | E_PARSE | E_USER_ERROR | E_USER_WARNING | E_RECOVERABLE_ERROR which doesn't include E_NOTICE or E_DEPRECATED

The PHP defaults are:
display_errors: 1 (i.e. display)
error_reporting: As stated in the PHP manual:

PHP 5.3 or later, the default value is E_ALL & ~E_NOTICE & ~E_STRICT & ~E_DEPRECATED. This setting does not show E_NOTICE, E_STRICT and E_DEPRECATED level errors. You may want to show them during development. Prior to PHP 5.3.0, the default value is E_ALL & ~E_NOTICE & ~E_STRICT. In PHP 4 the default value is E_ALL & ~E_NOTICE.

Ref: http://php.net/manual/en/errorfunc.configuration.php#ini.error-reporting

If we downgrade the E_NOTICE triggered by deprecated calls to E_DEPRECATED, I'm wary of this change causing deprecated usage to go unnoticed when a server is configured to log notices but not deprecated notices. Is this a common configuration?

I think the most common configuration is the default as quoted above which doesn't include notices nor deprecation errors, so effectively this change would not make a difference for the default case.

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


9 months ago

#16 @jrf
4 weeks ago

  • Focuses coding-standards added
  • Milestone changed from Future Release to 5.4

I would like to see if we can move this ticket forward. Probably too late for WP 5.3, but WP 5.4 early would be a good target IMO.

As WP 5.3 is no longer supported, we no longer need to be concerned over whether or not the PHP native E_USER_DEPRECATED (PHP 5.3) is available.

I will be uploading an updated patch momentarily.

@jrf
4 weeks ago

Refreshed patch: removed PHP 5.3 compat changes; updated the version mentioned in the @since entries; fixed up the trigger_error() function calls to multi-line for improved readability.

Note: See TracTickets for help on using tickets.