Safe methods should also catch `\ErrorException` and not only check for error conditions

[nagmat84]

Summary

In the nearer past, many functions have been changed to throw an \ErrorException which previously used to generate emit a PHP engine error on failure. This is also supported by the big PHP frameworks such as Symfony and Laravel which install a global error handler and then throw an \ErrorException.

IMHO, all the safe functions need not only to check, if the wrapped result indicates an error condition but also need to catch \ErrorException and wrap this into a more specific exception.

Example

As we also must check for an \ErrorException, I usually write code like that

try {
   $stream = \Safe\fopen('/my/path', r+b);
} catch(\Safe\Exceptions\FilesystemException|\ErrorException $e) {
  // do something
}

This is in particular annoying for methods which shall simply re-throw a particular exception. Then I end up with something like this

/**
 * @param string $path
 * @returns resource
 * @throws \Safe\Exceptions\FilesystemException
 */
function myMethod(string $path) {
  try {
    return \Safe\fopen('/my/path', r+b);
  } catch(\ErrorException $e) {
    throw new FilesystemException($e->getMessage(), $e, $e->getCode());
  }
}

I assume the best way to deal with this kind of problem is to catch these \ErrorException already in the safe function itself, for example \Safe\fopen would become

function fopen(string $filename, string $mode, bool $use_include_path = false, $context = null) {
  try {
    error_clear_last();
    if ($context !== null) {
        $result = \fopen($filename, $mode, $use_include_path, $context);
    } else {
        $result = \fopen($filename, $mode, $use_include_path);
    }
    if ($result === false) {
        throw FilesystemException::createFromPhpError();
    }
    return $result;
  } catch(\ErrorException $e) {
    throw new FilesystemException::createFromPhpErrorException($e);
  }
}

Of course, this has to be solved on a "global" scale.

[Kharhamel]

Are you sure this is needed? The doc page for fopen doesn't say anything about an ErrorException.

More generally, I believe that a function either throw an ErrorException or return false/null on error, not both. So safe functions should never throw ErrorException in the first place. Do you have an example of an function that can do both?

[nagmat84]

Yes, it is needed.

In short: The PHP documentation is incomplete in this respect.

The long version: The PHP documentation does not state that these functions throw an exception, because technically it is not an integral part of the function definition, but the global error handler which throws the exception on behalf of the method. With modern PHP the following happens: The method emits a (traditional) engine error of different severity (i.e. notice, warning, error, fatal) and returns false. However, the emitted error is caught by the global error handler. In older versions of PHP this error handler was a no-op unless a user or framework registered a custom error handler. However, with modern PHP (with the beginning of PHP7.x) the default error handler converts the engine error above a certain severity level into a proper ErrrorException and throws that. From the viewpoint of the caller this looks as if the method itself had thrown the ErrorException. So one could say that PHP is slowly moving towards something which resembles the safe-methods. Unfortunately, there is still a gap, where the function return false but the global error handler does not trigger, because the severity is below the treshold for throwing exceptions. So the safe-methods are still needed.

I will look up some of the PHP RFC when I am back at a proper PC.

I encountered this problem with fopen. That's why I specifically mentioned this method in the title. I will provide a MWE, when I am back at a PC. But I believe the error condition was simple: Try to fopen a non-existing file for write-read access with mode w+ in a directory without write access. In that case PHP 8 fopen throws an ErrorException and does not return false.

[Kharhamel]

Ok, so if I understand correctly, the ErrorException behavior is actually documented by this section (from fopen doc page):

Errors/Exceptions[ ¶](https://www.php.net/manual/en/function.fopen.php#refsect1-function.fopen-errors)

Upon failure, an E_WARNING is emitted.

In order to do what you ask, I need a reliable way to tell from the doc page if a function can throw an ErrorException. Is every E_WARNING converted into an ErrorException? Do the words used change much from a page to another? Is something else that can cause an ErrorException to be thrown?

If yes, this should be easy. If not, we need to make a list of theses functions and try to find some kind of way to identify them.

Or I guess I could just add the try{}catch{} in every functions but it will add some overhead. No idea how much.

[moufmouf]

For of all, the Safe exceptions extend the ErrorException.

So hopefully, you can do something like:

try {
   $stream = \Safe\fopen('/my/path', r+b);
} catch(\ErrorException $e) {
  // do something
}

and this will catch both exceptions (Safe and "standard" exceptions)

Now, it would indeed be cool to cast those exceptions as better typed "Safe" exceptions.

I see a number of pitfalls that make this not so trivial.

Right now, the fopen function throws an ErrorException. So we could write something like:

try {
    $return = \fopen($file, $mode);
} catch (\ErrorException $e) {
    throw new FileException($e->getMessage(), $e->getCode(), $e);
}

But that would not be 100% future-proof.

Let's assume in PHP 8.3, they start throwing better typed exceptions (like FileNotFoundException and AccessDeniedException). Our code will catch those errors and will rethrow an error that is actually less correctly typed.

So we will start racing against PHP to wrap all the exceptions they might define in the future.

A possible alternative could be to catch only ErrorException (and not the subtypes) like this:

try {
    $return = \fopen($file, $mode);
} catch (\ErrorException $e) {
    if (get_class($e) === '\\ErrorException') {
        throw new FileException($e->getMessage(), $e->getCode(), $e);
    } else {
        throw $e;
    }
}

That could work, but when upgrading my code from PHP 8.2 to the hypothetical 8.3, code like

try {
    Safe\fopen(...)
} catch (Safe\FileException $e) {
    // Catch file not found exceptions
}

would suddenly stop working. And finding the source of these issues will be a nightmare for developers as they are exceptions that are usually not that easy to reproduce.

So basically, I understand the request and find it legitimate, but I can't wrap my head around finding a way to make it "future proof" (assuming PHP will go in the right direction and type those exceptions some day)

[nagmat84]

For of all, the Safe exceptions extend the ErrorException.

So hopefully, you can do something like:

try {
   $stream = \Safe\fopen('/my/path', r+b);
} catch(\ErrorException $e) {
  // do something
}

and this will catch both exceptions (Safe and "standard" exceptions)

Yes, that is what I have found out in the meantime, too, and what I am doing right now.

Now, it would indeed be cool to cast those exceptions as better typed "Safe" exceptions. I see a number of pitfalls that make this not so trivial. [...] So basically, I understand the request and find it legitimate, but I can't wrap my head around finding a way to make it "future proof" (assuming PHP will go in the right direction and type those exceptions some day)

Totally agreed with everything.

Another candidate in into which I bumped earlier today, is preg-match() which also exists in a "safe" variant. If you look closely at the official doc and if you are aware of the backgrounds, then you may notice that the caption at the middle of the page (before the user contributions) is titled "Errors/Exceptions" and the text below the caption says

"If the regex pattern passed does not compile to a valid regex, an E_WARNING is emitted."

If you are one of those-in-the-know, it tells you everything. Unfortunately, the official documentation is not very reliable in respect to that for other methods.

There seems to be an ongoing change in PHP. Here is one of those RFC which changed some of the behavior in version 8: https://wiki.php.net/rfc/engine_warnings

Maybe this would be the best option:

I am fine with closing this issue, because honestly, I don't know an approach which is consistent, reliable and future-proof either. However, maybe there should be some kind of warning or advice in the docs of this project that matters are changing with PHP right now and the recommended use is as illustrated by @moumouf above.

[shalvah]

Hey, not a Safe user, but popping in here with a few thoughts because I like the library and I've recently dealt with similar issues.

First off @nagmat84 while I generally agree with you about PHP's changing error behaviour, I still couldn't reproduce this specific case on PHP 8.1:

try {
  fopen('C:/Windows/System32/ext', 'w+');
} catch (\Throwable $e) {
    var_dump($e);
}

No exception is raised, only a warning logged (twice):

PHP Warning:  fopen(C:/Windows/System32/ext): Failed to open stream: Permission denied in C:\Users\shalvah\test.php on line 4

Warning: fopen(C:/Windows/System32/ext): Failed to open stream: Permission denied in C:\Users\shalvah\test.php on line 4

Secondly, I suspect you may be mixing up the class names. From what I've been able to gather, ErrorException was designed for users to wrap an error/warning/notice so it can be caught by an exception handler. Even the example in the docs is along these lines. The most likely users of ErrorException are probably framework authors who attach set_error_handler listeners that do this.

I suspect the class you're thinking of is Error, which was (also) introduced in PHP 7 but originally proposed as EngineException. Error is meant for internal use, to replace fatal errors. (Also see the error hierarchy.)

But maybe I'm wrong, and fopen() really does throw an ErrorException (which complicates things).

Finally, I think Safe can actually ignore this. If Safe's job is to interpret return values and convert to exceptions, it's doing that already. Before Safe, you'd write this:

try {
  operation();
} catch (\Throwable $e) {
  echo "Failed!";
}

But the operation could fail by returning false, and you'd never know. Now you can write:

try {
  Safe\operation();
} catch (\Throwable $e) {
  echo "Failed!";
}

and be (almost) guaranteed that all possible failures are caught. That's the win, IMO. Safe is removing the need to remember to check return values, not trying to catch all errors.