Skip to content

Navigation Menu

Sign in
Appearance settings
Sign in
Sign up
Appearance settings

Commit 0cb81e0

Browse filesBrowse the repository at this point in the historyBrowse files
nicolas-grekasnicolas-grekas
committed
bug symfony#65621 [PropertyInfo] Do not prefer a static named constructor as the property mutator (nicolas-grekas)
This PR was merged into the 6.4 branch. Discussion ---------- [PropertyInfo] Do not prefer a static named constructor as the property mutator | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT Write-side follow-up to symfony#65619. `getWriteInfo()` selects the jQuery-style bare mutator `foo($value)` (only when `enable_getter_setter_extraction` is on, which PropertyAccess always enables) before the property itself and the magic methods, without checking `isStatic()`. A static method named after the property that accepts one argument, like a named constructor: ```php class Money { public int $value = 0; public static function value(int $value): self { $money = new self(); $money->value = $value; return $money; } } ``` is picked as the mutator for `value`. A static method cannot mutate the instance and PropertyAccess discards its return value, so `$propertyAccessor->setValue($money, 'value', 42)` calls the factory, throws away the result and leaves `$money->value` untouched: the write is silently lost while a writable public property sits right there. This patch demotes a static method named after the property behind every instance-based candidate: the prefixed mutators, the property itself, `__set()` and `__call()`. It remains a valid last-resort mutator, so a class exposing only such a static method keeps resolving to it and `isWritable()` still reports true; a static mutator can legitimately update static state. Prefixed mutators (`set`, `add`/`remove`) may still resolve to a static method as before. Resulting write priority for property `foo` with `enable_getter_setter_extraction` on: constructor parameter, adder/remover pair, `setFoo()` and the other prefixed mutators, `foo($value)` if non-static, the `foo` property, `__set()`, `__call()`, and finally `foo($value)` if static. With this and symfony#65619, the read and write paths agree again on the overlap case: a public property shadowed by a same-named static method is both read from and written to directly. Commits ------- 4bed9af [PropertyInfo] Do not prefer a static named constructor as the property mutator
2 parents 757b06e + 4bed9af commit 0cb81e0
Copy full SHA for 0cb81e0

3 files changed

+87-2Lines changed: 87 additions & 2 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php‎

Copy file name to clipboardExpand all lines: src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php
+19-2Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -385,12 +385,19 @@ public function getWriteInfo(string $class, string $property, array $context = [
385385
$getsetter = lcfirst($camelized);
386386
$getsetterNonCamelized = lcfirst($nonCamelized);
387387

388+
$staticGetsetter = null;
389+
388390
if ($allowGetterSetter) {
389391
[$accessible, $methodAccessibleErrors] = $this->isMethodAccessible($reflClass, $getsetter, 1);
390392
if ($accessible) {
391393
$method = $reflClass->getMethod($getsetter);
392394

393-
return new PropertyWriteInfo(PropertyWriteInfo::TYPE_METHOD, $getsetter, $this->getWriteVisibilityForMethod($method), $method->isStatic());
395+
if (!$method->isStatic()) {
396+
return new PropertyWriteInfo(PropertyWriteInfo::TYPE_METHOD, $getsetter, $this->getWriteVisibilityForMethod($method), false);
397+
}
398+
399+
// a static method named after the property, e.g. a named constructor, must not win over any instance-based candidate; try it last
400+
$staticGetsetter = [$getsetter, $method];
394401
}
395402

396403
$errors[] = $methodAccessibleErrors;
@@ -400,7 +407,11 @@ public function getWriteInfo(string $class, string $property, array $context = [
400407
if ($accessible) {
401408
$method = $reflClass->getMethod($getsetterNonCamelized);
402409

403-
return new PropertyWriteInfo(PropertyWriteInfo::TYPE_METHOD, $getsetterNonCamelized, $this->getWriteVisibilityForMethod($method), $method->isStatic());
410+
if (!$method->isStatic()) {
411+
return new PropertyWriteInfo(PropertyWriteInfo::TYPE_METHOD, $getsetterNonCamelized, $this->getWriteVisibilityForMethod($method), false);
412+
}
413+
414+
$staticGetsetter ??= [$getsetterNonCamelized, $method];
404415
}
405416
$errors[] = $methodAccessibleErrors;
406417
}
@@ -444,6 +455,12 @@ public function getWriteInfo(string $class, string $property, array $context = [
444455
)];
445456
}
446457

458+
if ($staticGetsetter) {
459+
[$methodName, $method] = $staticGetsetter;
460+
461+
return new PropertyWriteInfo(PropertyWriteInfo::TYPE_METHOD, $methodName, $this->getWriteVisibilityForMethod($method), true);
462+
}
463+
447464
$noneProperty = new PropertyWriteInfo();
448465
$noneProperty->setErrors(array_merge([], ...$errors));
449466

Collapse file

‎src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php‎

Copy file name to clipboardExpand all lines: src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php
+26Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
use Symfony\Component\PropertyInfo\Tests\Fixtures\DummyWithHasser;
2424
use Symfony\Component\PropertyInfo\Tests\Fixtures\DummyWithNonAsciiStaticAccessor;
2525
use Symfony\Component\PropertyInfo\Tests\Fixtures\DummyWithStaticConstructorAndAccessor;
26+
use Symfony\Component\PropertyInfo\Tests\Fixtures\DummyWithStaticMutator;
2627
use Symfony\Component\PropertyInfo\Tests\Fixtures\MultiParameterAdderDummy;
2728
use Symfony\Component\PropertyInfo\Tests\Fixtures\MultiParameterAdderParentDummy;
2829
use Symfony\Component\PropertyInfo\Tests\Fixtures\MultiParameterAdderValue;
@@ -728,6 +729,31 @@ public function testDisabledAdderAndRemoverReturnsError()
728729
self::assertSame([\sprintf('The property "baz" in class "%s" can be defined with the methods "addBaz()", "removeBaz()" but the new value must be an array or an instance of \Traversable', Php71Dummy::class)], $writeMutator->getErrors());
729730
}
730731

732+
public function testGetWriteMutatorPrefersTheSetterOverTheStaticMethodNamedAfterTheProperty()
733+
{
734+
$writeMutator = $this->extractor->getWriteInfo(DummyWithStaticMutator::class, 'quantity', ['enable_getter_setter_extraction' => true]);
735+
736+
$this->assertSame(PropertyWriteInfo::TYPE_METHOD, $writeMutator->getType());
737+
$this->assertSame('setQuantity', $writeMutator->getName());
738+
}
739+
740+
public function testGetWriteMutatorPrefersThePropertyOverTheStaticMethodNamedAfterIt()
741+
{
742+
$writeMutator = $this->extractor->getWriteInfo(DummyWithStaticMutator::class, 'value', ['enable_getter_setter_extraction' => true]);
743+
744+
$this->assertSame(PropertyWriteInfo::TYPE_PROPERTY, $writeMutator->getType());
745+
$this->assertSame('value', $writeMutator->getName());
746+
}
747+
748+
public function testGetWriteMutatorTriesTheStaticMethodNamedAfterThePropertyLast()
749+
{
750+
$writeMutator = $this->extractor->getWriteInfo(DummyWithStaticMutator::class, 'amount', ['enable_getter_setter_extraction' => true]);
751+
752+
$this->assertSame(PropertyWriteInfo::TYPE_METHOD, $writeMutator->getType());
753+
$this->assertSame('amount', $writeMutator->getName());
754+
$this->assertTrue($writeMutator->isStatic());
755+
}
756+
731757
public function testGetWriteInfoReadonlyProperties()
732758
{
733759
$writeMutatorConstructor = $this->extractor->getWriteInfo(Php81Dummy::class, 'foo', ['enable_constructor_extraction' => true]);
Collapse file

‎src/Symfony/Component/PropertyInfo/Tests/Fixtures/DummyWithStaticMutator.php‎

Copy file name to clipboard
+42Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\PropertyInfo\Tests\Fixtures;
13+
14+
class DummyWithStaticMutator
15+
{
16+
public int $value = 0;
17+
18+
private int $quantity = 0;
19+
20+
public static function value(int $value): self
21+
{
22+
$dummy = new self();
23+
$dummy->value = $value;
24+
25+
return $dummy;
26+
}
27+
28+
public static function quantity(int $quantity): self
29+
{
30+
return self::value($quantity);
31+
}
32+
33+
public static function amount(int $amount): self
34+
{
35+
return self::value($amount);
36+
}
37+
38+
public function setQuantity(int $quantity): void
39+
{
40+
$this->quantity = $quantity;
41+
}
42+
}

0 commit comments

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