Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[FrameworkBundle][PropertyInfo] Allow defining accessors and mutators via an attribute#59529
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| $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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Needed earlier
| $camelized = $this->camelize($property); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No point in getting the camelized version if the info can be extracted from the constructor. Moved it to when it's actually needed.
| $constructor = $reflClass->getConstructor(); | ||
| $singulars = $this->inflector->singularize($camelized); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This was only used infindAdderAndRemover(), so I moved it there.
| $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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The$adderAccessName and$removerAccessName have been defined in the metadata.
Uh oh!
There was an error while loading.Please reload this page.
8aec093 tod54d95aCompare| ->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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
do you ancitipate a use case for autowiring this in an app? if not we could remove the alias
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Uh oh!
There was an error while loading.Please reload this page.
d54d95a toe55a729Comparesrc/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
| ->set('property_info.mapping.class_metadata_factory', ClassMetadataFactory::class) | ||
| ->args([service('property_info.mapping.attribute_loader')]) | ||
| // Cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
captain obvious ;) let's remove these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done :D
| public ?string $adder = null, | ||
| public ?string $remover = null, | ||
| ) { | ||
| if (null === $this->getter && null === $this->setter && null === $this->adder && null === $this->remover) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if (null ===$this->getter&&null ===$this->setter&&null ===$this->adder&&null ===$this->remover) { | |
| if (!($this->getter||$this->setter||$this->adder||$this->remover)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if (null ===$this->adderxornull ===$this->remover) { | |
| if ($this->adderxor$this->remover) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| { | ||
| $propertyMetadata = $this->getClassMetadata($reflClass->getName())?->getPropertyMetadataFor($property); | ||
| if ((null !== $addMethod = $propertyMetadata?->adder) && (null !== $removeMethod = $propertyMetadata?->remover)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
for readability (to me at least), there are many cases where checks could be simplified like this:
| if ((null !==$addMethod =$propertyMetadata?->adder) && (null !==$removeMethod =$propertyMetadata?->remover)) { | |
| if (($addMethod =$propertyMetadata?->adder) && ($removeMethod =$propertyMetadata?->remover)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| } | ||
| if ($invalidMethods) { | ||
| throw new MappingException(\sprintf('The class "%s" does not have the following methods: "%s".', $refClass->name, implode('", "', $invalidMethods)), $refClass->name, $invalidMethods); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"Check its #[WithAccessors] attributes."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
e55a729 tobb0f56dCompare
@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. |
Uh oh!
There was an error while loading.Please reload this page.
A continuation/revival of#38515,
contains#59497 and#59528The idea of this PR is to allow defining accessors and mutators via a
#[WithAccessors]attribute, eg: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.