Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 4c672ef

Browse filesBrowse files
nicolas-grekasnicolas-grekas
committed
minor #59995 [Config] Make ifFalse() consistent between value and closure based checks (arjenm)
This PR was squashed before being merged into the 7.3 branch. Discussion ---------- [Config] Make ifFalse() consistent between value and closure based checks | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #59325 | License | MIT I noticed the Config/ExpBuilder got a nice new ifFalse in Symfony 7.3. But I think its implementation is confusing. The behavior for `ifTrue()` was: - If no closure is provided: if the actual value is `true` execute the then part - If a closure is provided: if it returns `true` execute the then part The behavior for `ifFalse()` is prior to this PR is: - If no closure is provided: if the actual value is `false` execute the then part - If a closure is provided: if it returns **`true`** execute the then part With this PR it becomes: - If no closure is provided: if the actual value is `false` execute the then part - If a closure is provided: if it returns **`false`** execute the then part Rationale, I think people (me included) would not expect these to be both be valid or invalid with the same input: `$expr->ifTrue(self::valueIsNotValid(...))->thenInvalid()` `$expr->ifFalse(self::valueIsNotValid(...))->thenInvalid()` They/I expect to have to change it to a test for valid values (rather than invalid ones): `$expr->ifFalse(self::valueIsValid(...))->thenInvalid()` Commits ------- 335bdbe [Config] Make ifFalse() consistent between value and closure based checks
2 parents fd38014 + 335bdbe commit 4c672ef
Copy full SHA for 4c672ef

File tree

2 files changed

+28
-4
lines changed
Filter options

2 files changed

+28
-4
lines changed

‎src/Symfony/Component/Config/Definition/Builder/ExprBuilder.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/Config/Definition/Builder/ExprBuilder.php
+1-1
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ public function ifTrue(?\Closure $closure = null): static
7676
*/
7777
public function ifFalse(?\Closure $closure = null): static
7878
{
79-
$this->ifPart = $closure ?? static fn ($v) => false === $v;
79+
$this->ifPart = $closure ? static fn ($v) => !$closure($v) : static fn ($v) => false === $v;
8080
$this->allowedTypes = self::TYPE_ANY;
8181

8282
return $this;

‎src/Symfony/Component/Config/Tests/Definition/Builder/ExprBuilderTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/Config/Tests/Definition/Builder/ExprBuilderTest.php
+27-3
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,23 @@ public function testIfTrueExpression()
4242
->end();
4343
$this->assertFinalizedValueIs('new_value', $test);
4444

45+
$test = $this->getTestBuilder()
46+
->ifTrue(fn () => 1)
47+
->then($this->returnClosure('new_value'))
48+
->end();
49+
$this->assertFinalizedValueIs('new_value', $test);
50+
4551
$test = $this->getTestBuilder()
4652
->ifTrue(fn () => false)
4753
->then($this->returnClosure('new_value'))
4854
->end();
4955
$this->assertFinalizedValueIs('value', $test);
56+
57+
$test = $this->getTestBuilder()
58+
->ifTrue(fn () => 0)
59+
->then($this->returnClosure('new_value'))
60+
->end();
61+
$this->assertFinalizedValueIs('value', $test);
5062
}
5163

5264
public function testIfFalseExpression()
@@ -58,16 +70,28 @@ public function testIfFalseExpression()
5870
$this->assertFinalizedValueIs('new_value', $test, ['key' => false]);
5971

6072
$test = $this->getTestBuilder()
61-
->ifFalse(fn () => true)
73+
->ifFalse(fn ($v) => 'value' === $v)
6274
->then($this->returnClosure('new_value'))
6375
->end();
64-
$this->assertFinalizedValueIs('new_value', $test);
76+
$this->assertFinalizedValueIs('value', $test);
6577

6678
$test = $this->getTestBuilder()
67-
->ifFalse(fn () => false)
79+
->ifFalse(fn ($v) => 1)
6880
->then($this->returnClosure('new_value'))
6981
->end();
7082
$this->assertFinalizedValueIs('value', $test);
83+
84+
$test = $this->getTestBuilder()
85+
->ifFalse(fn ($v) => 'other_value' === $v)
86+
->then($this->returnClosure('new_value'))
87+
->end();
88+
$this->assertFinalizedValueIs('new_value', $test);
89+
90+
$test = $this->getTestBuilder()
91+
->ifFalse(fn ($v) => 0)
92+
->then($this->returnClosure('new_value'))
93+
->end();
94+
$this->assertFinalizedValueIs('new_value', $test);
7195
}
7296

7397
public function testIfStringExpression()

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.