Reclassifying some PHP functions warning as exceptions

5 years ago by David Negrier — view source

unread

Hey list,

TL;DR:

I'm considering writing an RFC (and a patch) to turn some warning into
exceptions in a number of PHP functions.
I would first like to gather some feedback here.

The long version:

It is the first time I'm posting on internals.
Some of you may already know me. I'm one of the authors of
thecodingmachine/safe, a library that wraps all PHP functions that return
"false" on error in similar functions that will throw on exception on
error: https://github.com/thecodingmachine/safe/

Surprisingly enough, I've had a huge positive feedback with Safe (I reached
1000 stars on Github in a few weeks, so I think I can say this is something
that bothers a lot of people).

Of course, I could simply have written an error handler that would turn
E_WARNING into exceptions but:

For some libs, I wanted to write code that would behave consistently
(whatever the settings of error_reporting)
More than all, having a return type that can be "string|false", or
"object|false" is a true annoyance when you want to write type-safe code
(which is more and more common with the advent of tools like PHPStan or
Psalm).

For instance, "sprintf" can return "string" or "false" (in case the format
is incorrect). If the string passed to "sprintf" is invalid, I would
certainly prefer knowing right now (with an exception) than risking having
the warning sleep under my nose.

Needless to say: a good practice in error handling is "fail loud, fail
fast". A warning at some point will most certainly lead to an error a few
lines later if error handling is not done properly.

If you look at the frameworks out there, I believe all of them are shipping
with an error handler that throws exceptions by default (even on notices).

Also, my team and I are starting to spend a lot of time maintaining Safe.
So I started wondering if rather than spending time maintaining a patch in
user land, we could not instead spend time fixing things in the core.

So here we are, my team and I started playing with php-src. We are not
regular C developers, but we managed to write a small patch to turn
"sprintf" warnings into exceptions.

This is far from ready but the PR is here:
https://github.com/php/php-src/pull/4837

We would be happy to promote more warnings to exceptions as those:

are more predictable
can be more easily caught / handled
enhance the type system by removing the "false" return type

This is going in the same direction as Nikita RFC's regarding reclassifying
"Engine Warnings".

I understand there have been a lot of discussions going on recently
regarding maintaining backward compatibility vs evolving/sanitizing the
language and I know this is a sensitive topic.

That being said:

most frameworks are already providing an error handler that turns warning
into exceptions (so they should not be impacted by the change)
there are a huge number of warnings that can be turned into exceptions
with minimal impact ("sprintf" E_WARNING are clearly a good first candidate)

Ok, so my team and I are willing to put some time to turn some of those
warnings into exceptions. I understand there are some questions to be
answered regarding the nature of the exception (what class, what class
hierarchy...)

My questions:

1- Do you think there is a chance an RFC on this topic might be accepted?
Shall I start working on it?
2- Shall we start working on a list of candidate functions?

David

5 years ago by Dik Takken — view source

unread

I'm considering writing an RFC (and a patch) to turn some warning into
exceptions in a number of PHP functions.
I would first like to gather some feedback here.

I think this is definitely worth a shot. My suggestion would be to split
the effort in sets of functions based on the amount of BC break that
would be involved.

Functions that would require a minor BC break that is unlikely to affect
a lot of code could be bundled in one RFC. The list of functions could
then be discussed and finally voted on. Another RFC could address
functions that are expected to cause bigger BC issues which could be
fixed using an automatic fixer. Yet another RFC for functions that would
require manual fixing of BC issues. Some of these functions might
require an RFC of their own. A step at a time.

Regards,
Dik Takken

5 years ago by Benjamin Morel — view source

unread

I'm considering writing an RFC (and a patch) to turn some warning into
exceptions in a number of PHP functions.
I would first like to gather some feedback here.

I would LOVE if these functions could be fixed, and if I had the almighty
merge button at my disposal, I'd press it whenever such a PR is proposed.
To play the devil's advocate, however, IIRC some discussions in the past
mentioned that the BC breakage that comes with it is such an issue that it
would be preferable to create a brand new API rather than attempting to fix
the existing one.
Nobody ever jumped in to propose such an API, though :-(

— Benjamin

On Mon, 21 Oct 2019 at 15:52, David Negrier d.negrier@thecodingmachine.com
wrote:

Hey list,

TL;DR:

I'm considering writing an RFC (and a patch) to turn some warning into
exceptions in a number of PHP functions.
I would first like to gather some feedback here.

The long version:

It is the first time I'm posting on internals.
Some of you may already know me. I'm one of the authors of
thecodingmachine/safe, a library that wraps all PHP functions that return
"false" on error in similar functions that will throw on exception on
error: https://github.com/thecodingmachine/safe/

Surprisingly enough, I've had a huge positive feedback with Safe (I reached
1000 stars on Github in a few weeks, so I think I can say this is something
that bothers a lot of people).

Of course, I could simply have written an error handler that would turn
E_WARNING into exceptions but:

For some libs, I wanted to write code that would behave consistently
(whatever the settings of error_reporting)

More than all, having a return type that can be "string|false", or
"object|false" is a true annoyance when you want to write type-safe code
(which is more and more common with the advent of tools like PHPStan or
Psalm).

For instance, "sprintf" can return "string" or "false" (in case the format
is incorrect). If the string passed to "sprintf" is invalid, I would
certainly prefer knowing right now (with an exception) than risking having
the warning sleep under my nose.

Needless to say: a good practice in error handling is "fail loud, fail
fast". A warning at some point will most certainly lead to an error a few
lines later if error handling is not done properly.

If you look at the frameworks out there, I believe all of them are shipping
with an error handler that throws exceptions by default (even on notices).

Also, my team and I are starting to spend a lot of time maintaining Safe.
So I started wondering if rather than spending time maintaining a patch in
user land, we could not instead spend time fixing things in the core.

So here we are, my team and I started playing with php-src. We are not
regular C developers, but we managed to write a small patch to turn
"sprintf" warnings into exceptions.

This is far from ready but the PR is here:
https://github.com/php/php-src/pull/4837

We would be happy to promote more warnings to exceptions as those:

are more predictable

can be more easily caught / handled

enhance the type system by removing the "false" return type

This is going in the same direction as Nikita RFC's regarding reclassifying
"Engine Warnings".

I understand there have been a lot of discussions going on recently
regarding maintaining backward compatibility vs evolving/sanitizing the
language and I know this is a sensitive topic.

That being said:

most frameworks are already providing an error handler that turns warning
into exceptions (so they should not be impacted by the change)

there are a huge number of warnings that can be turned into exceptions
with minimal impact ("sprintf" E_WARNING are clearly a good first
candidate)

Ok, so my team and I are willing to put some time to turn some of those
warnings into exceptions. I understand there are some questions to be
answered regarding the nature of the exception (what class, what class
hierarchy...)

My questions:

1- Do you think there is a chance an RFC on this topic might be accepted?
Shall I start working on it?
2- Shall we start working on a list of candidate functions?

David

5 years ago by Nikita Popov — view source

unread

On Mon, Oct 21, 2019 at 3:52 PM David Negrier <
d.negrier@thecodingmachine.com> wrote:

Hey list,

TL;DR:

I'm considering writing an RFC (and a patch) to turn some warning into
exceptions in a number of PHP functions.
I would first like to gather some feedback here.

The long version:

It is the first time I'm posting on internals.
Some of you may already know me. I'm one of the authors of
thecodingmachine/safe, a library that wraps all PHP functions that return
"false" on error in similar functions that will throw on exception on
error: https://github.com/thecodingmachine/safe/

Surprisingly enough, I've had a huge positive feedback with Safe (I reached
1000 stars on Github in a few weeks, so I think I can say this is something
that bothers a lot of people).

Of course, I could simply have written an error handler that would turn
E_WARNING into exceptions but:

For some libs, I wanted to write code that would behave consistently
(whatever the settings of error_reporting)

More than all, having a return type that can be "string|false", or
"object|false" is a true annoyance when you want to write type-safe code
(which is more and more common with the advent of tools like PHPStan or
Psalm).

For instance, "sprintf" can return "string" or "false" (in case the format
is incorrect). If the string passed to "sprintf" is invalid, I would
certainly prefer knowing right now (with an exception) than risking having
the warning sleep under my nose.

Needless to say: a good practice in error handling is "fail loud, fail
fast". A warning at some point will most certainly lead to an error a few
lines later if error handling is not done properly.

If you look at the frameworks out there, I believe all of them are shipping
with an error handler that throws exceptions by default (even on notices).

Also, my team and I are starting to spend a lot of time maintaining Safe.
So I started wondering if rather than spending time maintaining a patch in
user land, we could not instead spend time fixing things in the core.

So here we are, my team and I started playing with php-src. We are not
regular C developers, but we managed to write a small patch to turn
"sprintf" warnings into exceptions.

This is far from ready but the PR is here:
https://github.com/php/php-src/pull/4837

We would be happy to promote more warnings to exceptions as those:

are more predictable

can be more easily caught / handled

enhance the type system by removing the "false" return type

This is going in the same direction as Nikita RFC's regarding reclassifying
"Engine Warnings".

I understand there have been a lot of discussions going on recently
regarding maintaining backward compatibility vs evolving/sanitizing the
language and I know this is a sensitive topic.

That being said:

most frameworks are already providing an error handler that turns warning
into exceptions (so they should not be impacted by the change)

there are a huge number of warnings that can be turned into exceptions
with minimal impact ("sprintf" E_WARNING are clearly a good first
candidate)

Ok, so my team and I are willing to put some time to turn some of those
warnings into exceptions. I understand there are some questions to be
answered regarding the nature of the exception (what class, what class
hierarchy...)

My questions:

1- Do you think there is a chance an RFC on this topic might be accepted?
Shall I start working on it?
2- Shall we start working on a list of candidate functions?

David

As mentioned on the PR, changing warnings to Error exceptions in library
functions doesn't need an RFC as long as the conversion satisfies the
following principle: The error condition must indicate a programming error
and it can be reasonably assumed that this error condition is not being
handled by checking the return value.

Example 1: fopen() on a non-existent file throws a warning and returns
false. This absolutely can not be converted into an exception, because
both a) it does not indicate a programming error (it is a totally expected
environment condition) and b) we expect people to check the return value of
the function for a "false" value.

Example 2: count_chars() accepts a "mode" parameter that can only have the
value 0, 1, 2, 3 or 4. Using an out-of-range value results in a warning and
false return value. This warning can be converted into an exception (and
in fact is a ValueError in PHP 8), because passing an invalid mode is an
unambiguous programming error.

It is not always as obvious into which category a particular error
condition falls, this is decided on a case-by-case basis.

Regards,
Nikita

5 years ago by Rowan Tommins — view source

unread

Hi David,

Firstly, I agree with Nikita's general rule of which Warnings should be
promoted and which need deeper thought.

I would like to challenge one assertion in your e-mail, which is related to
that thought process:

On Mon, 21 Oct 2019 at 14:52, David Negrier d.negrier@thecodingmachine.com
wrote:

We would be happy to promote more warnings to exceptions as those:

are more predictable

can be more easily caught / handled

I have always thought, and continue to think, that converting all Warnings
(and Notices) to Exceptions is inappropriate, because Exceptions have some
very specific behaviours, which are not always desirable:

They immediately jump control out of the current frame of execution.
Unless you put a separate "try-catch" around every line, there is no
"acknowledge and run next line".
Their default behaviour if not handled is to completely terminate
whatever is happening, right up to showing a blank white screen.

There is no general way to know whether the result of aborting execution
completely is better or worse than carrying on with unexpected values. In a
program that's not expecting it, either one could lead to data loss or
corruption.

There are definitely places where a Warning can reasonably be converted to
an Exception; but I don't think this should be done as some kind of bulk
change. Each Warning we promote should have at least one person answer the
question "is it likely that terminating processing when this happens at
run-time will be better than flagging a Warning and returning a defined
value?"

Regards,

Rowan Tommins
[IMSoP]

5 years ago by Mark Randall — view source

unread

They immediately jump control out of the current frame of execution.
Unless you put a separate "try-catch" around every line, there is no
"acknowledge and run next line".

I've been toying with the idea of:

$x = tryval fopen('missing.txt', 'r'),
FileException => null;

if ($x === null) {
die('File could not be opened.');
}

--
Mark Randall

5 years ago by Andreas Hennings — view source

unread

I agree with the distinction described by Nikita.
There are definitely cases where a special return value is still the
best option.

In addition I would say in some cases it can be useful to have both
versions available: One that returns FALSE or NULL, another that
throws an exception.
This is for cases where it is up to the calling code whether a failure
is considered "exceptional" or not, or whether the calling code wants
to check the error in place or have it bubble up the call stack.

Whether something is exceptional can be a matter of expectation, and
previous knowledge within the calling code.

Take e.g. the reflection API:

If we call "new ReflectionClass(self::class)", we assume that the
class exists. If it does not, this would warrant a runtime exception.
If we already checked "class_exists($class)" before, then it makes
sense for "new ReflectionClass($class)" to throw an exception. We
would even want a runtime exception.
If we receive an arbitrary string that matches a class name regex,
we would like to have a ReflectionClass::createOrNull($class), which
would NOT throw an exception, but return NULL if the class does not
exist.

If we provide two variations, they should definitely live in different
functions / methods, instead of e.g. having a parameter to determine
the failure behavior.
Having two separate methods/functions allows better code verification
by the IDE, and avoids false inspection warnings for "unhandled
exception".

Considering the BC impact, I think that providing an alternative
method/function will be the best we can do in most cases.

-- Andreas

Hi David,

Firstly, I agree with Nikita's general rule of which Warnings should be
promoted and which need deeper thought.

I would like to challenge one assertion in your e-mail, which is related to
that thought process:

On Mon, 21 Oct 2019 at 14:52, David Negrier d.negrier@thecodingmachine.com
wrote:

We would be happy to promote more warnings to exceptions as those:

are more predictable

can be more easily caught / handled

I have always thought, and continue to think, that converting all Warnings
(and Notices) to Exceptions is inappropriate, because Exceptions have some
very specific behaviours, which are not always desirable:

They immediately jump control out of the current frame of execution.
Unless you put a separate "try-catch" around every line, there is no
"acknowledge and run next line".

Their default behaviour if not handled is to completely terminate
whatever is happening, right up to showing a blank white screen.

There is no general way to know whether the result of aborting execution
completely is better or worse than carrying on with unexpected values. In a
program that's not expecting it, either one could lead to data loss or
corruption.

There are definitely places where a Warning can reasonably be converted to
an Exception; but I don't think this should be done as some kind of bulk
change. Each Warning we promote should have at least one person answer the
question "is it likely that terminating processing when this happens at
run-time will be better than flagging a Warning and returning a defined
value?"

Regards,

Rowan Tommins
[IMSoP]

5 years ago by Benjamin Morel — view source

unread

I agree with the distinction described by Nikita.
There are definitely cases where a special return value is still the
best option.

I personally like exceptions in all cases, as they allow for fine-grained
error handling, for example:

try {
    mkdir('somedir');
} catch (FileExistsException $e) {
    // only catch *this* exception, which my program expects,
    // let any other exception (say a PermissionException or a generic
IoException) bubble up!
    // this *is* what you want: it will be caught by your top-level
exception handler which will log it to inform you that something is wrong
with your system
}

Instead of:

if (! mkdir('somedir')) {
    // now what? did it fail because the directory already exists? or
because of any other reason?
    // sure, you can now perform a `file_exists()` or `is_dir()`, but:
    // 1. it's not atomic
    // 2. this second call may fail as well (especially if it's an I/O
error), and now what?
    // 3. if this is something like an I/O exception or a permission
exception, you really should log it;
    //    will you replicate the logging logic in each and every of your
filesystem function calls?
}

Last but not least, a blind mkdir() in a quick-and-dirty script will stop
execution if it fails, which in the vast majority of the cases is what you
want.

— Benjamin

I agree with the distinction described by Nikita.
There are definitely cases where a special return value is still the
best option.

In addition I would say in some cases it can be useful to have both
versions available: One that returns FALSE or NULL, another that
throws an exception.
This is for cases where it is up to the calling code whether a failure
is considered "exceptional" or not, or whether the calling code wants
to check the error in place or have it bubble up the call stack.

Whether something is exceptional can be a matter of expectation, and
previous knowledge within the calling code.

Take e.g. the reflection API:

If we call "new ReflectionClass(self::class)", we assume that the
class exists. If it does not, this would warrant a runtime exception.

If we already checked "class_exists($class)" before, then it makes
sense for "new ReflectionClass($class)" to throw an exception. We
would even want a runtime exception.

If we receive an arbitrary string that matches a class name regex,
we would like to have a ReflectionClass::createOrNull($class), which
would NOT throw an exception, but return NULL if the class does not
exist.

If we provide two variations, they should definitely live in different
functions / methods, instead of e.g. having a parameter to determine
the failure behavior.
Having two separate methods/functions allows better code verification
by the IDE, and avoids false inspection warnings for "unhandled
exception".

Considering the BC impact, I think that providing an alternative
method/function will be the best we can do in most cases.

-- Andreas

On Mon, 21 Oct 2019 at 18:03, Rowan Tommins rowan.collins@gmail.com
wrote:

Hi David,

Firstly, I agree with Nikita's general rule of which Warnings should be
promoted and which need deeper thought.

I would like to challenge one assertion in your e-mail, which is related
to
that thought process:

On Mon, 21 Oct 2019 at 14:52, David Negrier <
d.negrier@thecodingmachine.com>
wrote:

We would be happy to promote more warnings to exceptions as those:

are more predictable

can be more easily caught / handled

I have always thought, and continue to think, that converting all
Warnings
(and Notices) to Exceptions is inappropriate, because Exceptions have
some
very specific behaviours, which are not always desirable:

They immediately jump control out of the current frame of execution.
Unless you put a separate "try-catch" around every line, there is no
"acknowledge and run next line".

Their default behaviour if not handled is to completely terminate
whatever is happening, right up to showing a blank white screen.

There is no general way to know whether the result of aborting execution
completely is better or worse than carrying on with unexpected values.
In a
program that's not expecting it, either one could lead to data loss or
corruption.

There are definitely places where a Warning can reasonably be converted
to
an Exception; but I don't think this should be done as some kind of bulk
change. Each Warning we promote should have at least one person answer
the
question "is it likely that terminating processing when this happens at
run-time will be better than flagging a Warning and returning a defined
value?"

Regards,

Rowan Tommins
[IMSoP]

5 years ago by Rowan Tommins — view source

unread

I personally like exceptions in all cases, as they allow for
fine-grained error handling, for example:

try {
    mkdir('somedir');
} catch (FileExistsException $e) {
    // only catch *this* exception, which my program expects,
    // let any other exception (say a PermissionException or a generic 
IoException) bubble up!
    // this *is* what you want: it will be caught by your top-level 
exception handler which will log it to inform you that something is 
wrong with your system
}

I think this argument is something of a logical fallacy: just because
exceptions are (or can be) more fine-grained than the current return
value, that doesn't mean they're the best tool for the job.

If I'm remembering it correctly, an example is the old PEAR
error-handling model, where rather than false, functions returned a
PEAR_Error object. This could be just as fine-grained (using sub-classes
or error code constants) as an exception, but without the additional
behaviour of blowing through the stack.

In some cases, you can go one step further, and have a "result" object,
with methods like isSuccessful(), getErrorCode(), and getOutput().

Importantly, this can and should be different for different functions.

In a previous message you wrote this, which I think mischaracterizes /
misunderstands previous discussions:

To play the devil's advocate, however, IIRC some discussions in the past
mentioned that the BC breakage that comes with it is such an issue that it
would be preferable to create a brand new API rather than attempting to fix
the existing one.
Nobody ever jumped in to propose such an API, though :-(

It's not that we need "a brand new API", singular, it's that some of the
more complex sets of functions should be redesigned, with error handling
considered as part of each design.

So, a new file-handling API could provide a much richer set of error
codes for its equivalent of fopen(), which might or might not involve
exceptions. It might instead have a file handle object which could
represent closed and errored handles, so that attempting to open a file
would always give the same result type, but would have information about
what failed.

Meanwhile, if we were to design a new string API to make things more
consistent, and perhaps introduce better Unicode support where relevant,
that would be a good chance to throw exceptions for truly exceptional
cases. However, we might also include more forgiving variants of
functions, for "best effort" processing of user input (like iconv's
//TRANSLIT and //IGNORE modes).

On the other hand, there are some errors where the answer to the
question "would it be better to halt processing than continue in an
unknown state?" is "yes", so it's reasonable to introduce an exception,
even though existing programs aren't expecting it. But again, this
should be considered for every case, not as a general rule that all
Warnings should be promoted.

Regards,

--
Rowan Tommins (né Collins)
[IMSoP]

5 years ago by Benjamin Morel — view source

unread

I think this argument is something of a logical fallacy: just because
exceptions are (or can be) more fine-grained than the current return
value, that doesn't mean they're the best tool for the job.
If I'm remembering it correctly, an example is the old PEAR
error-handling model, where rather than false, functions returned a
PEAR_Error object. This could be just as fine-grained (using sub-classes
or error code constants) as an exception, but without the additional
behaviour of blowing through the stack.

Sure, you can do without exceptions. I think what you're suggesting is
similar to Go's error handling. But PHP at some point decided in favour of
exceptions, so it would be logical to pursue in that direction.

On the other hand, there are some errors where the answer to the

question "would it be better to halt processing than continue in an
unknown state?" is "yes", so it's reasonable to introduce an exception,
even though existing programs aren't expecting it. But again, this
should be considered for every case, not as a general rule that all
Warnings should be promoted.

I would classify most, if not all, filesystem-related functions as mostly
"yes, do stop execution by default when something fails". So this is, as
well, in favour of exceptions.

To take the mkdir() example, the only reasonable failure reason I can think
of that would not justify throwing an exception, is when the directory
already exists.
So I would be OK with this:

mkdir(): bool; // true if successful, false if the target directory already
exists

This is, IMO, the only time we can safely proceed while ignoring the
outcome of the operation, as the filesystem is in the same state whether or
not the function succeeded (the directory exists). All other outcomes are,
IMO, exceptional situations, and throwing an exception is a safe way to
guarantee that execution will not continue after such an error.

Handling each and every error manually by using the return value requires a
lot of discipline, which could be a very steep learning curve for PHP
developers used to a fast prototyping language. And if people don't
strictly follow the rules, you're opening the door to a whole lot potential
bugs in codebases. And, you miss the ability of automatic error reporting
for uncaught errors, that you get almost for free with exceptions and a
single try/catch at the top level.

— Benjamin

I personally like exceptions in all cases, as they allow for
fine-grained error handling, for example:
try {
    mkdir('somedir');
} catch (FileExistsException $e) {
    // only catch *this* exception, which my program expects,
    // let any other exception (say a PermissionException or a generic
IoException) bubble up!
    // this *is* what you want: it will be caught by your top-level
exception handler which will log it to inform you that something is
wrong with your system
}
I think this argument is something of a logical fallacy: just because
exceptions are (or can be) more fine-grained than the current return
value, that doesn't mean they're the best tool for the job.

If I'm remembering it correctly, an example is the old PEAR
error-handling model, where rather than false, functions returned a
PEAR_Error object. This could be just as fine-grained (using sub-classes
or error code constants) as an exception, but without the additional
behaviour of blowing through the stack.

In some cases, you can go one step further, and have a "result" object,
with methods like isSuccessful(), getErrorCode(), and getOutput().

Importantly, this can and should be different for different functions.

In a previous message you wrote this, which I think mischaracterizes /
misunderstands previous discussions:

To play the devil's advocate, however, IIRC some discussions in the past
mentioned that the BC breakage that comes with it is such an issue that
it
would be preferable to create a brand new API rather than attempting to
fix
the existing one.
Nobody ever jumped in to propose such an API, though :-(

It's not that we need "a brand new API", singular, it's that some of the
more complex sets of functions should be redesigned, with error handling
considered as part of each design.

So, a new file-handling API could provide a much richer set of error
codes for its equivalent of fopen(), which might or might not involve
exceptions. It might instead have a file handle object which could
represent closed and errored handles, so that attempting to open a file
would always give the same result type, but would have information about
what failed.

Meanwhile, if we were to design a new string API to make things more
consistent, and perhaps introduce better Unicode support where relevant,
that would be a good chance to throw exceptions for truly exceptional
cases. However, we might also include more forgiving variants of
functions, for "best effort" processing of user input (like iconv's
//TRANSLIT and //IGNORE modes).

On the other hand, there are some errors where the answer to the
question "would it be better to halt processing than continue in an
unknown state?" is "yes", so it's reasonable to introduce an exception,
even though existing programs aren't expecting it. But again, this
should be considered for every case, not as a general rule that all
Warnings should be promoted.

Regards,

--
Rowan Tommins (né Collins)
[IMSoP]

5 years ago by Mike Schinkel — view source

unread

Sure, you can do without exceptions. I think what you're suggesting is
similar to Go's error handling.

Yes, it is like Go's error handling.

But PHP at some point decided in favour of
exceptions, so it would be logical to pursue in that direction.

Must we?

Maybe it is time to (at least) consider alternate approaches? Not to deprecate Exceptions — as many prefer to use them — but as an alternative to Exceptions for those who would prefer to use error values instead?

I would classify most, if not all, filesystem-related functions as mostly
"yes, do stop execution by default when something fails". So this is, as
well, in favour of exceptions.

Are you sure? Could a file system function not just as easily "stop" and return an error value vs. throwing an exception?

To take the mkdir() example, the only reasonable failure reason I can think
of that would not justify throwing an exception, is when the directory
already exists.

So I would be OK with this:

mkdir(): bool; // true if successful, false if the target directory already
exists

This is, IMO, the only time we can safely proceed while ignoring the
outcome of the operation, as the filesystem is in the same state whether or
not the function succeeded (the directory exists). All other outcomes are,
IMO, exceptional situations, and throwing an exception is a safe way to
guarantee that execution will not continue after such an error.

Why must it be a thrown exception vs. a returned error object?

For a given application that failure might not be exceptional; the app might have a fallback solution that want to use instead.

Handling each and every error manually by using the return value requires a
lot of discipline, which could be a very steep learning curve for PHP
developers used to a fast prototyping language. And if people don't
strictly follow the rules, you're opening the door to a whole lot potential
bugs in codebases. And, you miss the ability of automatic error reporting
for uncaught errors, that you get almost for free with exceptions and a
single try/catch at the top level.

That is definitely a good argument to keep supporting Exceptions in PHP.

But not an argument to make Exceptions the only approach. I have been coding professionally for a really long long and — until recently — never felt like I was happy with how I was doing error handling, always thinking I would "get around to figuring out a better approach."

Then about a year ago I started using Go, and Go's approach to error handling just clicked for me. Go's philosophy is to handle the error as soon as you one is aware of it and to only ever handle it once. Since I have been using that strategy I have become very happy with my error handling, and I now (attempt) to use the same strategy in my PHP code.

In PHP I now typically wrap anything that can generate an exception in my own try_catch() function, and that function returns an error object I can then process if an except is thrown inside. But it is visually ugly and tedious to have to use that function all the time.

So what I am hoping for is that PHP recognizes there are two strategies for error handling — 1. Throwing exceptions vs. 2. Returning error values — and that PHP 8.0 and behind will respect programmers and allow them to embrace the strategy they feel is best for their use-case.

-Mike

5 years ago by Rowan Tommins — view source

unread

Sure, you can do without exceptions. I think what you're suggesting is
similar to Go's error handling. But PHP at some point decided in
favour of exceptions, so it would be logical to pursue in that direction.

I didn't say "do without exceptions", I said "use exceptions when
they're the right tool, and other tools at other times".

I would classify most, if not all, filesystem-related functions as
mostly "yes, do stop execution by default when something fails". So
this is, as well, in favour of exceptions.

You've backed that up with exactly one example function, and immediately
concede a case where you might want a non-exception flow. Here are a
handful more examples off the top of my head:

Attempt to atomically lock and open file, returning either a file
handle or a "could not lock" status.
Attempt to read from a file, terminate loop when the file handle
reaches EOF.
Attempt to delete file, ignore if it doesn't exist.

All of these are candidates for returning false, or an error code, or an
object in a particular status. They could be handled with exceptions,
but that would force common patterns to use exceptions for flow control,
which is generally seen as a "bad smell".

Handling each and every error manually by using the return value
requires a lot of discipline, which could be a very steep learning
curve for PHP developers used to a fast prototyping language.

Again, I am not saying that every function should have a complex
error-handling system. It may well be sensible to have a function for
"open file, I don't expect anything to go wrong" which throws an
exception, and a separate one for "try to open file, and report the
result without ever triggering an exception". That's the point of
designing a new API, to work out what the use cases are, and cater for
them in a clean way.

Regards,

--
Rowan Tommins (né Collins)
[IMSoP]

5 years ago by Benjamin Morel — view source

unread

@Mike,

Then about a year ago I started using Go, and Go's approach to error
handling just clicked for me. Go's philosophy is to handle the error as
soon as you one is aware of it and to only ever handle it once. Since I
have been using that strategy I have become very happy with my error
handling, and I now (attempt) to use the same strategy in my PHP code.
So what I am hoping for is that PHP recognizes there are two strategies
for error handling — 1. Throwing exceptions vs. 2. Returning error values —
and that PHP 8.0 and behind will respect programmers and allow them to
embrace the strategy they feel is best for their use-case.

I do see some value in the way Go handles errors (i.e. forcing you to deal
with them), especially the expected ones; the issue, if I'm not mistaken,
is how you deal with unexpected errors. AFAICS, this forces you to either
return it to the caller (which may not expect this error), or wrap it in a
more specific error that's part of your function's contract, which is
pretty much what you do with exception chaining. I don't have a hands-on
experience with Go, though, so I may have missed something. Please do
correct me if/where needed.

@Rowan,

You've backed that up with exactly one example function, and immediately

concede a case where you might want a non-exception flow. Here are a
handful more examples off the top of my head:

Attempt to atomically lock and open file, returning either a file
handle or a "could not lock" status.

Attempt to read from a file, terminate loop when the file handle
reaches EOF.

Attempt to delete file, ignore if it doesn't exist.
All of these are candidates for returning false, or an error code, or an
object in a particular status. They could be handled with exceptions,
but that would force common patterns to use exceptions for flow control,
which is generally seen as a "bad smell".

I used this one function as an example, but I'm happy to apply my point of
view to other examples if you wish. I'll take yours above, in reverse order:

- Attempt to delete file, ignore if it doesn't exist:

unlink($file): bool; // true if the file was deleted, false if it didn't
exist; exception on other errors

As I see it, the only safe-to-ignore error is "the file does not exist"
(non-exceptional error), as the outcome is the same whatever the return
value: the file does not exist (anymore).
Any other error should, IMO, be treated as exceptional and throw.

- Attempt to read from a file, terminate loop when the file handle reaches
EOF:

I think the current API is already quite good at that, it just needs a bit
of polish in the way it handles errors:

while (! feof($fp)) $block = fread($fp, 4096);

feof($fp): bool; // true if EOF, false if not; exception on error
fread($fp): string; // the data, empty string if EOF (non-exceptional);
exception on error

I don't see much value in doing something like:

for (;;) {
$result = fread($fp, 4096);
if ($result->isEOF) break;
if ($result->error) {
// ...
}

$block  = $result->data;

}

- Attempt to atomically lock and open file, returning either a file handle
or a "could not lock" status:

fopen_and_lock($fp); // true if lock was acquired, false if not; exception
on error

This one is trickier, as the outcome is roughly the same (the file was
open) but maybe the lock failed to acquire. I can't say if I would return
bool, or void and an exception on failure-to-acquire.

Don't get me wrong, I respect your points of view, they are perfectly
valid, and as such I'm not trying to push exceptions over anything else;
but I don't personally see them as bad at all, they're actually quite
convenient if used properly. I do see some value in returning errors, too,
but with the strong caveat expressed below, that may severely affect
inexperienced, or, should I say it, lazy developers. I like the exception
model, breaking the program flow by default if unhandled. This makes an app
much more reliable by default IMHO.

Therefore I do like the original proposal, even if I can hardly imagine it
overcome the usual resistance here.
This proposal would be as good under the form of a new API though, but in
this case the naming should be changed to clearly differentiate both APIs.

I wish you luck, @David, anyway.

— Benjamin

Sure, you can do without exceptions. I think what you're suggesting is
similar to Go's error handling. But PHP at some point decided in
favour of exceptions, so it would be logical to pursue in that direction.

I didn't say "do without exceptions", I said "use exceptions when
they're the right tool, and other tools at other times".

I would classify most, if not all, filesystem-related functions as
mostly "yes, do stop execution by default when something fails". So
this is, as well, in favour of exceptions.

You've backed that up with exactly one example function, and immediately
concede a case where you might want a non-exception flow. Here are a
handful more examples off the top of my head:

Attempt to atomically lock and open file, returning either a file
handle or a "could not lock" status.

Attempt to read from a file, terminate loop when the file handle
reaches EOF.

Attempt to delete file, ignore if it doesn't exist.

All of these are candidates for returning false, or an error code, or an
object in a particular status. They could be handled with exceptions,
but that would force common patterns to use exceptions for flow control,
which is generally seen as a "bad smell".

Handling each and every error manually by using the return value
requires a lot of discipline, which could be a very steep learning
curve for PHP developers used to a fast prototyping language.

Again, I am not saying that every function should have a complex
error-handling system. It may well be sensible to have a function for
"open file, I don't expect anything to go wrong" which throws an
exception, and a separate one for "try to open file, and report the
result without ever triggering an exception". That's the point of
designing a new API, to work out what the use cases are, and cater for
them in a clean way.

Regards,

--
Rowan Tommins (né Collins)
[IMSoP]

5 years ago by Rowan Tommins — view source

unread

On Tue, 22 Oct 2019 at 14:38, Benjamin Morel benjamin.morel@gmail.com
wrote:

I used this one function as an example, but I'm happy to apply my point of
view to other examples if you wish.

You have phrased this as though your point of view is different from mine,
but I think you've just misunderstood it. I said:

All of these are candidates for returning false, or an error code, or an
object in a particular status.

That's precisely what your examples show: the functions returning false. So
we're in agreement, some errors should become exceptions, others should be
handled a different way, and some cases need new functions.

This proposal would be as good under the form of a new API though, but in
this case the naming should be changed to clearly differentiate both APIs.

Yes, new names is almost entirely the point of "a new API". The idea is
that you don't have to recalibrate people's expectations of what fopen()
does, breaking thousands of lines of existing code, nor compromise the
design to minimise that impact. Instead, you say "here's a new function, it
has these nice features, and handles errors in this really smooth way,
please use it" - and more importantly, "here's a consistent set of
functions which all work in this same way, please use them".

Finally, to reiterate what's been said several times, there are definitely
functions where the error cases are so "exceptional", that just throwing an
exception would be perfectly fine. It's the commonly used sets of functions
with a variety of different error conditions and use cases, like file
handling, where more careful redesign is prudent.

Regards,

Rowan Tommins
[IMSoP]

5 years ago by Dik Takken — view source

unread

There is no general way to know whether the result of aborting execution
completely is better or worse than carrying on with unexpected values. In a
program that's not expecting it, either one could lead to data loss or
corruption.

I would guess that carrying on with unexpected values is worse because
it happens more often. Checking for false or null is something that
everybody forgets all the time. Exceptions are more explicit and an IDE
will point you at places in your code where you forgot to either handle
or delegate them.

There are definitely places where a Warning can reasonably be converted to
an Exception; but I don't think this should be done as some kind of bulk
change. Each Warning we promote should have at least one person answer the
question "is it likely that terminating processing when this happens at
run-time will be better than flagging a Warning and returning a defined
value?"

I fully agree. However I think the fear of having to add tons of try /
catch to existing code is exaggerated. I use Python a lot, which is
quite an exception throwing machine in comparison. Still, my code is not
cluttered with exception handling.

So, converting a ton of warnings to exceptions in PHP does not worry me
at all. I would welcome it, but not as a bulk operation. I agree that we
need to ask the question for each case. I would still prefer the answers
to bias towards exceptions though.

Regards,
Dik Takken

5 years ago by Mike Schinkel — view source

unread

I fully agree. However I think the fear of having to add tons of try /
catch to existing code is exaggerated. I use Python a lot, which
quite an exception throwing machine in comparison. Still, my code is not
cluttered with exception handling.

So, converting a ton of warnings to exceptions in PHP does not worry me
at all. I would welcome it, but not as a bulk operation. I agree that we
need to ask the question for each case. I would still prefer the answers
to bias towards exceptions though.

It is not a "fear" for some, where fear is something someone worries about that might be unfounded.

For me, at least, it is an annoyance I already have to deal with for those things that throw exception, such as DateTime and DateTimeZone.

So not a fear but a preference not to make existing annoyances any worse than they already are. #jmtcw

Or maybe introduce a try() function that can wrap calls. Since most - ife
not all - functions would throw a single type of exception, the
following would catch that exception and return null in stead:

$x = try(fopen('missing.txt', 'r'))

Now that I could really get behind.

However, I might suggest it be handled slightly differently, based on my experience with Go (which IMO does not handle in a optimal way.)

Basically you want to be able to do something like this:

if ( $result = try( fopen( 'missing.txt', 'r' ), $handle ) ) {
error_log( $result->getMessage() );
return;
}
$contents = fread($handle, filesize($filename));
fclose($handle);

Effectively I think we want to be able to easily tell as an expression whether it succeeded for failed. Try() could returns either the exception, if thrown, or false (or null) if no exception was thrown. The called function's return value would be returned by reference as the second parameter.

Here, try() would swallow only FileException, other exceptions are still
thrown. I'm not sure if this construct is worth introducing though, the
difference compared to a proper try / catch is much smaller.

I would want it to capture every exception. Why not?

-Mike

5 years ago by Rowan Tommins — view source

unread

Here, try() would swallow only FileException, other exceptions are still
thrown. I'm not sure if this construct is worth introducing though, the
difference compared to a proper try / catch is much smaller.

I would want it to capture every exception. Why not?

Because you might want to convert "FileLockedByAnotherProcessException" to
a locally-handled error, but allow "ServerIsOnFireException" to bubble up
to a higher-level exception handler. That's why catch blocks basically
force you to specify class/interface names: you should catch only the
exceptions you actually know how to handle in that particular piece of code.

Regards,

Rowan Tommins
[IMSoP]

5 years ago by Mike Schinkel — view source

unread

Here, try() would swallow only FileException, other exceptions are still
thrown. I'm not sure if this construct is worth introducing though, the
difference compared to a proper try / catch is much smaller.

I would want it to capture every exception. Why not?

Because you might want to convert "FileLockedByAnotherProcessException" to
a locally-handled error, but allow "ServerIsOnFireException" to bubble up
to a higher-level exception handler. That's why catch blocks basically
force you to specify class/interface names: you should catch only the
exceptions you actually know how to handle in that particular piece of code.

I totally understand that, for those wanting to program with exceptions.

But if we added a try() "function", the goal of that function IMO would be to capture all exceptions and return them as errors for those who don't want to use exceptions. But I could just as easily has logic that says "if it is ServerOnException exception, I can throw it again" If I want too. Or I can handle it another way.

The point it, I am asking to be put 100% in control and be able to choose how to handle errors, not have to figure out which exceptions somethings throws because a function can throw many different types and those types are not always well documented.If it it is returned, I will always get it, even if my code did not expect it.

-Mike

5 years ago by Mike Schinkel — view source

unread

Hey list,

TL;DR:

I'm considering writing an RFC (and a patch) to turn some warning into
exceptions in a number of PHP functions.
I would first like to gather some feedback here.

JMTCW, which is an opinion admittedly probably not shared by most who work with PHP, but I find exceptions problematic and try to avoid them whenever possible. I have again included this list of references[1-6] to explain why I found them problematic, which I had mentioned just yesterday on an unrelated message to this list.

The idea that functions that I could previously dealt with in four lines of code:
$filepath = DIR . '/foobar.json';
if (false === ($content = file_get_contents($filepath)) ) {
error_log( sprintf("failed to open '%s'", $filepath ));
return false;
}
Will potentially now requires me to add a second brace-enclosed block and two more lines of code and force me to handle varying exception classes causes me to cringe and will make me want to cry:

$filepath = DIR . '/foobar.json';
try {
$content = file_get_contents( $filepath );
} catch (Exception $exception) {
error_log( sprintf( "failed to open '%s'", $filepath ) );
return false;
}

Since I know that many in the PHP world believe that exceptions are a good thing, I understand that I will be unlikely to change everyone's mind and get them to start doing error handling as soon as they discover the error rather than just throw it and let some other code that doesn't have as much context deal with it.

But my hope is at least PHP won't change to force having to use try/catch for practically every function call. Such a change might even be enough to finally get me to give up on PHP and move full time to Go because it will make coding in PHP so tedious.

For me to support such a change — not that I have a vote — I would ask we add a pragma that could be set at the top of every file that specifies that functions throw exceptions or return errors, and for BC please make the lack of such pragma to default to returning errors.

Or better yet, accept Benjamin Morel's suggestion to create a brand new API for this rather than attempting to fix the existing one.

One of the other. Please.

-Mike
P.S. Frankly — in my perfect world — we would have options to not throw Exceptions for everything that currently throws an exception and instead return an Error object value that indicates the error. They could even return the Exception — that would work — just don't throw them so I do not have to set up a try{}catch(Execption $e){} construct.

Just the other day I had to deal with the fact PhpStorm flags functions that return Exceptions if I don't handle them, and new DateTime() throws an Exception because it's optional parameter could cause a failure, even though I was not passing in any parameter. I had to wrap in my own TryCatch() function that basically just buries the Exception to get PhpStorm to stop complaining.

But I do not hold out much hope for having an option to bypass all Exception ever being accepted by the PHP community.

[1] http://www.lighterra.com/papers/exceptionsharmful/
[2] https://sidburn.github.io/blog/2016/03/25/exceptions-are-evil
[3] http://xahlee.info/comp/why_i_hate_exceptions.html
[4] https://www.joelonsoftware.com/2003/10/13/13/
[5] https://mortoray.com/2012/04/02/everything-wrong-with-exceptions/
[6] https://www.atlassian.com/blog/archives/exceptions_are_bad

5 years ago by Dik Takken — view source

unread

The idea that functions that I could previously dealt with in four lines of code:
$filepath = DIR . '/foobar.json';
if (false === ($content = file_get_contents($filepath)) ) {
error_log( sprintf("failed to open '%s'", $filepath ));
return false;
}
Will potentially now requires me to add a second brace-enclosed block and two more lines of code and force me to handle varying exception classes causes me to cringe and will make me want to cry:

$filepath = DIR . '/foobar.json';
try {
$content = file_get_contents( $filepath );
} catch (Exception $exception) {
error_log( sprintf( "failed to open '%s'", $filepath ) );
return false;
}

Indeed, surrounding the function call with try / catch is what you need
to do when you want to retain the old behavior and convert the exception
into a return value that indicates an error.

However, you could also choose to leverage the new behavior of throwing
exceptions to simplify your code. Passing the error condition to the
caller can now be done by not handling the exception that is thrown, so:

$filepath = DIR . '/foobar.json';
if (false === ($content = file_get_contents($filepath)) ) {
error_log( sprintf("failed to open '%s'", $filepath ));
return false;
}

becomes:

$content = file_get_contents($filepath)

Here we assume that the exception contains an appropriate message
allowing us to omit generating it manually. Of course, the caller will
have to be adjusted to handle the exception in stead of the false. An
IDE like PHPStorm will nicely point you at the places in your code that
need to be adjusted.

Since I know that many in the PHP world believe that exceptions are a good thing, I understand that I will be unlikely to change everyone's mind and get them to start doing error handling as soon as they discover the error rather than just throw it and let some other code that doesn't have as much context deal with it.

But my hope is at least PHP won't change to force having to use try/catch for practically every function call. Such a change might even be enough to finally get me to give up on PHP and move full time to Go because it will make coding in PHP so tedious.

One could also reason the other way around. When a function returns some
value or false in case of an error you have to check the return value
for practically every function call. And we all forget to do so,
resulting in unexpected failures. Exceptions allow us to simply use the
return value without additional checks. Should anything go wrong calling
the function we have plenty of options:

Just let it crash (which is the right thing to do in some cases)
Have a caller higher up in the call stack handle it
Surround the function call with a try / catch later, should we decide
that we want to handle the problem locally anyway.

Just the other day I had to deal with the fact PhpStorm flags functions that return Exceptions if I don't handle them, and new DateTime() throws an Exception because it's optional parameter could cause a failure, even though I was not passing in any parameter. I had to wrap in my own TryCatch() function that basically just buries the Exception to get PhpStorm to stop complaining.

You do not have to use a wrapper. Yes, PHPStorm nicely points you at a
possible failure point in your code, which reminds you to think about
how to handle them. You don't get that when a function returns false or
null in case of error. When you get the code inspection warning, you can
either:

Handle the exception
Pass the exception to the caller and add a @throws to the doc string
Suppress the inspection by using the following inspection hint:

/* @noinspection PhpUnhandledExceptionInspection */

Since exceptions should mostly be used for situations where it is best
to run for the emergency exit, wrapping a function call in a try / catch
is not expected to be something done frequently.

Regards,
Dik Takken

5 years ago by Mike Schinkel — view source

unread

However, you could also choose to leverage the new behavior of throwing
exceptions to simplify your code. Passing the error condition to the
caller can now be done by not handling the exception that is thrown, so:

$filepath = DIR . '/foobar.json';
if (false === ($content = file_get_contents($filepath)) ) {
error_log( sprintf("failed to open '%s'", $filepath ));
return false;
}

becomes:

$content = file_get_contents($filepath)

Here we assume that the exception contains an appropriate message
allowing us to omit generating it manually. Of course, the caller will
have to be adjusted to handle the exception in stead of the false. An
IDE like PHPStorm will nicely point you at the places in your code that
need to be adjusted.

Of course you can program that way, but you eventually have to deal with the error, and if you don't handle it where it happens then you know less about the context than if you just go ahead and handle it were the error first occurred.

This is not just my opinion, but the opinion of numerous others[1-6] (yes, I have recently shared these links twice before.)

One could also reason the other way around. When a function returns some
value or false in case of an error you have to check the return value
for practically every function call.

Note the I am not arguing people should be required to handle errors, only that I (and others like me) are empowered if we want to
to handle the errors without having to try{]catch{} lots of exceptions. And your try() "function" was a good way to empower us to do that.

And we all forget to do so,
resulting in unexpected failures. Exceptions allow us to simply use the
return value without additional checks. Should anything go wrong calling
the function we have plenty of options:

That assumes the developer is going to be sloppy. Admittedly most developer are sloppy, myself included at times.

But I want to be able to get rid of the training wheels and be allowed to build robust software if I choose to do so.

You do not have to use a wrapper. Yes, PHPStorm nicely points you at a
possible failure point in your code, which reminds you to think about
how to handle them.

Yes, unfortunately I know that. :-(

Let's say that the function that I am calling is 10 levels deep in the function call tree. Now I have to either have a try{}catch{} for everything that can throw an exception, or a @throw PHPDoc a the top of each function. But if I do the latter now I have to do the same in the 9th level. And if I add @throw at the 9th level now I have to document at the 8th level too. And so on and so on all the way up to level 2.

So basically with Exceptions in PhpStorm I am forced to me deal with the exception either with a try{}catch{} OR I have to do it at every level of my code. And it is possible to disable those warnings, but I WANT those warning so I don't miss an exception. I just want a way not to have most functions return exceptions.

So it would be much easier to just handle it once locally without the try{}catch{}. And if PHP were to introduce a standard error object, say PHP\Error, then PhpStorm could flag them as not being handled for every function that potentially returns one.

You don't get that when a function returns false or
null in case of error.

True, but that is not what your try() "function" idea would do, as described above. It would/should return either the exception object, or maybe better a new PHP\Error type, or SplError type.

Since exceptions should mostly be used for situations where it is best
to run for the emergency exit, wrapping a function call in a try / catch
is not expected to be something done frequently.

The problem with that is the code throwing the exception makes the decision it is "exceptional" on behalf of the app/website, but it is really the app/websites responsibility to decide it is truly exceptional from the perspective of the app/website. It is very possible the
app/website has a fallback for a scenario that someone who implemented a core PHP function believed was exceptional.

Basically, I am just asking to be put in control related to errors/exceptions instead of having to work around decisions other people made for me.

-Mike

[1] http://www.lighterra.com/papers/exceptionsharmful/ http://www.lighterra.com/papers/exceptionsharmful/
[2] https://sidburn.github.io/blog/2016/03/25/exceptions-are-evil https://sidburn.github.io/blog/2016/03/25/exceptions-are-evil
[3] http://xahlee.info/comp/why_i_hate_exceptions.html http://xahlee.info/comp/why_i_hate_exceptions.html
[4] https://www.joelonsoftware.com/2003/10/13/13/ https://www.joelonsoftware.com/2003/10/13/13/
[5] https://mortoray.com/2012/04/02/everything-wrong-with-exceptions/ https://mortoray.com/2012/04/02/everything-wrong-with-exceptions/
[6] https://www.atlassian.com/blog/archives/exceptions_are_bad <https://www.atlassian.com/blog/archives/exceptions_are_bad