8000 minor #59995 [Config] Make ifFalse() consistent between value and clo… · soyuka/symfony@4c672ef · GitHub
[go: up one dir, main page]
Skip to content

Commit 4c672ef

Browse files
nicolas-grekasnicolas-grekas
committed
minor symfony#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 symfony#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

File tree

2 files changed

+28
-4
lines changed

2 files changed

+28
-4
lines changed

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

Lines changed: 1 addition & 1 deletion
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

Lines changed: 27 additions & 3 deletions
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