Reclassifying engine warnings View as Markdown

Introduction

While newly introduced error conditions in the engine typically use Error exceptions, we have many old error conditions that use inappropriate severity levels for historical reasons. For example, accessing an undefined variable, while being a very severe programming error, only generates a notice. This RFC proposes to reevaluate existing error conditions and reclassify their severity level as appropriate.

Proposal

General Guidelines

As we don't have any existing rules on the matter, here are some general guidelines I try to follow in the following reclassification:

• Error exceptions should be the baseline for error conditions that indicate a programming error.
• If there is an expectation that a certain error condition is commonly intentionally suppressed, especially in legacy code, an exception should not be used.
• If the error condition is data-dependent, it may be preferable not to use an exception.
• For error conditions that have known false positives, a notice should be used.
• Avoid promoting from notice directly to Error exception. I'm only proposing this for the case of undefined variables, because it is so severely misclassified right now.

Proposed Classification

The following table contains a list of errors with notice or warning severity generated in the engine, excluding warnings that are generated by functions which have an optimized opcode implementation.

The table shows both the current error level, as well as the proposed level. A rationale for the proposed change (or non-change) is provided below each group of errors.

The “undefined variable”, “undefined array index” and “division by zero” error conditions are discussed separately below, because they are more controversial.

Undefined variable

In most cases, accessing an undefined variable is a severe programming error. The current low classification is a legacy from the Dark Ages of PHP, where features like register_globals made conditionally defined variables more typical, and code quality standards were lower.

Ideally, undefined variables should be compile errors, but as the dynamic nature of PHP precludes a reliable compile-time analysis, this RFC proposes to generate an Error exception instead.

However, throwing an exception may complicate the upgrading of legacy code that currently suppresses the generation of notices wholesale, as the issue can no longer be ignored. Some people have even suggested that the use of undefined variables is a legitimate coding style choice.

For this reason, a separate vote will decide whether we should throw an Error exception, generate a warning or keep the current notice.

Undefined array index

Similarly to undefined variables or an undefined object properties, reading an undefined array index/key would generally be considered a programming error in modern PHP code. However, while variables and object properties are predominantly statically known (i.e., when variable variables and dynamic object properties are not used), the same is not true to array keys, which tend to be dynamic.

Some languages, such as JavaScript, do not consider accesses to undefined array keys to be an error condition at all, and allow such an operation to be performed silently. While it is not predominant in the PHP world, some people subscribe to such a coding style also for PHP code, and as such would prefer undefined array key access to remain an easily suppressible notice.

A separate vote will decide whether to elevate undefined array offset/index conditions to a warning, or leave them as notices.

Division by zero

Division by zero currently has somewhat inconsistent behavior. The % operator throws a DivisionByZeroError. However, the / throws a “Division by zero” warning and returns one of +Inf, -Inf or NaN, following IEEE 754 semantics.

The rationale for this behavioral discrepancy is as follows: The % operator works on integers. As such the conjugated operation is actually not /, but rather intdiv(), which does throw DivisionByZeroError. An integer operation shouldn't return a floating point number, so throwing an Error exception is the only choice here.

Not throwing an Error exception for division by zero using / is motivated by the fact that such division does have a well-defined result under IEEE 754. In some areas of application (such as numerics) it may be useful to not treat division by zero as an error condition at all, though such applications are unusual for PHP. Similarly, Bob Weinand argued that for reporting code that makes heavy use of divisions, it may be preferable to have the ability to suppress this error condition.

On the other hand, the current behavior, and especially the discrepancy with % is quite unexpected, and many people expect that a division by zero error will in fact generate a DivisionByZeroError (duh). This discussion has already come up multiple times on the internals list and in pull requests.

As I think that both sides have a reasonable argument here, there will be a separate vote on whether to change the division by zero behavior.

Backward Incompatible Changes

Conversion of notices to warnings is fairly harmless, because both continue execution after the diagnostic has been generated. Conversion to exceptions implies that the current control flow will be aborted.

This may impact code that makes very liberal use of the error suppression operator @ or disables error reporting wholesale. The proposal does try to avoid changing notices that are more likely to be suppressed into exceptions.

If desired, an error handler can be provided that filters out the error conditions that will be turned into an exception, so that projects can focus on addressing them prior to an upgrade.

Vote

All the following votes are independent. Each requires a 2/3 majority and may pass/fail independent of other votes. Voting closes 2019-09-26.

As the “undefined variable” vote is a 3-way vote, acceptance is determined as follows: “Error exception” is accepted if it has 2/3 majority. Otherwise, “Warning” is accepted if the first two options together have 2/3 majority. Otherwise, “Keep Notice” applies.

Vote: Change undefined variable severity to? — Error exception: 36 · Warning: 18 · Keep Notice: 10 (closed)

.

Vote: Change undefined array index severity to? — Warning: 42 · Keep Notice: 21 (closed)

.

Vote: Change division by zero severity to? — DivisionByZeroError exception: 52 · Keep Warning: 8 (closed)

.

Vote: Accept remaining classifications shown in the table above? — Yes: 54 · No: 3 (closed)

Changelog

• 2019-09-12: Split out undefined index/offset into a separate section.
• 2019-09-10: Keep current classification for reference errors.
• 2019-08-28: Split off the “undefined variable” case into a separate vote, as it was a major point of contention on-list.