Everything

php

Missing warning for stats(), etc. when invoked with empty string

php.internals

Claude Pache

5 years ago

Hi,

`stats()` and friends invoked with empty string or null as argument, return `false` (indicating failure), but do not emit the Warning that is expected on failure. See: https://3v4l.org/jXC2N <https://3v4l.org/jXC2N>

That discrepancy between the two ways of indicating failure is problematic. Indeed, whenever I use one of those functions without prefixing it by @, I expect that it will either return a value of expected type (`array` in case of `stats()`), or run the panic procedure of my error handler.

I propose to ensure that those functions always emit a Warning in each and every situation they return false/null (provided, of course, that those functions are not supposed to return `false/null` in absence of failure, and that their purpose is not to check failure condition as in `file_exists()`).

—Claude

Claude Pache

5 years ago

[Of course, I meant `stat()`, not `stats()`. Resending the message with correct spelling, including in subject line. Sorry.]

Hi,

`stat()` and friends invoked with empty string or null as argument, return `false` (indicating failure), but do not emit the Warning that is expected on failure. See: https://3v4l.org/jXC2N <https://3v4l.org/jXC2N>

That discrepancy between the two ways of indicating failure is problematic. Indeed, whenever I use one of those functions without prefixing it by @, I expect that it will either return a value of expected type (`array` in case of `stat()`), or run the panic procedure of my error handler.

I propose to ensure that those functions always emit a Warning in each and every situation they return false/null (provided, of course, that those functions are not supposed to return `false/null` in absence of failure, and that their purpose is not to check for failure condition as in `file_exists()`).

—Claude

Unnamed Person

5 years ago

On Thu, Dec 3, 2020 at 6:30 AM Claude Pache <claude.pache@gmail.com> wrote:

>
> [Of course, I meant `stat()`, not `stats()`. Resending the message with correct spelling, including in subject line. Sorry.]
>
>
> Hi,
>
> `stat()` and friends invoked with empty string or null as argument, return `false` (indicating failure), but do not emit the Warning that is expected on failure. See: https://3v4l.org/jXC2N <https://3v4l.org/jXC2N>
>
> That discrepancy between the two ways of indicating failure is problematic. Indeed, whenever I use one of those functions without prefixing it by @, I expect that it will either return a value of expected type (`array` in case of `stat()`), or run the panic procedure of my error handler.
>
> I propose to ensure that those functions always emit a Warning in each and every situation they return false/null (provided, of course, that those functions are not supposed to return `false/null` in absence of failure, and that their purpose is not to check for failure condition as in `file_exists()`).
>
> —Claude
>

This is a long-standing behavior. Further, I don't know many people
who want _more_ warnings in their codebase.

In other words, I disagree with this proposal.

Nikita Popov

5 years ago

On Thu, Dec 3, 2020 at 10:06 PM Levi Morrison via internals <
internals@lists.php.net> wrote:

> On Thu, Dec 3, 2020 at 6:30 AM Claude Pache <claude.pache@gmail.com>
> wrote:
> >
> > [Of course, I meant `stat()`, not `stats()`. Resending the message with
> correct spelling, including in subject line. Sorry.]
> >
> >
> > Hi,
> >
> > `stat()` and friends invoked with empty string or null as argument,
> return `false` (indicating failure), but do not emit the Warning that is
> expected on failure. See: https://3v4l.org/jXC2N <https://3v4l.org/jXC2N>
> >
> > That discrepancy between the two ways of indicating failure is
> problematic. Indeed, whenever I use one of those functions without
> prefixing it by @, I expect that it will either return a value of expected
> type (`array` in case of `stat()`), or run the panic procedure of my error
> handler.
> >
> > I propose to ensure that those functions always emit a Warning in each
> and every situation they return false/null (provided, of course, that those
> functions are not supposed to return `false/null` in absence of failure,
> and that their purpose is not to check for failure condition as in
> `file_exists()`).
> >
> > —Claude
> >
>
> This is a long-standing behavior. Further, I don't know many people
> who want _more_ warnings in their codebase.
>
> In other words, I disagree with this proposal.

This seems like a pretty odd position to take. I think there's two ways it
can reasonably behave:

1. Always generate a warning if stat() fails.
2. Never generate a warning if stat() fails.

While the current option is:

3. Always generate a warning if stat() fails ... unless an empty string was
passed.

I'm okay with either 1. or 2., but I don't think that 3. constitutes
sensible behavior.

Regards,
Nikita

Christian Schneider

5 years ago

Am 03.12.2020 um 22:13 schrieb Nikita Popov <nikita.ppv@gmail.com>:

> On Thu, Dec 3, 2020 at 10:06 PM Levi Morrison via internals <
> internals@lists.php.net <mailto:internals@lists.php.net>> wrote:
>> This is a long-standing behavior. Further, I don't know many people
>> who want _more_ warnings in their codebase.
>>
>> In other words, I disagree with this proposal.
>
>
> This seems like a pretty odd position to take. I think there's two ways it
> can reasonably behave:
>
> 1. Always generate a warning if stat() fails.
> 2. Never generate a warning if stat() fails.
>
> While the current option is:
>
> 3. Always generate a warning if stat() fails ... unless an empty string was
> passed.
>
> I'm okay with either 1. or 2., but I don't think that 3. constitutes
> sensible behavior.

Wel,, there is actually a third case: null (which is also the reason for the empty string behavior)

Some people might use it like "I have a filename but there is no such file, so stat() should issue a warning because something unexpected happened" and "I have no filename so the warning is not necessary".

So 3 might not be completely useless. That being said: I'm not against changing 3 to 1, I just wanted to point out the null-case.

- Chris

Claude Pache

5 years ago

> Le 3 déc. 2020 à 22:05, Levi Morrison <levi.morrison@datadoghq.com> a écrit :
>
> This is a long-standing behavior. Further, I don't know many people
> who want _more_ warnings in their codebase.

My guess is that this position was already discussed in the occasion of the several past RFC whose main purpose was to introduce more warnings/errors/exceptions? [1], [2], [3], etc. Apparently, many people do want more warnings...

But my main point is not exactly that. It is inconsistency in behaviour, that leads to wrong assumptions, that leads to incorrect code. As another example, see the faulty `@is_file( $data ) === false` check mentioned in the other thread [4].

Precisely (and I realise that maybe I wasn’t explicit enough in my message), the issue is that `stat("non/existent/file")` raises a warning and returns false, that leads to the wrong assumption that `stat($random_string)` raises a warning whenever it returns `false` instead of an array, that leads to the deceptive confidence that the `false` case will be always handled by the error handler.

—Claude

[1]: https://wiki.php.net/rfc/counting_non_countables <https://wiki.php.net/rfc/counting_non_countables>
[2]: https://wiki.php.net/rfc/notice-for-non-valid-array-container <https://wiki.php.net/rfc/notice-for-non-valid-array-container>
[3]: https://wiki.php.net/rfc/magic-methods-signature <https://wiki.php.net/rfc/magic-methods-signature>

[4]: https://externals.io/message/112333#112350 <https://externals.io/message/112333#112350>

Nikita Popov

5 years ago

On Fri, Dec 4, 2020 at 9:33 AM Claude Pache <claude.pache@gmail.com> wrote:

>
>
> > Le 3 déc. 2020 à 22:05, Levi Morrison <levi.morrison@datadoghq.com> a
> écrit :
> >
> > This is a long-standing behavior. Further, I don't know many people
> > who want _more_ warnings in their codebase.
>
>
> My guess is that this position was already discussed in the occasion of
> the several past RFC whose main purpose was to introduce more
> warnings/errors/exceptions? [1], [2], [3], etc. Apparently, many people do
> want more warnings...
>

This is getting a bit off-topic, but I think it's necessary to clarify:
What many of us actually want is *less* warnings. Having less warnings can
take the form of converting them to exceptions, or take the form of
removing them entirely. Both of these things happen, but the conversions to
exceptions tend to be the more controversial and thus take the spotlight.

I'm personally not a fan of warnings thrown by stream/FS functions
(especially when backed by socket streams), as robust code just ends up
having to suppress them all. Unfortunately I can't say that these warnings
are entirely useless, as they *are* helpful for debugging. It would be nice
if PHP had a stronger separation between "this is an error" warnings and
"this is informational" warnings. Things are slowly moving in this
direction with the first category being partially converted into
exceptions...

But well, that's a global problem. The current stance of stream/FS
functions is very much that all error conditions should warn.

Regards,
Nikita

Christoph Becker

5 years ago

On 03.12.2020 at 11:20, Claude Pache wrote:

> `stats()` and friends invoked with empty string or null as argument, return `false` (indicating failure), but do not emit the Warning that is expected on failure. See: https://3v4l.org/jXC2N <https://3v4l.org/jXC2N>
>
> That discrepancy between the two ways of indicating failure is problematic. Indeed, whenever I use one of those functions without prefixing it by @, I expect that it will either return a value of expected type (`array` in case of `stats()`), or run the panic procedure of my error handler.
>
> I propose to ensure that those functions always emit a Warning in each and every situation they return false/null (provided, of course, that those functions are not supposed to return `false/null` in absence of failure, and that their purpose is not to check failure condition as in `file_exists()`).

Returning false without raising a warning has been introduced[1] to fix
bug #21410[2].  The problem back then was that without that early
return, on Windows the empty string gave erroneous stat() results, and
filetype("") returned "dir".  This is no longer the case at least as of
PHP 7.3.0 (I didn't test older versions); instead after reverting
560e33968d I now get:

Warning: stat(): stat failed for  in %s on line %d
bool(false)

So, yes, I think we should revert that commit.

[1] <https://github.com/php/php-src/commit/560e33968d>
[2] <https://bugs.php.net/21410>

Regards,
Christoph