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.

Sign up for GitHub

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

Jump to bottom

[PropertyInfo][ReflectionExtractor] Check the array mutator prefixes last when the property is singular #36305

Merged
merged 1 commit into from Apr 6, 2020
Merged

[PropertyInfo][ReflectionExtractor] Check the array mutator prefixes last when the property is singular #36305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php
View file
Open in desktop
Expand Up @@ -61,6 +61,9 @@ class ReflectionExtractor implements PropertyListExtractorInterface, PropertyTyp
*/
private $arrayMutatorPrefixes;

private $arrayMutatorPrefixesFirst;
Copy link
Contributor Author

@fancyweb fancyweb Apr 1, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need these new class properties if we do some refacto on the existing $mutatorPrefixes and $arrayMutatorPrefixes ones. Basically, currently, $mutatorPrefixes includes the values of $arrayMutatorPrefixes. We could separate the values and join them when we need them.

Copy link
Member

@dunglas dunglas Apr 6, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As these properties are marked @internal, doing the refactoring is ok. I'm in favor for the refactor if it simplifies the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the refacto should target master

private $arrayMutatorPrefixesLast;

/**
* @param string[]|null $mutatorPrefixes
* @param string[]|null $accessorPrefixes
Expand All @@ -72,6 +75,9 @@ public function __construct(array $mutatorPrefixes = null, array $accessorPrefix
$this->mutatorPrefixes = null !== $mutatorPrefixes ? $mutatorPrefixes : self::$defaultMutatorPrefixes;
$this->accessorPrefixes = null !== $accessorPrefixes ? $accessorPrefixes : self::$defaultAccessorPrefixes;
$this->arrayMutatorPrefixes = null !== $arrayMutatorPrefixes ? $arrayMutatorPrefixes : self::$defaultArrayMutatorPrefixes;

$this->arrayMutatorPrefixesFirst = array_merge($this->arrayMutatorPrefixes, array_diff($this->mutatorPrefixes, $this->arrayMutatorPrefixes));
$this->arrayMutatorPrefixesLast = array_reverse($this->arrayMutatorPrefixesFirst);
}

/**
Expand Down Expand Up @@ -330,7 +336,9 @@ private function getMutatorMethod($class, $property)
$ucProperty = ucfirst($property);
$ucSingulars = (array) Inflector::singularize($ucProperty);

foreach ($this->mutatorPrefixes as $prefix) {
$mutatorPrefixes = \in_array($ucProperty, $ucSingulars, true) ? $this->arrayMutatorPrefixesLast : $this->arrayMutatorPrefixesFirst;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the the property singulars contains the property itself, we can assume the property is singular isn'it?


foreach ($mutatorPrefixes as $prefix) {
$names = [$ucProperty];
if (\in_array($prefix, $this->arrayMutatorPrefixes)) {
$names = array_merge($names, $ucSingulars);
Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php
View file
Open in desktop
Expand Up @@ -61,6 +61,7 @@ public function testGetProperties()
'realParent',
'xTotals',
'YT',
'date',
'c',
'd',
'e',
Expand Down Expand Up @@ -96,6 +97,7 @@ public function testGetPropertiesWithCustomPrefixes()
'foo4',
'foo5',
'files',
'date',
'c',
'd',
'e',
Expand Down Expand Up @@ -156,6 +158,8 @@ public function typesProvider()
['staticSetter', null],
['self', [new Type(Type::BUILTIN_TYPE_OBJECT, false, 'Symfony\Component\PropertyInfo\Tests\Fixtures\Dummy')]],
['realParent', [new Type(Type::BUILTIN_TYPE_OBJECT, false, 'Symfony\Component\PropertyInfo\Tests\Fixtures\ParentDummy')]],
['date', [new Type(Type::BUILTIN_TYPE_OBJECT, false, \DateTime::class)]],
['dates', [new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, new Type(Type::BUILTIN_TYPE_INT), new Type(Type::BUILTIN_TYPE_OBJECT, false, \DateTime::class))]],
];
}

Expand Down
8 changes: 8 additions & 0 deletions src/Symfony/Component/PropertyInfo/Tests/Fixtures/Dummy.php
View file
Open in desktop
Expand Up @@ -190,4 +190,12 @@ public function getXTotals()
public function getYT()
{
}

public function setDate(\DateTime $date)
{
}

public function addDate(\DateTime $date)
{
}
}