Hey Jakub,
On 18.11.2025 19:38:37, Jakub Zelenka wrote:
Hello,
I would like to introduce a new stream error handling RFC that is part
of my stream evolution work (PHP Foundation project funded by
Sovereign Tech Fund) :
https://wiki.php.net/rfc/stream_errors
Kind regards,
Jakub
I super appreciate that you've tackled this. This has been a big wart in
PHP's stream handling for a long time.
I have a couple questions:
Why have you chosen a big list of constants rather than enums?
I think we should push for enums where applicable, meaning enum
StreamErrorMode, enum StreamErrorStore, enum StreamError.
You define what a terminal error is, but does it correlate to specific
error codes? Like, is a given error code always terminal or not
terminal? Or can some error codes sometimes be terminal or not terminal
depending on context?
I've tried to look at the code, but I don't quite understand it, e.g.
php_plain_files_rename: When chown or chmod fail, the error is terminal.
When the copying fails (did you forget to adapt php_copy_file_ctx?),
that's non-terminal, though. Reading through the code, I have the
feeling that operations which do not call out to other operations, which
can error, are terminal. And all others are non-terminal. E.g. failing
to open a file is terminal. That the copying where the file opening is
part of, failed, is non-terminal.
And then *additionally* some operations, which don't prevent success are
also marked non-terminal - squeezing it into the boolean for the purpose
of not erroring.
Which makes some sense to me, but the description in the RFC is really
confusing here. And the meanings are muddied.
Thus, if I understand that correctly, there should be an unique mapping
from error code to terminal. If we were to go with enums, we could make
it trivially a property of the individual enum values instead.
Should StreamException be attached the non-terminal errors which are
caused by the terminal errors? I.e. StreamException clearly is
STREAM_ERROR_CODE_PERMISSION_DENIED for example, but it happens during a
copy, so, should the information that a STREAM_ERROR_CODE_COPY_FAILED
was caused by that, also be present on the StreamException?
Are stream errors which happen as part of a fully wrapped functionality
marked as wrapper error as well? Like if the server sent an invalid TLS
response to a file_get_contents("https://...";) operation, making the
whole operation fail? You obviously don't have access to the
intermediary internal stream resource there, where the stream error is
attached.
Is this what the logged errors section is about? Is that section talking
about "php_stream_display_wrapper_errors", which can concat some errors?
I see that this logic is mostly retained.
Do I understand the implementation correctly, that user error handlers
can be called multiple times for a same operation? E.g. when copying a
file, you'll get first a PERMISSION_DENIED then a COPY_FAILED? Should
these not be somehow merged? Especially with global user_error handlers,
these cannot be aware of the fact that the permission denied is part of
anything else - at least not until possibly a second user_error
invocation happens.
Also, is the internal resource used for e.g. file_get_contents() leaked
into user error handlers? I'm not sure whether that's desirable. If
that's intended, that's fine too, I think.
Further suggestion on stream_get_errors() - why not return a proper
object instead of a loosely typed array? Proper typing, and creation is
actually faster. Also "docref" is so... antiquated? maybe just
"function" and have it contain the function name of the current operation.
Does stream_get_errors() ever reset? Looking at the code it doesn't seem
so. It will just forever grow. E.g. fwrite() will report an error every
single time when the buffer is full and a write is attempted. If errors
aren't freed, that's just a memory leak (as long as the socket lives).
This is a serious flaw.
Lastly, stream_get_errors() is overkill in a lot of cases (especially
when using fread()/fwrite() on simple sockets). I just care about the
last error code (there's just going to be one anyway). I'd love having a
stream_get_last_error() returning just the code.
Overall, I like the direction of the RFC, but I'd like to see improved
handling for "terminal error inside non-terminal operation having its
own error" in particular. Enums are rather a suggestion, not a
requirement, but they would be in my opinion very fitting here.
Thanks,
Bob
