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.

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:

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

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)) {).

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:

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.