FIX: Handle E_RECOVERABLE_ERROR (fixes #2928)

[kinglozzer]

See http://www.php.net/manual/en/errorfunc.constants.php

Catchable fatal error. It indicates that a probably dangerous error occurred, but did not leave the Engine in an unstable state. If the error is not caught by a user defined handle, the application aborts as it was an E_ERROR.

[ajshort]

Maybe it would be good to add a default case which returns a fatal handler? That way we eliminate the risk of silently hiding errors.

[kinglozzer]

Yep, definitely think we need a default case for this. Are you happy for me to add that to this pull request or would a separate one be preferred?

[ajshort]

I reckon do it in this one, may as well fully solve it. You can probably just move the fatal error handler to the end of the switch, get rid of all the cases for it, and make it the default. I think that would be the best fix.

[ajshort]

The only concern I have is that this might be an upgrading issue, so it might be best to go into master. Any thoughts on that @chillu or @simonwelsh?

[simonwelsh]

This is a massive upgrading issue. The amount of places where I've seen code, even in the CMS, that this will break is staggering. I feel like it should be a non-fatal warning in 3.2 (like an E_DEPRECATED) and then changed into a fatal for 4.

[kinglozzer]

@simonwelsh Sure, I’m fine with that - it’s an improvement on the errors not showing at all! So:

• E_RECOVERABLE becomes a warning for now
• Remove the fatal error cases & make fatal error handling the default for the switch statement to prevent this sort of issue reoccurring

Are you happy with these two changes being made against master?

[simonwelsh]

I'd just add default to the list of fatal cases. I like making things like this explicit, for readability, rather than just relying on default behaviour.

[ajshort]

Fair enough. In any case though, I think we should add a default case that throws an exception or something so this doesn't happen again.

I agree that moving to a non-fatal warning in the master branch is the way to go.

[kinglozzer]

Just to outline this again:

• Add E_RECOVERABLE_ERROR to the warning cases
• Add ‘default’ to the existing fatal cases

I think I’ll also re-order the cases (ascending in severity) so the fatal group comes last, as that’s where you’d expect the default to be. All sound okay?

[simonwelsh]

Fine by me. Go for it!

[kinglozzer]

Re-opened in #2955. Also added a default/unknown error to DebugView or the error description is ‘[]’ if the code is unknown.