webmozarts-assert/PR.md

# Make Assert::regex() report PCRE failures instead of a bogus mismatch

Fixes #119 (one instance of it -- see "Scope" below).

## Problem

`Assert::regex()` treats the return value of `preg_match()` as a boolean. `preg_match()`
has three outcomes, not two: `1` (match), `0` (no match) and `false` (the engine could not
run the match at all). Folding `false` into the `0` branch produces two bad behaviours.

**1. A raw PHP warning escapes from inside the library, followed by a misleading exception.**

```php
Assert::regex('abc', '/(/');
```

on PHP 8.3.6 before this change:

```
PHP Warning:  preg_match(): Compilation failed: missing closing parenthesis at offset 1 in .../src/Assert.php on line 1510
Webmozart\Assert\InvalidArgumentException: The value "abc" does not match the expected pattern.
```

The warning is emitted from library code the caller does not control, and the exception
then blames the value for what is actually a broken pattern.

**2. A PCRE engine failure is silently reclassified as a failed assertion.**

With `pcre.backtrack_limit=100`:

```php
Assert::regex(str_repeat('a', 30).'c', '/^(a+)+$/');
// Webmozart\Assert\InvalidArgumentException: The value "aaaa...c" does not match the expected pattern.
// preg_last_error_msg() === 'Backtrack limit exhausted'
```

Here `preg_match()` returned `false` because it hit the backtrack limit -- it never
determined whether the value matches. The caller is told the value failed validation.
That is the more dangerous of the two: on a large input, a resource limit becomes an
indistinguishable "invalid input" error, and the real cause is invisible.

The same masking happens for `PREG_BAD_UTF8_ERROR`, `PREG_JIT_STACKLIMIT_ERROR`, and so on.

## Root cause

`src/Assert.php:1510`

```php
if (!\preg_match($pattern, $value)) {
```

`!false` and `!0` are both `true`, so the "engine failed" and "value did not match" cases
are indistinguishable, and nothing suppresses or reports the warning `preg_match()` raises
on its way out.

## Solution

Separate the three outcomes, mirroring how this repository already handles a native
function that can return `false` (`Assert::strlen()`, `src/Assert.php:2590`:
`if (false === $encoding = \mb_detect_encoding($value)) {`).

```php
if (false === $result = @\preg_match($pattern, $value)) {
    static::reportInvalidArgument(\sprintf(
        'The pattern %s could not be evaluated: %s.',
        static::valueToString($pattern),
        \preg_last_error_msg()
    ));
}

if (0 === $result) {
    // unchanged: the existing mismatch message, including the caller's custom message
}
```

Now:

```
Assert::regex('abc', '/(/');
  -> InvalidArgumentException: The pattern "/(/" could not be evaluated: Internal error.
     (no PHP warning)

Assert::regex(str_repeat('a', 30).'c', '/^(a+)+$/');   // pcre.backtrack_limit=100
  -> InvalidArgumentException: The pattern "/^(a+)+$/" could not be evaluated: Backtrack limit exhausted.
```

Notes on the two deliberate choices here:

- **The caller's custom `$message` is not applied to the engine-failure branch.** A custom
  message such as `'The value %s is not a valid slug.'` describes a mismatch. Reusing it for
  a pattern that never compiled would preserve exactly the confusion this change removes.
- **The diagnostic comes from `preg_last_error_msg()` rather than from the raised warning
  text.** Capturing the warning text ("Compilation failed: missing closing parenthesis at
  offset 1") would require `set_error_handler()`/`restore_error_handler()`, and both are
  listed in Psalm's `dictionaries/ImpureFunctionsList.php`, so calling them inside a method
  annotated `@psalm-pure` is the wrong trade. `preg_match()` and `preg_last_error_msg()` are
  not in that list.

Behaviour on the normal paths is unchanged: a matching value still returns the value, and a
genuine mismatch still throws with the existing default or custom message. No public
signature changes, so no BC break.

## Scope

Issue #119 is broadly worded ("Some assertions call functions that may generate a warning
... e.g. preg_match, strlen"). **This PR fixes only `regex()`**, which is one instance of
that class of problem. I did not extend it to `length()`, `contains()` or the `count*`
assertions, because I have no reproduction showing a defect there, and guessing at those
would make the change harder to review.

One sibling is worth flagging explicitly, though I left it out to keep this PR to a single
behaviour change. `notRegex()` (`src/Assert.php:1533` before this change) has the same root
shape with the opposite and arguably worse symptom -- because it checks the truthiness of
`preg_match()` directly, a pattern that fails to compile makes the assertion silently
**pass**:

```php
Assert::notRegex('abc', '/(/');   // emits a PHP warning, then returns 'abc' successfully
```

Happy to fold `notRegex()` into this PR, or file it separately, whichever you prefer.

## How this was tested

Environment: PHP 8.3.6, Composer 2.10.2, on commit `2ccb7c2` ("Prepare for 2.4.1 release").

Four tests were added to `tests/AssertTest.php`, following the existing standalone-test
convention (`testResourceOfTypeCustomMessage`, `testEnumAssertionErrorMessage`):

- `testRegexRejectsAnUncompilablePattern`
- `testRegexSuppressesTheWarningForAnUncompilablePattern`
- `testRegexDoesNotUseTheCustomMessageForAnUncompilablePattern`
- `testRegexRejectsAPatternExceedingTheBacktrackLimit`

**Each new test fails without the source change and passes with it.**

With `src/Assert.php` reverted to its original state and only the new tests applied:

```
$ ./tools/phpunit/vendor/bin/phpunit --filter 'testRegex'
1) Webmozart\Assert\Tests\AssertTest::testRegexRejectsAnUncompilablePattern
2) Webmozart\Assert\Tests\AssertTest::testRegexSuppressesTheWarningForAnUncompilablePattern
3) Webmozart\Assert\Tests\AssertTest::testRegexDoesNotUseTheCustomMessageForAnUncompilablePattern
4) Webmozart\Assert\Tests\AssertTest::testRegexRejectsAPatternExceedingTheBacktrackLimit
FAILURES!
Tests: 4, Assertions: 6, Failures: 4, Warnings: 1.
```

With the fix applied:

```
$ ./tools/phpunit/vendor/bin/phpunit --filter 'testRegex'
OK (4 tests, 6 assertions)
```

Full suite before the change:

```
$ composer install && composer --working-dir=tools/phpunit install && ./tools/phpunit/vendor/bin/phpunit
Tests: 4906, Assertions: 5823, Notices: 2, Skipped: 56.
```

Full suite after the change:

```
$ ./tools/phpunit/vendor/bin/phpunit
Tests: 4910, Assertions: 5829, Notices: 2, Skipped: 56.
```

Same 2 notices and 56 skips in both runs -- they are pre-existing on a clean checkout
(the notices originate from `tests/AssertTest.php:618`) and are unrelated to this change.
The 4 added tests account for the entire delta.

One caveat on local verification: `composer run cs-check` and `composer run static-analysis`
could **not** be run on this machine. `friendsofphp/php-cs-fixer v3.92.3` requires PHP >= 8.4
via `symfony/console v8.0.3`, and `vimeo/psalm 6.14.3` refuses to start with
`Psalm requires a PHP version ">= 8.3.16". You are running 8.3.6.` The added code was instead
matched by hand to the conventions already in the tree: Yoda comparisons (`false === $x`, as
in `src/Assert.php:829/897/2590`), global classes imported rather than written with a leading
backslash (as with `LogicException`, `Error`, `Exception` in `tests/AssertTest.php`), and
concatenation without surrounding spaces. CI will be the real check on both of those gates.