Page MenuHomePhabricator

Make "undefined index" PHP errors throw a RuntimeException
Closed, ResolvedPublic

Description

See PHI1653. An install ran into an issue which incidentally was made more complicated because Undefined index: ... PHP runtime errors are not currently converted to exceptions. This is inconsistent with how most other types of similar error are converted.

A relatively surgical fix is to extend the list of notices which convert up:

// Convert uses of undefined variables into exceptions.
if (preg_match('/^Undefined variable: /', $str)) {
  throw new RuntimeException($str);
}

// Convert uses of undefined properties into exceptions.
if (preg_match('/^Undefined property: /', $str)) {
  throw new RuntimeException($str);
}

// Convert undefined constants into exceptions. Usually this means there
// is a missing `$` and the program is horribly broken.
if (preg_match('/^Use of undefined constant /', $str)) {
  throw new RuntimeException($str);
}

However, I suspect we may be able to express this more cleanly and more generally as "all E_NOTICE errors convert to RuntimeException". It's not obvious to me what errors might exist which we'd want to fall through here.

The only cases I can come up with offhand that should fall through are the E_USER cases, where some caller has explicitly invoked trigger_error(). There's still sort of no reason to do this, but the use of trigger_error() instead of throw at least implies some degree of "log and ignore" intent.

(phlog() doesn't trigger a real error, and isn't affected by this.)

Event Timeline

epriestley triaged this task as Normal priority.Mar 6 2020, 5:22 PM
epriestley created this task.
epriestley lowered the priority of this task from Normal to Wishlist.Sun, Mar 22, 7:43 PM

D21044 may resolve this alone, but I suspect there will be at least a little bit of followup work so I'm going to leave this open for the moment.

epriestley closed this task as Resolved.Mon, Apr 6, 2:26 PM

One long-standing warning in a unit test cropped up (D21055) but if this does have far-reaching implications, they don't seem terribly obvious/common.