[FrameworkBundle][PropertyInfo] Allow defining accessors and mutators via an attribute #59529

HypeMC · 2025-01-16T19:04:22Z

Q	A
Branch?	7.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	-
License	MIT

A continuation/revival of #38515, ~~contains #59497 and #59528~~

The idea of this PR is to allow defining accessors and mutators via a #[WithAccessors] attribute, eg:

class Foo
{
    #[WithAccessors(getter: 'giveProp', setter: 'receiveProp', adder: 'pushProp', remover: 'popProp')]
    private array $prop;

    public function giveProp(): string {}

    public function receiveProp(string $prop): void {}

    public function pushProp(string $prop): void {}

    public function popProp(string $prop): void {}
}

I haven't added YAML or XML support yet, but the foundation is in place, so it can easily be added in a future PR if needed.

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

HypeMC · 2025-01-16T19:12:10Z

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

        $allowGetterSetter = $context['enable_getter_setter_extraction'] ?? false;
        $magicMethods = $context['enable_magic_methods_extraction'] ?? $this->magicMethodsFlags;
        $allowMagicCall = (bool) ($magicMethods & self::ALLOW_MAGIC_CALL);
        $allowMagicSet = (bool) ($magicMethods & self::ALLOW_MAGIC_SET);
        $allowConstruct = $context['enable_constructor_extraction'] ?? $this->enableConstructorExtraction;
-        $allowAdderRemover = $context['enable_adder_remover_extraction'] ?? true;


Needed earlier

HypeMC · 2025-01-16T19:13:35Z

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


-        $camelized = $this->camelize($property);


No point in getting the camelized version if the info can be extracted from the constructor. Moved it to when it's actually needed.

HypeMC · 2025-01-16T19:14:04Z

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

        $constructor = $reflClass->getConstructor();
-        $singulars = $this->inflector->singularize($camelized);


This was only used in findAdderAndRemover(), so I moved it there.

HypeMC · 2025-01-16T19:15:18Z

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

-            $mutator = new PropertyWriteInfo(PropertyWriteInfo::TYPE_ADDER_AND_REMOVER);
-            $mutator->setAdderInfo(new PropertyWriteInfo(PropertyWriteInfo::TYPE_METHOD, $adderAccessName, $this->getWriteVisibilityForMethod($adderMethod), $adderMethod->isStatic()));
-            $mutator->setRemoverInfo(new PropertyWriteInfo(PropertyWriteInfo::TYPE_METHOD, $removerAccessName, $this->getWriteVisibilityForMethod($removerMethod), $removerMethod->isStatic()));
+        if (null === $adderAccessName || null === $removerAccessName) {


The $adderAccessName and $removerAccessName have been defined in the metadata.

src/Symfony/Bundle/FrameworkBundle/composer.json

nicolas-grekas · 2025-01-17T12:54:12Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/property_info.php

+        ->set('property_info.mapping.class_metadata_factory', ClassMetadataFactory::class)
+            ->args([service('property_info.mapping.attribute_loader')])
+
+        ->alias(ClassMetadataFactoryInterface::class, 'property_info.mapping.class_metadata_factory')


do you ancitipate a use case for autowiring this in an app? if not we could remove the alias

No, removed https://github.com/symfony/symfony/compare/d54d95aaff03e50a1925eb7c5bd6d4a50de2bc15..e55a729db2f7ac56900f6333a2d81eb0bddcd8af

src/Symfony/Bundle/FrameworkBundle/composer.json

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

nicolas-grekas

I'm wondering about the added metadata factory infrastructure. Do we really need to provide an API for this? I'm not sure it's a useful extensibility point.
Wouldn't the PR be way simpler without? What's the real world use case of these interfaces if you think we should add them?

nicolas-grekas · 2025-01-28T16:43:36Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/property_info.php

+        ->set('property_info.mapping.class_metadata_factory', ClassMetadataFactory::class)
+            ->args([service('property_info.mapping.attribute_loader')])
+
+        // Cache


captain obvious ;) let's remove these

nicolas-grekas · 2025-01-28T16:44:47Z

src/Symfony/Component/PropertyInfo/Attribute/WithAccessors.php

+        public ?string $adder = null,
+        public ?string $remover = null,
+    ) {
+        if (null === $this->getter && null === $this->setter && null === $this->adder && null === $this->remover) {


Suggested change

if (null === $this->getter && null === $this->setter && null === $this->adder && null === $this->remover) {

if (!($this->getter || $this->setter || $this->adder || $this->remover)) {

nicolas-grekas · 2025-01-28T16:45:12Z

src/Symfony/Component/PropertyInfo/Attribute/WithAccessors.php

+        if (null === $this->getter && null === $this->setter && null === $this->adder && null === $this->remover) {
+            throw new LogicException('You need to have at least one method name set.');
+        }
+        if (null === $this->adder xor null === $this->remover) {


Suggested change

if (null === $this->adder xor null === $this->remover) {

if ($this->adder xor $this->remover) {

nicolas-grekas · 2025-01-28T16:50:01Z

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

    {
+        $propertyMetadata = $this->getClassMetadata($reflClass->getName())?->getPropertyMetadataFor($property);
+        if ((null !== $addMethod = $propertyMetadata?->adder) && (null !== $removeMethod = $propertyMetadata?->remover)) {


for readability (to me at least), there are many cases where checks could be simplified like this:

Suggested change

if ((null !== $addMethod = $propertyMetadata?->adder) && (null !== $removeMethod = $propertyMetadata?->remover)) {

if (($addMethod = $propertyMetadata?->adder) && ($removeMethod = $propertyMetadata?->remover)) {

nicolas-grekas · 2025-01-28T16:52:30Z

src/Symfony/Component/PropertyInfo/Mapping/Loader/AttributeLoader.php

+        }
+
+        if ($invalidMethods) {
+            throw new MappingException(\sprintf('The class "%s" does not have the following methods: "%s".', $refClass->name, implode('", "', $invalidMethods)), $refClass->name, $invalidMethods);


"Check its #[WithAccessors] attributes."

… via an attribute

HypeMC · 2025-02-03T01:12:23Z

I'm wondering about the added metadata factory infrastructure. Do we really need to provide an API for this? I'm not sure it's a useful extensibility point. Wouldn't the PR be way simpler without? What's the real world use case of these interfaces if you think we should add them?

@nicolas-grekas I think it really depends on whether XML and/or YAML support should or will be added. If supporting multiple formats is desired, then the current infrastructure makes sense to me. Otherwise, I'd simplify it by keeping only an attribute loader, possibly with a cached loader.

Personally, I never use XML/YAML for any kind of mapping (validator, serializer, etc.), so I'd be fine with removing it.

HypeMC requested a review from dunglas as a code owner January 16, 2025 19:04

carsonbot added Status: Needs Review Feature FrameworkBundle PropertyInfo labels Jan 16, 2025

carsonbot added this to the 7.3 milestone Jan 16, 2025

carsonbot changed the title ~~[PropertyInfo][FrameworkBundle] Allow defining accessors and mutators via an attribute~~ [FrameworkBundle][PropertyInfo] Allow defining accessors and mutators via an attribute Jan 16, 2025

HypeMC commented Jan 16, 2025

View reviewed changes

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php Show resolved Hide resolved

HypeMC commented Jan 16, 2025

View reviewed changes

mtarld reviewed Jan 16, 2025

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/composer.json Outdated Show resolved Hide resolved

HypeMC force-pushed the with-accessors-attribute branch 3 times, most recently from 8aec093 to d54d95a Compare January 17, 2025 12:57

nicolas-grekas reviewed Jan 17, 2025

View reviewed changes

HypeMC force-pushed the with-accessors-attribute branch from d54d95a to e55a729 Compare January 17, 2025 13:29

nicolas-grekas reviewed Jan 21, 2025

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Show resolved Hide resolved

nicolas-grekas reviewed Jan 28, 2025

View reviewed changes

[PropertyInfo][FrameworkBundle] Allow defining accessors and mutators…

bb0f56d

… via an attribute

HypeMC force-pushed the with-accessors-attribute branch from e55a729 to bb0f56d Compare February 2, 2025 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle][PropertyInfo] Allow defining accessors and mutators via an attribute #59529

[FrameworkBundle][PropertyInfo] Allow defining accessors and mutators via an attribute #59529

HypeMC commented Jan 16, 2025 •

edited

Loading

HypeMC Jan 16, 2025

HypeMC Jan 16, 2025 •

edited

Loading

HypeMC Jan 16, 2025

HypeMC Jan 16, 2025

nicolas-grekas Jan 17, 2025

HypeMC Jan 17, 2025

nicolas-grekas left a comment

nicolas-grekas Jan 28, 2025

HypeMC Feb 2, 2025

nicolas-grekas Jan 28, 2025

HypeMC Feb 2, 2025

nicolas-grekas Jan 28, 2025

HypeMC Feb 2, 2025

nicolas-grekas Jan 28, 2025

HypeMC Feb 2, 2025

nicolas-grekas Jan 28, 2025

HypeMC Feb 2, 2025

HypeMC commented Feb 3, 2025

		$constructor = $reflClass->getConstructor();
		$singulars = $this->inflector->singularize($camelized);

	if (null === $this->getter && null === $this->setter && null === $this->adder && null === $this->remover) {
	if (!($this->getter \|\| $this->setter \|\| $this->adder \|\| $this->remover)) {

	if (null === $this->adder xor null === $this->remover) {
	if ($this->adder xor $this->remover) {

	if ((null !== $addMethod = $propertyMetadata?->adder) && (null !== $removeMethod = $propertyMetadata?->remover)) {
	if (($addMethod = $propertyMetadata?->adder) && ($removeMethod = $propertyMetadata?->remover)) {

[FrameworkBundle][PropertyInfo] Allow defining accessors and mutators via an attribute #59529

Are you sure you want to change the base?

[FrameworkBundle][PropertyInfo] Allow defining accessors and mutators via an attribute #59529

Conversation

HypeMC commented Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

HypeMC Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HypeMC commented Feb 3, 2025

HypeMC commented Jan 16, 2025 •

edited

Loading

HypeMC Jan 16, 2025 •

edited

Loading