Recommend turning errors into exceptions?

[NotionCommotion]

I recently had a need to check several arrays, and if an index wasn't set, set a variable to a given value.  The code was such that I couldn't perform the isset check all at once.  I then thought "wouldn't it be great if I could try the script, and catch the error or warning". Searching a bit, I came across the following script.  I am a little nervous, however, that there might be negative consequences of doing so.  Thoughts?

set_error_handler(function($errno, $errstr, $errfile, $errline, array $errcontext) {
// error was suppressed with the @-operator
if (0 === error_reporting()) {
return false;
}

throw new ErrorException($errstr, 0, $errno, $errfile, $errline);
});

try {
dns_get_record();
} catch (ErrorException $e) {
// ...
}

[kicken]

I wouldn't do it permanently for the whole script. If you want to change it for the duration of a particular function or something maybe it'd be ok.

 

I can't think of any reason why you couldn't just fix the code to properly check everything with isset() though and prevent the error in the first place. It can be an annoying amount of extra typing sometimes but such is life.

[NotionCommotion]

I wouldn't do it permanently for the whole script.

 

Why not?  I am sure you are right, but I can't think of a good reason not to and it does add some flexibility.

[requinix]

"Fatal" errors, yes (the only one you can really convert is E_USER_ERROR), warnings, maybe, but I wouldn't do it for notices, strict, or deprecation errors. Those aren't so severe that I want to drop what I'm doing to fix them, and more importantly I don't want my live site to crash because a rare codepath triggered an undefined index notice that I didn't encounter while testing. All that would teach you to do is try/catch everything and that's definitely worse than letting some minor issues get past you.

 

Until PHP has exceptions in the engine, and specialized exceptions at that, I wouldn't rely on ErrorException. It's helpful to generate backtraces automatically but I don't throw them.

There's a fair amount of irrelevant stuff but

<?php

namespace Kernel {

	/**
	 * @property-read \AssertionException $assertion
	 */
	class AssertionEvent extends Event { }

	/**
	 * @property-read \Exception $error
	 */
	class ErrorEvent extends Event { }

	final class ErrorHandler {

		private static $top = null;

		private $callback = null;
		private $mask = 0;
		private $previous = null;

		/**
		 * @param callable|bool $callback
		 * @param int $mask
		 */
		private function __construct($callback, $mask = null) {
			$this->callback = $callback;
			$this->mask = ($mask ? (int)$mask : -1);
		}

		/**
		 * Handle an assertion
		 *
		 * @param string $file
		 * @param int $line
		 * @param string $code
		 * @param string $description
		 */
		public static function assertion($file, $line, $code = "", $description = null) {
			$e = new \AssertionException($code, "Assertion" . ($description ? " '{$description}'" : $code) . " failed", $file, $line);
			event(new AssertionEvent(["assertion" => $e]));
			self::bubble($e, E_USER_ERROR) || trigger_error("{$e->getMessage()} in {$file} on line {$line}, unhandled", E_USER_ERROR);
		}

		/**
		 * Bubble an exception through the handlers
		 *
		 * @param \Exception $e
		 * @param int $severity
		 * @return bool
		 */
		private static function bubble(\Exception $e, $severity) {
			for ($handler = self::$top; $handler; $handler = $handler->previous) {
				if (!($handler->mask & $severity)) {
					continue;
				}
				if (is_callable($handler->callback) ? call_user_func($handler->callback, $e, $severity) : $handler->callback) {
					return true;
				}
			}
			return false;
		}

		/**
		 * Handle an error
		 *
		 * @param int $errno
		 * @param string $errstr
		 * @param string $errfile
		 * @param int $errline
		 * @return bool
		 */
		public static function error($errno, $errstr, $errfile, $errline) {
			if (error_reporting() == 0) {
				$e = new \ErrorException($errstr, 0, 0, $errfile, $errline);
				notice("kernel.error", new \ErrorException("Suppressed error: {$errstr}", 0, $errno, $errfile, $errline, $e));
				return true;
			} else {
				$e = new \ErrorException($errstr, 0, $errno, $errfile, $errline);
				event(new ErrorEvent(["error" => $e]));
				return self::bubble($e, $errno);
			}
		}

		/**
		 * Handle an uncaught exception
		 *
		 * @param \Exception $e
		 */
		public static function exception(\Exception $e) {
			event(new ErrorEvent(["error" => $e]));
			self::bubble($e, E_USER_ERROR);
		}

		/**
		 * Pop the most recent error handler off of the stack
		 */
		public static function pop() {
			self::$top && self::$top = self::$top->previous;
		}

		/**
		 * Push a new error handler onto the stack
		 *
		 * function $callback(\Exception $e, $severity) returns bool
		 *
		 * @param callable|bool $callback
		 * @param int $mask
		 * @return \Kernel\ErrorHandler
		 */
		public static function push($callback, $mask = null) {
			debug("kernel.error", [
				"Adding %serror handler for %s",
				is_bool($callback) ? ($callback ? "true " : "false ") : "",
				($mask ? "mask " . (int)$mask : "all errors")
			]);
			$top = new self($callback, $mask);
			$top->previous = self::$top;
			return self::$top = $top;
		}

	}

}

namespace {

	/**
	 * Return a single frame from a backtrace
	 *
	 * The default $frame = 0 will give the stack frame for the code calling your function.
	 *
	 * @param int $frame
	 * @param int $options
	 * @return array
	 */
	function debug_backtrace_frame($frame = 0, $options = null) {
		is_null($options) && $options = DEBUG_BACKTRACE_PROVIDE_OBJECT;

		foreach (debug_backtrace($options) as $stackframe) {
			$lastframe = $stackframe;
			if (isset($stackframe["file"], $stackframe["line"])) {
				if ($frame-- < 0) {
					break;
				}
			}
		}
		return $lastframe;
	}

	final class AssertionException extends Exception {

		protected $description = "";

		/**
		 * @param string $description
		 * @param string $message
		 * @param string $file
		 * @param int $line
		 * @param \Exception $previous
		 */
		public function __construct($description, $message = "", $file = null, $line = null, Exception $previous = null) {
			parent::__construct($message, 0, $previous);
			$this->description = $description;

			if ($file === null || $line === null) {
				$frame = debug_backtrace_frame(0, 0);
				$file || $file = $frame["file"];
				$line || $line = $frame["line"];
			}
			$this->file = $file;
			$this->line = $line;
		}

		/**
		 * Get the code which triggered the exception
		 *
		 * @return string
		 */
		public function getDescription() {
			return $this->description;
		}

	}

	assert_options(ASSERT_CALLBACK, "\\Kernel\\ErrorHandler::assertion");
	assert_options(ASSERT_WARNING, false);
	set_error_handler("\\Kernel\\ErrorHandler::error");
	set_exception_handler("\\Kernel\\ErrorHandler::exception");

	listen("__applyconf", function() {
		foreach (cfg()->node("error/handlers") as $handler) {
			\Kernel\ErrorHandler::push($handler->handler, $handler->mask);
		}
	});

}

[Jacques1]

Exceptions are not meant to make programming more convenient for you or save you from writing down isset() checks. This is abuse.

 

The purpose of exceptions is to handle errors. You throw them when a (serious) problem occurs. If you're using them for anything else, then you're generally doing it wrong.

 

So, no, this is not a valid approach. And yes, it has huge negative effects, because now all notices, warnings etc. trigger an exception. In the worst case, you'll even catch an entirely unrelated error and assume that it's just a missing index again. That's not good.

 

On top of this, the whole construct is incredibly confusing. I mean, just imagine yourself trying to explain it to a fellow programmer: “Look, I actually just want to do an isset() check. But since I couldn't do it, I've created a global error handler which turns all notices into exceptions. So this piece of code (as well as all other code) will now throw an exception if there's a missing index, and you have to catch that exception and put the logic into the catch block.”

 

WTF?

 

What's the problem with isset(), anyway? Shouldn't we talk about that rather than come up with weird hacks?

Edited January 13, 2015 by Jacques1

[NotionCommotion]

WTF?

 

Convenience is not always bad.  I would be throwing an exception when a serious error occurs which doesn't natively throw an exception.  The exception handler could be designed to deal with different errors appropriately.

 

That being said, is it a kludge?  Well, I suppose it is, and won't be going down this path.  Thank you for calling a spade a spade.

[requinix]

Exceptions should be thrown when the code cannot possibly continue what it is doing and needs to drastically fail. Except for maybe E_USER_ERROR like I said earlier, your error handler can't know whether a warning (or notice or whatever) is so bad that the code raising it cannot continue. In fact, it's more likely that such problems are not fatal because if they were then they wouldn't be mere warnings.

 

That means the error handler should not throw exceptions. Log them, spit them out into your HTML, ignore them, whatever, but don't kill your code over them.

[NotionCommotion]

Exceptions should be thrown when the code cannot possibly continue what it is doing and needs to drastically fail. Except for maybe E_USER_ERROR like I said earlier, your error handler can't know whether a warning (or notice or whatever) is so bad that the code raising it cannot continue. In fact, it's more likely that such problems are not fatal because if they were then they wouldn't be mere warnings.

 

That means the error handler should not throw exceptions. Log them, spit them out into your HTML, ignore them, whatever, but don't kill your code over them.

 

Hey Requinx,  Maybe I still need a bit more...

 

As I see it, there are three (and probable more) general type of errors

1. Syntax errors, compile errors, or what ever they should be called.  For example, unbalanced quotes such as echo(bla'), unexpect text such as bla bla bla, etc.
2. Errors thrown by something other than a class.  For example, $x=someNonExistingFunction(123);, class foo extends notExistingClass bar {}, require('non_existing_file.bla');
3. Errors (or exceptions) thrown by a class.  For example, $stmt=$conn->prepare('SELECT invalidColumn FROM myTable WHERE x=? AND y=?');,  $stmt->execute('onlyOneArrayElement');, etc.

Why should exceptions be dealt with more harshly, and errors be allowed to continue without killing the code?

[requinix]

1. Syntax errors, compile errors, or what ever they should be called.  For example, unbalanced quotes such as echo(bla'), unexpect text such as bla bla bla, etc.

Can't catch those. You can often react after-the-fact but that's a different discussion.

 

2. Errors thrown by something other than a class.  For example, $x=someNonExistingFunction(123);, class foo extends notExistingClass bar {}, require('non_existing_file.bla');

All fatal and uncatchable.

 

3. Errors (or exceptions) thrown by a class.  For example, $stmt=$conn->prepare('SELECT invalidColumn FROM myTable WHERE x=? AND y=?');,  $stmt->execute('onlyOneArrayElement');, etc.

If you're talking about PDOException, that has nothing to do with an error handler.

Or PDO may return a silent false on error. No PHP warnings.

 

For other functions, say fopen() on an unreadable file, or PDO problems with ERRMODE_WARNING, theoretically you could use an error handler to convert that into a FileNotFoundException (or whatever) but that would be impractical as it requires parsing the assorted error messages. Which is not nearly worth the effort.

A generic ErrorException is the only option, but it's worthless in normal use: throwing it implies you try to catch it somewhere up the call stack, but you have no way to differentiate between an unreadable file or one of the thousands of other potential warnings PHP can raise. Thus catching it means you're using the "catch everything" approach to exceptions, which is a very bad practice to get into.

 

With the current state of PHP, warnings are worth more as warnings than as exceptions.

 

Why should exceptions be dealt with more harshly, and errors be allowed to continue without killing the code?

An exception and an error are basically of the same status: a fatal error of some sort. That may be totally fatal like your script stops entirely, or that may be "relatively fatal" in that the code that was executing at the time has to stop but other code may be able to continue. For example, a FileNotFoundException thrown when opening a file is relatively fatal because the file could not be opened - the whole point of the "opening a file" code - but calling code may catch the exception and deal with the problem. Like present a message to the user.

 

PHP doesn't have a "relatively fatal" error, so it uses warnings and false/null/error codes instead.

Edited January 14, 2015 by requinix

[Jacques1]

Nobody said that you should take warnings less seriously than exceptions – at least I didn't.

 

In fact, a professional developer should take warnings as seriously as any other error. While it's true that they sometimes just indicate poor coding style (which is bad enough), they may very well point to a serious bug.

 

So a zero-tolerance approach towards errors is actually a good idea, because it reduces bugs and enforces basic quality. A lot of languages already work like this. For example, if you try to access a nonexistent index in Java, the application will throw an exception, and there's no such thing as turning off error reporting.

 

What you have to consider, though, is that PHP was designed as a toy language for non-programmers who just wanted to get their homepage up and running and didn't care about the underlying code. That's how it is. So if you reject this approach altogether (which is understandable), it probably makes more sense to choose a different language than to try and turn PHP into something else.