Hi Tim, > I regretfully were not able to work through the list backlog after my > summer vacation and thus also missed the intent to vote. I have just > read through the RFC and voted against it, despite being in agreement of > the general concept.
No worries at all. I'd already read the quiet on the thread as people being busy rather than as sign-off, so I'm glad it came when it did. I'd rather get this right than get it fast. > Specifically: > > 1. I disagree with keeping the Warning on compilation errors. This > feature is entirely new and opt-in, thus there are no backwards > compatibility expectations or considerations. The `$e->getMessage() === > preg_last_error_msg()` guarantee makes the feature much worse than it > could be for compilation errors. Including all necessary information in > the Exception is a must for me. On your first point, I think I can safely claim that I understand exactly where you're coming from, because what you're describing was what I actually chose first. What moved me off it was a single decision that I had to make later when I faced the array case[^1] about whether this flag should add error semantics of its own, or only deliver the error the call already recorded via an exception. I chose the second, and the reasons for the warning/error messages case were: 1. The detailed compile message isn't in the error state to begin with. When a pattern fails to compile, the error from `pcre2_get_error_message()` only ever goes into the `E_WARNING` and is then thrown away. To put that detail in the exception, we must store it somewhere first. 2. It would make the flag the one place in ext/pcre that knows more about the error than `preg_last_error_msg()` does. 3. The better fix, which resolves the two issues above, is to repair the anemic message at its source, in `preg_last_error_msg()` itself, store the real reason in the error state for compile errors, and the exception just inherits it, with no new flag behavior at all. But that would widen the scope of the RFC from just a flag that throws an exception, to also changing the message returned by `preg_last_error_msg()`. For those reasons, I chose the `$e->getMessage() === preg_last_error_msg()` guarantee to keep things consistent, and I chose to push changing the `preg_last_error_msg()` error message into future work. To me that's also the better separation of concerns. Letting the exception report exactly what `preg_last_error_msg()` reports buys two things: 1. Consistency: one error, one message, whether you read it from the exception or from the function. 2. The anemic message gets fixed where it actually originates. Because if `$e->getMessage()` returning `"Internal error"` is a problem, then `preg_last_error_msg()` returning `"Internal error"` is the same problem, and it's worth fixing there rather than papering over it on the exception alone. So while I agree all the necessary information should typically be in the exception, I don't want to get there by breaking the `$e->getMessage() === preg_last_error_msg()` guarantee. Now what I would suggest instead of breaking that guarantee, is to pull enriching the anemic `preg_last_error_msg()` error message forward into this RFC instead of leaving it for later, store the real reason in the error state, and the exception inherits it through the very same channel, with the guarantee intact. I know this is arguably its own debate, but I am more ok with that than introducing what I think is an inconsistency. > 2. I disagree with the behavior of not wrapping Exceptions thrown in > user callbacks: I believe the correct choice is to throw a > \PregException with the Exception thrown in the callback as the > `->previous` exception. Not wrapping the user callback exception means > that one needs a `catch(Exception)` with a try just around the preg_ > call to reliably handle all errors during regular expression execution, > which nullifies much of the benefit of having a dedicated exception > class in the first place. > > It also violates the exception policy in > https://github.com/php/policies/blob/main/coding-standards-and-naming.rst#throwables , > which states: > > > If an extension uses external functionality that may throw an exception it MUST wrap any exception thrown by that functionality into an appropriate exception of its own. It MUST set the $previous property to the original exception when doing so. On your second point, if this is a violation of a policy, then there isn't much to argue. I will just retract the vote and fix that. But I think I got confused here, and I would appreciate you explaining how that violates the policy. To make sure we're on the same ground, this is what I understood from your statement about wrapping exceptions thrown in user callbacks: ``` preg_replace_callback( $pattern, fn () => throw new CustomException(), // <- You want this wrapped in PregException? $subject, flags: PREG_THROW_ON_ERROR, ); ``` If I got it right (and I suspect I did), then how does that violate the policy? A user callback isn't external functionality, is it? Because as far as I understand, external functionality is something the extension itself depends on as part of its own implementation. I am also unaware of any functions that behave like that (wraps exceptions thrown in user callbacks in its own exception). In fact, the opposite is the case for one of the precedents this RFC follows (`json_encode()` with `JSON_THROW_ON_ERROR` - although it doesn't accept a user callback): https://3v4l.org/CtHYH#v8.5.10 --- [^1]: https://wiki.php.net/rfc/preg_throw_on_error#arrays Thanks, Osama
