Skip to content

Commit 05db035

Browse files
mkasbergsebastianbergmann
authored andcommitted
Fix StringMatchesFormatDescription with \r\n
This is a fix for #3040. There was a bug in that the following assertion would fail, even though it should pass: $this->assertStringMatchesFormat("\r\n", "\r\n"); Obviously, we would expect a string to match itself. The bug was due to newline conversion - `\r\n` was being converted to `\n` on the pattern, but not on the string to be matched against. In order to be compatible with existing behavior, we need to continue converting `\r\n` to `\n` in the pattern. (Otherwise, users who expect this behavior may have tests fail that would have previously passed.) Therefore, the only option for fixing the bug is to also sanitize the string to be matched. This commit makes that change, and provides some additional tests to verify the behavior.
1 parent 978ed6b commit 05db035

File tree

2 files changed

+67
-9
lines changed

2 files changed

+67
-9
lines changed

src/Framework/Constraint/StringMatchesFormatDescription.php

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,22 +26,37 @@ public function __construct(string $string)
2626
{
2727
parent::__construct(
2828
$this->createPatternFromFormat(
29-
\preg_replace('/\r\n/', "\n", $string)
29+
$this->convertNewlines($string)
3030
)
3131
);
3232

3333
$this->string = $string;
3434
}
3535

36+
/**
37+
* Evaluates the constraint for parameter $other. Returns true if the
38+
* constraint is met, false otherwise.
39+
*
40+
* @param mixed $other Value or object to evaluate.
41+
*
42+
* @return bool
43+
*/
44+
protected function matches($other): bool
45+
{
46+
return parent::matches(
47+
$this->convertNewlines($other)
48+
);
49+
}
50+
3651
protected function failureDescription($other): string
3752
{
3853
return 'string matches format description';
3954
}
4055

4156
protected function additionalFailureDescription($other): string
4257
{
43-
$from = \preg_split('(\r\n|\r|\n)', $this->string);
44-
$to = \preg_split('(\r\n|\r|\n)', $other);
58+
$from = \explode("\n", $this->string);
59+
$to = \explode("\n", $this->convertNewlines($other));
4560

4661
foreach ($from as $index => $line) {
4762
if (isset($to[$index]) && $line !== $to[$index]) {
@@ -97,4 +112,9 @@ private function createPatternFromFormat(string $string): string
97112

98113
return '/^' . $string . '$/s';
99114
}
115+
116+
private function convertNewlines($text): string
117+
{
118+
return \preg_replace('/\r\n/', "\n", $text);
119+
}
100120
}

tests/Framework/Constraint/StringMatchesFormatDescriptionTest.php

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,11 @@
1010

1111
namespace PHPUnit\Framework\Constraint;
1212

13+
use PHPUnit\Framework\ExpectationFailedException;
14+
1315
class StringMatchesFormatDescriptionTest extends ConstraintTestCase
1416
{
15-
public function testConstraintStringMatches(): void
17+
public function testConstraintStringMatchesCharacter(): void
1618
{
1719
$constraint = new StringMatchesFormatDescription('*%c*');
1820

@@ -22,7 +24,7 @@ public function testConstraintStringMatches(): void
2224
$this->assertCount(1, $constraint);
2325
}
2426

25-
public function testConstraintStringMatches2(): void
27+
public function testConstraintStringMatchesString(): void
2628
{
2729
$constraint = new StringMatchesFormatDescription('*%s*');
2830

@@ -32,7 +34,7 @@ public function testConstraintStringMatches2(): void
3234
$this->assertCount(1, $constraint);
3335
}
3436

35-
public function testConstraintStringMatches3(): void
37+
public function testConstraintStringMatchesInteger(): void
3638
{
3739
$constraint = new StringMatchesFormatDescription('*%i*');
3840

@@ -42,7 +44,7 @@ public function testConstraintStringMatches3(): void
4244
$this->assertCount(1, $constraint);
4345
}
4446

45-
public function testConstraintStringMatches4(): void
47+
public function testConstraintStringMatchesUnsignedInt(): void
4648
{
4749
$constraint = new StringMatchesFormatDescription('*%d*');
4850

@@ -52,7 +54,7 @@ public function testConstraintStringMatches4(): void
5254
$this->assertCount(1, $constraint);
5355
}
5456

55-
public function testConstraintStringMatches5(): void
57+
public function testConstraintStringMatchesHexadecimal(): void
5658
{
5759
$constraint = new StringMatchesFormatDescription('*%x*');
5860

@@ -62,7 +64,7 @@ public function testConstraintStringMatches5(): void
6264
$this->assertCount(1, $constraint);
6365
}
6466

65-
public function testConstraintStringMatches6(): void
67+
public function testConstraintStringMatchesFloat(): void
6668
{
6769
$constraint = new StringMatchesFormatDescription('*%f*');
6870

@@ -71,4 +73,40 @@ public function testConstraintStringMatches6(): void
7173
$this->assertEquals('matches PCRE pattern "/^\*[+-]?\.?\d+\.?\d*(?:[Ee][+-]?\d+)?\*$/s"', $constraint->toString());
7274
$this->assertCount(1, $constraint);
7375
}
76+
77+
public function testConstraintStringMatchesNewline(): void
78+
{
79+
$constraint = new StringMatchesFormatDescription("\r\n");
80+
81+
$this->assertFalse($constraint->evaluate("*\r\n", '', true));
82+
$this->assertTrue($constraint->evaluate("\r\n", '', true));
83+
$this->assertEquals("matches PCRE pattern \"/^\n$/s\"", $constraint->toString());
84+
$this->assertCount(1, $constraint);
85+
}
86+
87+
public function testFailureMessageWithNewlines(): void
88+
{
89+
$constraint = new StringMatchesFormatDescription("%c\nfoo\n%c");
90+
91+
try
92+
{
93+
$constraint->evaluate("*\nbar\n*");
94+
$this->fail('Expected ExpectationFailedException, but it was not thrown.');
95+
}
96+
catch (ExpectationFailedException $e)
97+
{
98+
$expected = <<<EOD
99+
Failed asserting that string matches format description.
100+
--- Expected
101+
+++ Actual
102+
@@ @@
103+
*
104+
-foo
105+
+bar
106+
*
107+
108+
EOD;
109+
$this->assertEquals($expected, $e->getMessage());
110+
}
111+
}
74112
}

0 commit comments

Comments
 (0)