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:
- 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 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:
- 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.
- It would make the flag the one place in ext/pcre that knows more about the error than
preg_last_error_msg() does.
- 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:
- Consistency: one error, one message, whether you read it from the exception or from the function.
- 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.
- 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
Thanks,
Osama