Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assertStringMatchesFormat() should allow string type (not non-empty-string) for $actual parameter #5459

Closed
mpyw opened this issue Aug 4, 2023 · 2 comments
Assignees
Labels
type/bug Something is broken version/10 Something affects PHPUnit 10

Comments

@mpyw
Copy link

mpyw commented Aug 4, 2023

Q A
PHPUnit version 10.3.0

Summary

I encountered an issue that was introduced with the commit 9f7422c labeled "Narrow types". This change seems to have modified the expected type for certain arguments in the assertStringMatchesFormat() method, from string to non-empty-string.

    /**
     * Asserts that a string matches a given format string.
     *
     * @psalm-param non-empty-string $format
     * @psalm-param non-empty-string $string
     *
     * @throws ExpectationFailedException
     */
    final public static function assertStringMatchesFormat(string $format, string $string, string $message = ''): void
    {
        static::assertThat($string, new StringMatchesFormatDescription($format), $message);
    }

This change has led to the issue where previously valid code now fails. An example of such a scenario is:

$this->assertStringMatchesFormat(
    '%x-%x-%x-%x-%x',
    (new UuidGenerator())->generate(),
);

In this case, unless the generate() method explicitly returns a non-empty-string, this piece of code will fail due to the newly implemented type restriction.

While I understand the intention to ensure meaningful strings are passed into the method, requiring non-empty-string as the type for all $actual parameters might be too restrictive. This could potentially break a significant amount of code that previously ran without issues.

Could you please clarify whether this change was intentional and discuss the possibility of revisiting this requirement? The concept of a non-empty string being a subtype of a string suggests that this change could exclude valid use-cases, particularly when interfacing with methods that do not provide a non-empty-string guarantee but are nevertheless valid.

Current behavior

Fails

 ------ --------------------------------------------------------------- 
  Line   tests/Example.php                                  
 ------ --------------------------------------------------------------- 
  XX     Parameter #2 $string of method                                 
         PHPUnit\Framework\Assert::assertStringMatchesFormat() expects  
         non-empty-string, string given.                                
 ------ --------------------------------------------------------------- 

Expected behavior

Passes

@mpyw mpyw added type/bug Something is broken version/10 Something affects PHPUnit 10 labels Aug 4, 2023
@mpyw
Copy link
Author

mpyw commented Aug 4, 2023

I am certain that the requirement for $actual to be a non-empty-string is a mistake. However, while I understand the reasoning for changing $expected to non-empty-string, I believe such a modification should not be made in a minor version upgrade (like from 10.2 to 10.3), but rather in a major one. This is unquestionably a breaking change.

@sebastianbergmann sebastianbergmann changed the title assertStringMatchesFormat() should allow string type (not non-empty-string) as $actual assertStringMatchesFormat() should allow string type (not non-empty-string) for $actual parameter Aug 4, 2023
@sebastianbergmann sebastianbergmann self-assigned this Aug 4, 2023
@sebastianbergmann
Copy link
Owner

The changes have been reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something is broken version/10 Something affects PHPUnit 10
Projects
None yet
Development

No branches or pull requests

2 participants