Implement #51103: a warning should be displayed if there is a pcre error

[cmb69]

As it's now, preg_*() may silently fail, and the developer would have to
explicitly check preg_last_error(). Raising a warning additionally appears
to be helpful for development and debugging.

As E_WARNING might be regarded too heavy a BC break, E_NOTICE appears to
be reasonable to mitigate BC concerns.

[nikic]

If we do this we should print a descriptive error message instead of just the error code.

However, I don't think this is a good idea, as the PCRE errors are generally data-dependent. Code shouldn't start throwing errors all over the place because (likely user-controlled) data was malformed or exceeded a limit.

[cmb69]

However, I don't think this is a good idea, as the PCRE errors are generally data-dependent. Code shouldn't start throwing errors all over the place because (likely user-controlled) data was malformed or exceeded a limit.

However, the current situation seems to cause a lot of confusion, particularly, wrt. different PHP versions and/or ini settings (there have been several bug reports and at least three FRs, because of that). E_NOTICE or E_WARNING may help developers to be aware that it's prudent to check preg_last_error() (even if that's clumsy).

More descriptive error messages would be something to consider, but I would like that it would also be possible to access these messages from userland (maybe preg_last_errormsg() or preg_error_msg(preg_last_error() should be introduced.)

[ju1ius]

I think @nikic is right: warnings should be raised for developer-dependent errors (regexp compilation, et al.), not for data-related errors.

For example, using preg_match in utf-8 mode, you might have a warning on invalid UTF-8 subject.
So a developer might think "Okay, I need to check data encoding beforehand".
But since there's no way to disable PCRE internal UTF-8 validation, any valid UTF-8 input would now be validated twice !

Of course the best would be to be able to just

try {
  preg_match('/foo/u', $data);
} catch (\PCRE\EncodingException $err) {
  // invalid user data
}

but hey...

[cmb69]

Of course the best would be to be able to just [throw an exception].

I agree, but PHP functions don't (yet) throw exceptions (the only exceptions so far being random_bytes() and random_int() for security reasons).

I also agree that ideally warnings should be raised for developer-dependent errors only, but warnings are the next best thing to work around that we can't throw an exception. Consider, for instance, https://bugs.php.net/70110. In practise, we might find code like that:

if (preg_match('/^(A{1,2}B)+$/', $subject)) {
    // match
} else {
    // no match
}

However, to be able to track errors (which may depend on the system, PHP version, configuration, etc.), the developer would have to write something like:

$res = preg_match('/^(A{1,2}B)+$/', $subject);
if ($res > 0) {
    // match
} elseif ($res === 0) {
    // no match
} else {
    error_log('preg_match error ' . preg_last_error());
}

Of course, that boilerplate could be hidden behind a userland function, but I'm quite sure, most devs won't do that, resulting in rather lengthy debugging sessions.

And finally: if we even would throw an exception, what's the harm in raising a warning?

[nikic]

The harm in raising a warning is that you now need to prepend @ to every preg call, just like you need to do for (racy) filesystem functions etc.

[ju1ius]

The harm in raising a warning is that you now need to prepend @ to every preg call, just like you need to do for (racy) filesystem functions etc.

Absolutely. That would completely kill the performance of everything making heavy use of preg_match.

if we even would throw an exception, what's the harm in raising a warning?

You can't explicitely catch them. And you can't differentiate them.

try {
  preg_match(...$args);
} catch(\PCRE\CompileError $err) {
  // your mistake, let everything blow-up, unit-test it and fix it
  // log it with a CRITICAL level, etc...
} catch (\PCRE\EncodingException $err) {
  // runtime error, fail gracefully, log it, whatever
}

[cmb69]

The harm in raising a warning is that you now need to prepend @ to every preg call, just like you need to do for (racy) filesystem functions etc.

I almost never use the silence operator, because it suppresses useful diagnostic messages (even in the case of racy filesystem functions). I prefer display_errors=0 and maybe to adjust error_reporting on production systems. And frankly, I can't understand why somebody would want to silence preg_* calls, particularly wrt. to ext/pcre's system dependend behavior (see https://3v4l.org/vMXp8#v700).

Anyhow, this PR is obviously controversial, so would require an RFC. I don't consider it important enough to warrant the effort to go through the RFC process, though, so I'm withdrawing the PR. :-)