Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:107606
MIME-Version: 1.0
References: <CAJkq-zzRaVYVvQ_pJbKu1vGNTQSTZ_pFF6yzhVrDF4N-M8NbiA@mail.gmail.com>
In-Reply-To: <CAJkq-zzRaVYVvQ_pJbKu1vGNTQSTZ_pFF6yzhVrDF4N-M8NbiA@mail.gmail.com>
Date: Mon, 21 Oct 2019 17:41:21 +0200
Message-ID: <CAF+90c-Vd8iB7eyVrK6XNdZm+QwMZ2fXRPt5SOgpKPtVa6mL6Q@mail.gmail.com>
To: David Negrier <d.negrier@thecodingmachine.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000e6f86805956d84a2"
Subject: Re: [PHP-DEV] Reclassifying some PHP functions warning as exceptions
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000e6f86805956d84a2
Content-Type: text/plain; charset="UTF-8"

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

--000000000000e6f86805956d84a2--