Skip to content

[PropertyInfo] Extract public properties first in ReflectionExtractor #59430

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

Open
wants to merge 1 commit into
base: 7.4
Choose a base branch
from
Open

[PropertyInfo] Extract public properties first in ReflectionExtractor #59430

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 45 additions & 19 deletions src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,29 @@ public function getTypesFromConstructor(string $class, string $property): ?array

public function getType(string $class, string $property, array $context = []): ?Type
{
// BC layer, remove context key in 8.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you update the CHANGELOG.md of PropertyInfo and UPGRADE-7.3.md as well?

$extractPublicPropertiesFirst = \array_key_exists('extract_public_properties_first', $context);
if (!$extractPublicPropertiesFirst) {
trigger_deprecation('symfony/property-info', '7.3', 'Not setting "extract_public_properties_first" context key is deprecated, it will default to "true" in 8.0.');
Comment on lines +211 to +213
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$extractPublicPropertiesFirst = \array_key_exists('extract_public_properties_first', $context);
if (!$extractPublicPropertiesFirst) {
trigger_deprecation('symfony/property-info', '7.3', 'Not setting "extract_public_properties_first" context key is deprecated, it will default to "true" in 8.0.');
$extractPublicPropertiesFirst = $context['extract_public_properties_first'] ?? false;
if (!$extractPublicPropertiesFirst) {
trigger_deprecation('symfony/property-info', '7.3', 'Not setting "extract_public_properties_first" context value to "true" is deprecated, it will always be considered as "true" in 8.0.');

}

if ($extractPublicPropertiesFirst) {
try {
$reflectionClass = new \ReflectionClass($class);
$reflectionProperty = $reflectionClass->getProperty($property);
} catch (\ReflectionException) {
return null;
}

if ($reflectionProperty->isPublic()) {
try {
return $this->typeResolver->resolve($reflectionProperty);
} catch (UnsupportedException $e) {
throw $e;
}
}
}

[$mutatorReflection, $prefix] = $this->getMutatorMethod($class, $property);

if ($mutatorReflection) {
Expand Down Expand Up @@ -240,27 +263,30 @@ public function getType(string $class, string $property, array $context = []): ?
}
}

try {
$reflectionClass = new \ReflectionClass($class);
$reflectionProperty = $reflectionClass->getProperty($property);
} catch (\ReflectionException) {
return null;
}
// BC layer, remove block in 8.0
if (!$extractPublicPropertiesFirst) {
try {
$reflectionClass = new \ReflectionClass($class);
$reflectionProperty = $reflectionClass->getProperty($property);
} catch (\ReflectionException) {
return null;
}

try {
return $this->typeResolver->resolve($reflectionProperty);
} catch (UnsupportedException) {
}
try {
return $this->typeResolver->resolve($reflectionProperty);
} catch (UnsupportedException) {
}

if (null === $defaultValue = ($reflectionClass->getDefaultProperties()[$property] ?? null)) {
return null;
}
if (null === $defaultValue = ($reflectionClass->getDefaultProperties()[$property] ?? null)) {
return null;
}

$typeIdentifier = TypeIdentifier::from(static::MAP_TYPES[\gettype($defaultValue)] ?? \gettype($defaultValue));
$type = 'array' === $typeIdentifier->value ? Type::array() : Type::builtin($typeIdentifier);
$typeIdentifier = TypeIdentifier::from(static::MAP_TYPES[\gettype($defaultValue)] ?? \gettype($defaultValue));
$type = 'array' === $typeIdentifier->value ? Type::array() : Type::builtin($typeIdentifier);

if ($this->isNullableProperty($class, $property)) {
$type = Type::nullable($type);
if ($this->isNullableProperty($class, $property)) {
$type = Type::nullable($type);
}
}

return $type;
Expand Down Expand Up @@ -738,7 +764,7 @@ private function isAllowedProperty(string $class, string $property, bool $writeA
/**
* Gets the accessor method.
*
* Returns an array with a the instance of \ReflectionMethod as first key
* Returns an array with the instance of \ReflectionMethod as first key
* and the prefix of the method as second or null if not found.
*/
private function getAccessorMethod(string $class, string $property): ?array
Expand All @@ -764,7 +790,7 @@ private function getAccessorMethod(string $class, string $property): ?array
}

/**
* Returns an array with a the instance of \ReflectionMethod as first key
* Returns an array with the instance of \ReflectionMethod as first key
* and the prefix of the method as second or null if not found.
*/
private function getMutatorMethod(string $class, string $property): ?array
Expand Down
20 changes: 20 additions & 0 deletions src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1024,4 +1024,24 @@ public static function extractConstructorTypesProvider(): iterable
yield ['dateTime', Type::object(\DateTimeImmutable::class)];
yield ['ddd', null];
}

/**
* @dataProvider extractPropertyOrderProvider
*/
public function testPublicPropertyExtractedFirst(string $property, Type $type)
{
$this->assertEquals(
$type,
$this->extractor->getType('Symfony\Component\PropertyInfo\Tests\Fixtures\PublicPropertyWithHasDummy', $property, ['extract_public_properties_first' => true])
);
}

/**
* @return iterable<array{0: string, 1: Type}>
*/
public static function extractPropertyOrderProvider(): iterable
{
yield ['publicProperty', Type::string()];
yield ['privateProperty', Type::bool()];
}
}
27 changes: 27 additions & 0 deletions src/Symfony/Component/PropertyInfo/Tests/Fixtures/PublicPropertyWithHasDummy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\PropertyInfo\Tests\Fixtures;

class PublicPropertyWithHasDummy
{
public string $publicProperty;

private string $privateProperty;

public function hasPublicProperty(): bool
{
}

public function hasPrivateProperty(): bool
{
}
}
14 changes: 7 additions & 7 deletions src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ public function denormalize(mixed $data, string $type, ?string $format = null, a
}
}

if (null !== $type = $this->getType($resolvedClass, $attribute)) {
if (null !== $type = $this->getType($resolvedClass, $attribute, $context)) {
try {
// BC layer for PropertyTypeExtractorInterface::getTypes().
// Can be removed as soon as PropertyTypeExtractorInterface::getTypes() is removed (8.0).
Expand Down Expand Up @@ -926,7 +926,7 @@ private function validateAndDenormalize(Type $type, string $currentClass, string
*/
protected function denormalizeParameter(\ReflectionClass $class, \ReflectionParameter $parameter, string $parameterName, mixed $parameterData, array $context, ?string $format = null): mixed
{
if ($parameter->isVariadic() || null === $this->propertyTypeExtractor || null === $type = $this->getType($class->getName(), $parameterName)) {
if ($parameter->isVariadic() || null === $this->propertyTypeExtractor || null === $type = $this->getType($class->getName(), $parameterName, $context)) {
return parent::denormalizeParameter($class, $parameter, $parameterName, $parameterData, $context, $format);
}

Expand All @@ -946,7 +946,7 @@ protected function denormalizeParameter(\ReflectionClass $class, \ReflectionPara
/**
* @return Type|list<LegacyType>|null
*/
private function getType(string $currentClass, string $attribute): Type|array|null
private function getType(string $currentClass, string $attribute, array $context = []): Type|array|null
{
if (null === $this->propertyTypeExtractor) {
return null;
Expand All @@ -957,7 +957,7 @@ private function getType(string $currentClass, string $attribute): Type|array|nu
return false === $this->typeCache[$key] ? null : $this->typeCache[$key];
}

if (null !== $type = $this->getPropertyType($currentClass, $attribute)) {
if (null !== $type = $this->getPropertyType($currentClass, $attribute, $context)) {
return $this->typeCache[$key] = $type;
}

Expand All @@ -967,7 +967,7 @@ private function getType(string $currentClass, string $attribute): Type|array|nu
}

foreach ($discriminatorMapping->getTypesMapping() as $mappedClass) {
if (null !== $type = $this->getPropertyType($mappedClass, $attribute)) {
if (null !== $type = $this->getPropertyType($mappedClass, $attribute, $context)) {
return $this->typeCache[$key] = $type;
}
}
Expand All @@ -984,10 +984,10 @@ private function getType(string $currentClass, string $attribute): Type|array|nu
*
* @return Type|list<LegacyType>|null
*/
private function getPropertyType(string $className, string $property): Type|array|null
private function getPropertyType(string $className, string $property, array $context = []): Type|array|null
{
if (class_exists(Type::class) && method_exists($this->propertyTypeExtractor, 'getType')) {
return $this->propertyTypeExtractor->getType($className, $property);
return $this->propertyTypeExtractor->getType($className, $property, $context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Default context must be handled here

}

return $this->propertyTypeExtractor->getTypes($className, $property);
Expand Down
45 changes: 45 additions & 0 deletions src/Symfony/Component/Serializer/Tests/SerializerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1674,6 +1674,30 @@ public function testPartialDenormalizationWithInvalidVariadicParameter()
DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS => true,
]);
}

public function testExtractPublicPropertiesFirst()
Copy link
Contributor

Choose a reason for hiding this comment

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

This test must be long to the AbstractObjectNormalizerTest instead.
Also, you might add a test in the ReflectionExtractorTest as well.

{
$json = '{ "publicProperty": "foo", "privateProperty": true }';

$serializer = new Serializer([new ObjectNormalizer()], ['json' => new JsonEncoder()]);
$result = $serializer->deserialize(
$json,
DummyPublicPropertyWithHas::class,
'json',
[
'extract_public_properties_first' => true,
]
);

$this->assertEquals(
'foo',
$result->publicProperty
);
$this->assertEquals(
'1',
$result->getPrivateProperty()
);
}
}

class Model
Expand Down Expand Up @@ -1828,6 +1852,27 @@ public function getIterator(): \ArrayIterator
}
}

class DummyPublicPropertyWithHas
{
public string $publicProperty;

private string $privateProperty;

public function hasPublicProperty(): bool
{
}

public function getPrivateProperty(): bool
{
return $this->privateProperty;
}

public function setPrivateProperty(bool $value): void
{
$this->privateProperty = (string) $value;
}
}

abstract class DummyNormalizer implements NormalizerInterface, DenormalizerInterface
{
abstract public function getSupportedTypes(?string $format): array;
Expand Down
Loading