Hi, On Thu, Mar 26, 2026 at 5:57 PM Tim Düsterhus <[email protected]> wrote:
> Hi > > thank you for your RFC. I'm regretfully late to the party, but my > primary comment about the RFC relates to the latest changes. > Specifically: > > Am 2026-02-27 23:21, schrieb Jakub Zelenka: > > I changed it to the StreamError class constants and also move the > > is*Error > > functions there. > > The RFC specifically defines “ranges” for the numeric value of the error > constants, some of which are quite small with only 10 values available. > I believe this is dangerous, because it is possible to run out of values > within a specific range and users might make assumptions based on the > ranges when they really should rely on the `is*()` methods to check what > type of error there is. This is not a problem that would exist if the > error code was an unbacked enum. > > Contrary to Nicolas, I also don't think it is a problem for the error > code enum to be extended in future PHP versions. In other programming > languages, Rust specifically, it is also common for errors to be defined > as an enum that is explicitly defined to not be exhaustive. Here's an > example in the standard library: > https://doc.rust-lang.org/std/io/enum.ErrorKind.html. Note how the > documentation says: > > I agree with this and as you pointed out privately we already have precedent in URI extension - enum UrlValidationErrorType: https://github.com/php/php-src/blob/5e45c17d817df003cd24109f0ae222c5d82fecd1/ext/uri/php_uri.stub.php#L112-L143 . The constant were also not user friendly as they were just numbers so we would need some look up function to produce name. Especially painful for logging which is probably the main use case here. I changed it back to enum and this time I made it non backed which was possible to do in a nicer way due to recent changes that Arnaud did for declaration headers containing C enums. It makes the whole implementation much nicer actually. I just extended the gen_stub.php to optionally produce look up table for enum name which should speed up and simplify the look ups. As for the `StreamError` class being a linked list: I agree with the > folks that mentioned that the returned errors should be an array > instead. `count()` would naturally be available, `hasCode()` can be > implemented with `array_any($errors, fn ($error) => $error->code === > CODE_DECODING_FAILED)` and the “primary error” can be obtained with > `array_first()` (which we added in PHP 8.5). > > As you were the second person to ask for it and that linked list was not really a PHP thing, I changed it as suggested. This changed API in more places to accommodate for it but think for logging and other use cases, it will be a bit nicer at the end. > For the naming of `stream_get_last_error()`: Within PHP we have both > `X_get_last_error()` and `X_last_error()`. The latter seems to be more > common and also what I would prefer here, because the `stream_get_` > prefix sounds to me like we would get something from a stream, but the > returned value is not related to a specific stream, but rather a global. > > Good point, I changed it but because it now returns array (no linked list), it's called stream_last_errors(). I also added stream_clear_errors for explicit clearing which might be useful in some situations. The RFC and the implementation is updated so please take a look! Kind regards, Jakub
