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

3 years ago by Claude Pache — view source

unread

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

3 years ago by Claude Pache — view source

unread

[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.

—Claude

3 years ago by Levi Morrison via internals — view source

unread

[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.

3 years ago by Nikita Popov — view source

unread

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:

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

While the current option is:

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

3 years ago by Christian Schneider — view source

unread

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:

Always generate a warning if stat() fails.

Never generate a warning if stat() fails.

While the current option is:

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

3 years ago by Claude Pache — view source

unread

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

3 years ago by Nikita Popov — view source

unread

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

3 years ago by Christoph M. Becker — view source

unread

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