[PropertyInfo] Add PropertyAttributesExtractor
#57601
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
9562951 to
e0199a8
Compare
src/Symfony/Component/PropertyInfo/PropertyAttributesExtractorInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/PropertyInfo/Tests/Fixtures/DummyAttribute.php
Outdated
Show resolved
Hide resolved
e55b199 to
20631f8
Compare
20631f8 to
ceafe5d
Compare
PropertyAttributesExtractor
|
@OskarStark Hi, thanks for the review! Since this is my first PR, can you point out any potential issues that might be preventing feedback, or is it just the high number of PRs for the maintainers? The tests are failing, but it seems unrelated to my changes. |
| * - name: The fully-qualified class name of the attribute | ||
| * - arguments: An associative array of attribute arguments if present |
There was a problem hiding this comment.
I would use @param annotation to describe these parameters
There was a problem hiding this comment.
Since the @param annotation is used for describing method's arguments, are you suggesting using it for the string $class, string $property, array $context = [] arguments?
| * - name: The fully-qualified class name of the attribute | ||
| * - arguments: An associative array of attribute arguments if present | ||
| * | ||
| * @return array<int, array{name: string, arguments: array<array-key, mixed>}>|null |
There was a problem hiding this comment.
| * @return array<int, array{name: string, arguments: array<array-key, mixed>}>|null | |
| * @return list<array{name: string, arguments: array<array-key, mixed>}>|null |
| public function getAttributes($class, $property, array $context = []): ?array | ||
| { | ||
| $this->assertIsString($class); | ||
| $this->assertIsString($property); | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
| public function getAttributes($class, $property, array $context = []): ?array | |
| { | |
| $this->assertIsString($class); | |
| $this->assertIsString($property); | |
| return null; | |
| } | |
| public function getAttributes(string $class, string $property, array $context = []): ?array | |
| { | |
| return null; | |
| } |
I think that is enough, thanks to modern PHP typing system.
| @@ -45,6 +47,7 @@ | |||
| ->tag('property_info.type_extractor', ['priority' => -1002]) | |||
| ->tag('property_info.access_extractor', ['priority' => -1000]) | |||
| ->tag('property_info.initializable_extractor', ['priority' => -1000]) | |||
| ->tag('property_info.attributes_extractor', ['priority' => -1000]) | |||
There was a problem hiding this comment.
| ->tag('property_info.attributes_extractor', ['priority' => -1000]) | |
| ->tag('property_info.attribute_extractor', ['priority' => -1000]) |
To be consistent? (same in FrameworkExtension and PropertyInfoPass)
| /** | ||
| * @author Andrew Alyamovsky <andrew.alyamovsky@gmail.com> | ||
| */ | ||
| interface PropertyAttributesExtractorInterface |
There was a problem hiding this comment.
| interface PropertyAttributesExtractorInterface | |
| interface PropertyAttributeExtractorInterface |
To be consistent with other extractors. For example, even if getTypes is plural, PropertyTypeExtractorInterface is singular.
| foreach ($reflProperty->getAttributes() as $attribute) { | ||
| $attributes[] = [ | ||
| 'name' => $attribute->getName(), | ||
| 'arguments' => $attribute->getArguments(), | ||
| ]; | ||
| } | ||
|
|
||
| return $attributes; |
There was a problem hiding this comment.
| foreach ($reflProperty->getAttributes() as $attribute) { | |
| $attributes[] = [ | |
| 'name' => $attribute->getName(), | |
| 'arguments' => $attribute->getArguments(), | |
| ]; | |
| } | |
| return $attributes; | |
| return $reflProperty->getAttributes(); |
Why not returning attributes themselves? So that we can have all the attribute data (repeated, etc) and use the newInstance method.
If it was because you want to decouple getAttributes from \ReflectionXXX, I think that we shouldn't try to decouple as it's part of the PHP language itself.
While the PropertyInfo component works great for extracting property annotations, in its current state it does not support extracting attributes. This means there's a necessity to use
ReflectionClasson the target class to extract attributes, and that it's not possible to leveragePropertyInfoCacheExtractorto cache the results.This PR adds a separate
PropertyAttributesExtractorInterfaceinterface that does just that, which is implemented by the existingReflectionExtractorclass.