[PropertyInfo] Extract public properties first in ReflectionExtractor #59430
Conversation
tBibaut
commented
Jan 8, 2025
| Q | A |
|---|---|
| Branch? | 7.3 |
| Bug fix? | yes/no |
| New feature? | yes |
| Deprecations? | yes |
| Issues | Fix #57719 |
| License | MIT |
|
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
2ee42e8 to
77e76ee
Compare
| @@ -987,7 +987,7 @@ private function getType(string $currentClass, string $attribute): Type|array|nu | |||
| private function getPropertyType(string $className, string $property): 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: ['extract_public_properties_first' => true]); | |||
There was a problem hiding this comment.
I wonder if the context should instead be given by the user instead?
IMO it should be given from the normalize/denormalize context argument. Then we can leverage the default context (and update the recipe) to reduce the impact of that depreciation. WDYT?
77e76ee to
406655d
Compare
| $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.'); |
There was a problem hiding this comment.
| $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.'); |
| @@ -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 | |||
There was a problem hiding this comment.
Could you update the CHANGELOG.md of PropertyInfo and UPGRADE-7.3.md as well?
| { | ||
| if (class_exists(Type::class) && method_exists($this->propertyTypeExtractor, 'getType')) { | ||
| return $this->propertyTypeExtractor->getType($className, $property); | ||
| return $this->propertyTypeExtractor->getType($className, $property, $context); |
There was a problem hiding this comment.
Default context must be handled here
| @@ -1674,6 +1674,30 @@ public function testPartialDenormalizationWithInvalidVariadicParameter() | |||
| DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS => true, | |||
| ]); | |||
| } | |||
|
|
|||
| public function testExtractPublicPropertiesFirst() | |||
There was a problem hiding this comment.
This test must be long to the AbstractObjectNormalizerTest instead.
Also, you might add a test in the ReflectionExtractorTest as well.