Error instead of returning false

Hello internals,

I wanted to propose an RFC, but i'd like to check first if this idea
has been considered before already.

My idea, extremely summarized, would be to take the functions that
return false/null when they 'error', and instead make them actually
throw an error.

Currently, functions like getcwd or json_decode will return false/null
when an error occurs, and they populate a global error state, which
can be retrieved by error_get_last, or similar functions. json_decode
already got the JSON_THROW_ON_ERROR option in PHP 7.3, and this RFC
would basically make that behaviour default. This RFC would target PHP
8.0, but i'd like to hear some opinions on this.

Greetings,
Gert de Pagter (BackEndTea)

Hello internals,

I wanted to propose an RFC, but i'd like to check first if this idea
has been considered before already.

My idea, extremely summarized, would be to take the functions that
return false/null when they 'error', and instead make them actually
throw an error.

Currently, functions like getcwd or json_decode will return false/null
when an error occurs, and they populate a global error state, which
can be retrieved by error_get_last, or similar functions. json_decode
already got the JSON_THROW_ON_ERROR option in PHP 7.3, and this RFC
would basically make that behaviour default. This RFC would target PHP
8.0, but i'd like to hear some opinions on this.

This would be a pretty big BC break for those functions. That's not
necessarily a bad thing, but, I think the burden for justification is
larger than just adding a new feature. These are the questions I'd ask

1.) What is detrimental about the current behavior?
2.) What is beneficial about the new behavior?
3.) Are the items for #1 and/or #2 big enough to justify breaking existing
code?
4.) Is there a way to achieve the desired behavior without a BC break.

My initial thoughts are that impacts of the BC break would be pretty big.
EVERYONE using json_decode without JSON_THROW_ON_ERROR will have to modify
their code. It's not a quick find/replace change either. Instead of
checking the return, and then checking the error state (or, maybe they are
OK with just continuing if there is an error), now has to wrap the call
into a try/catch block.

I personally don't see anything that detrimental about the current behavior
or that beneficial about the new behavior. It's not like try/catch is that
much simpler from an implementation standpoint that checking a global error
state based on the return value:
$foo = json_decode($bar);
if(false === $foo){
//get error and do whatever to handle it
}
vs
try {
$foo = json_decode($bar);
} catch (\Throwable $t){
//do whatver to handle it
}

It also appears we can easily provide the functionality you desire with
additional options. You mentioned that json_decode has already been given a
new option, JSON_THROW_ON_ERROR. What is the advantage to making this the
default behavior vs letting people specify the flag when they want? If
there a reason adding such an option as a new parameter wouldn't work for
other methods, like getcwd?

Greetings,
Gert de Pagter (BackEndTea)

--

--
Chase Peeler
chasepeeler@gmail.com

Hello internals,

I wanted to propose an RFC, but i'd like to check first if this idea
has been considered before already.

My idea, extremely summarized, would be to take the functions that
return false/null when they 'error', and instead make them actually
throw an error.

Currently, functions like getcwd or json_decode will return false/null
when an error occurs, and they populate a global error state, which
can be retrieved by error_get_last, or similar functions. json_decode
already got the JSON_THROW_ON_ERROR option in PHP 7.3, and this RFC
would basically make that behaviour default. This RFC would target PHP
8.0, but i'd like to hear some opinions on this.

Greetings,
Gert de Pagter (BackEndTea)

There's two cases to distinguish here:

1. Error conditions that indicate a programmer error (what we would throw
   an Error exception for nowadays).
2. Error conditions that indicate a data/environment/etc error (what we
   would throw an Exception exception for nowadays).

One large category of warnings of the first kind has already been converted
to TypeErrors in https://wiki.php.net/rfc/consistent_type_errors. And I'd
be in favor of changing more of these in PHP 8. This is (relatively)
non-intrusive from a BC perspective, because these conditions are not
something you should ever explicitly check for: If your code hits them, it
is simply broken.

However, what you seem to have in mind here are error conditions of the
second kind. I don't think we can change these on any timeline. Current
code (that is careful about handling errors) will be checking false/null
return values for them, and throwing an exception instead would be a major
and very hard to automatically fix BC break.

The proper way (imho) to handle an eventual migration to exceptions is to
introduce new APIs that supersede the existing ones. E.g. if we ever get
around to introducing an OO API for the stream layer, than that would be
the time to also introduce use of exceptions.

A possible alternative would be to control this behavior in some way
similar to declare(strict_types).

Regards,
Nikita

My idea, extremely summarized, would be to take the functions that
return false/null when they 'error', and instead make them actually
throw an error.

It comes up on various discussion forums every other month. There's a
few downsides to it, but mainly that any existing error handling would
need to be re-written as tests for the false value would never be
reached after the exception was thrown.

It's a bit less of a hassle for more modern PHP design practices, as any
warnings are globally promoted to exceptions using set_error_handler in
userspace, but this too has its problems (for example in third party
libraries).

IIRC, most functions are at least checking their input argument types
now and throwing InvalidArgumentExceptions, and the odd few like
json_decode have their exception behaviour, provided its flagged. The
new security focused stuff always throws (such as if there's not enough
entropy available).

I don't think it's in any way practical to add throw-or-not flags to
every other internal function that could error out, and so there are 4
options that I can see:

1. Make no change.

2. Everything throws. People have to re-write. The compiler would
   probably need to further inspect the AST at compile time, and throw a
   depreciation warning if the result of an error-able internal function
   was tested for false.

3. Class (or namespace) level declares to throw on internal function errors.

4. Leave the existing functions as-is, finally claim the \php namespace,
   alias all of the internal functions into it, and make their default,
   unchangeable behaviour be to throw a RuntimeException on error.

--
Mark Randall

Le 7 mai 2019 à 16:22, Gert gertp93@gmail.com a écrit :

Hello internals,

I wanted to propose an RFC, but i'd like to check first if this idea
has been considered before already.

My idea, extremely summarized, would be to take the functions that
return false/null when they 'error', and instead make them actually
throw an error.

Currently, functions like getcwd or json_decode will return false/null
when an error occurs, and they populate a global error state, which
can be retrieved by error_get_last, or similar functions. json_decode
already got the JSON_THROW_ON_ERROR option in PHP 7.3, and this RFC
would basically make that behaviour default. This RFC would target PHP
8.0, but i'd like to hear some opinions on this.

Greetings,
Gert de Pagter (BackEndTea)

--

The big issue, with your proposal, is, of course, migration of legacy code:

<?php
$f = @fopen($whatever);
if ($f === false) {
// handle the error.
}
?>

If you do think that ideally an exception should be thrown here, it is better to devise a new function—and, while you’re at it, a function that returns an object of some definite class instead of a resource.

Another issue is that a “failure” is not necessarily an “error”. For example, when I use strpos(), I expect that it returns sometimes false; and either using a try/catch construct, or using a separate check beforehand would be cumbersome.

—Claude

Hi!

My idea, extremely summarized, would be to take the functions that
return false/null when they 'error', and instead make them actually
throw an error.

First of all, they definitely should not throw Error, because Errors are
for very specific types of things that mean the engine does not know
what to do - e.g. if you asked for an integer and you gave parameter
that can not be converted to integer, or you tried to use a function
which does not exist, and so on. Normal functionality should almost
always use Exception unless you have a really great reason not to.

Second, I don't see any point in such a huge BC break for people that
are fine with current return values. Exceptions is not something you can
ignore (even if you can easily ignore method returning false - sometimes
you don't want to, but sometimes you do), it's something that breaks
your application. In some APIs for some corner cases it might be OK, but
in general it's probably not something we'd ever want to do on a global
scale.

Third, I think throwing exceptions for routine situations that are
expected to happen (like file not being there, or data not being valid)
is wrong and this is what makes Java exceptions, for example, annoying.
They confuse "everything broken and world is on fire, head for the
exits" with "there's some dirt on the floor, bring the mop and clean it
up". I.e. situations which are expected to be handled and situations
where things are just gone wrong and it's best for this code do give up
immediately.

Exceptions should be used for situations which are, well, exceptional -
ones that the code surrounding the call is unlikely to be able to fix -
e.g. trying to open file while giving the argument that can not be used
as a filename. Of course, the question what is "normal" for a given API
is very specific to the API - e.g. for a search API not finding any
result would be normal, but for an authorization API not finding certain
record in the database may be exceptional - but I think there should be
a distinction between the two. Which should be used in each specific
case depends a lot on API design and specifics of the case, but there
should be an option for both behaviors.

--
Stas Malyshev
smalyshev@gmail.com

My idea, extremely summarized, would be to take the functions that
return false/null when they 'error', and instead make them actually
throw an error.

It comes up on various discussion forums every other month. There's a
few downsides to it, but mainly that any existing error handling would
need to be re-written as tests for the false value would never be
reached after the exception was thrown.

It's a bit less of a hassle for more modern PHP design practices, as any
warnings are globally promoted to exceptions using set_error_handler in
userspace, but this too has its problems (for example in third party
libraries).

IIRC, most functions are at least checking their input argument types
now and throwing InvalidArgumentExceptions, and the odd few like
json_decode have their exception behaviour, provided its flagged. The
new security focused stuff always throws (such as if there's not enough
entropy available).

I don't think it's in any way practical to add throw-or-not flags to
every other internal function that could error out, and so there are 4
options that I can see:

1. Make no change.

2. Everything throws. People have to re-write. The compiler would
   probably need to further inspect the AST at compile time, and throw a
   depreciation warning if the result of an error-able internal function
   was tested for false.

3. Class (or namespace) level declares to throw on internal function
   errors.

4. Leave the existing functions as-is, finally claim the \php namespace,
   alias all of the internal functions into it, and make their default,
   unchangeable behaviour be to throw a RuntimeException on error.

--
Mark Randall

--

For the record, there is already a library for option 3:
https://github.com/thecodingmachine/safe
Another option would be to introduce a new declare directive, something
like

<?php

declare(throw_on_error=true);

$json = file_get_contents('foobar.json'); // will throw

--
Thank you and best regards,
Eugene Leonovich

If you do think that ideally an exception should be thrown here, it is better to devise a new function—and, while you’re at it, a function that returns an object of some definite class instead of a resource.

Isn’t this exactly what SplFileObject / SplFileInfo::openFile is for?

Isn’t this exactly what SplFileObject / SplFileInfo::openFile is for?

SplFileObject does not expose the underlying filehandle, so it can't
be used with any of the other PHP functions that expect a file handle.

It's not safe to expose the filehandle, as none of the SplFileObject
code checks for the file handle still being valid.

cheers
Dan
Ack