# 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.