[PropertyInfo] Add accessor and mutator extractor interface and implementation on reflection

[joelwurtz]

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#30248, partially: #22190, #18016, #5013, #9336, #5219,
License	MIT
Doc PR	TODO

This PR brings accessor / mutator extraction on the PropertyInfo component,

There is no link to existing code, as IMO it should be in another PR as this will add a dependency on property access to the property info component and not sure this is something wanted (although, it will reduce a lot of code base on the property access component as a lot of code seems to be duplicated)

Code is extracted from #30248 also there is some new features (that can be removed if not wanted)

• Allow extracting private accessor / mutator (will do a new PR that improve private extraction on reflection latter)
• Allow extracting static accessor / mutators
• Allow extracting constructor mutators

Current implementation try to be as close as the PropertyAccess implementation and i did not reuse some methods already available in the class as there is some differences in implementation, but maybe it will be a good time to make this consistent (Looking forward to your input) ?

Things that should be done in a new PR:

• Linking property info to property access to remove a lot of duplicate code
• Add a new system that allow adding Virtual Property based on this extractor

[joelwurtz]

Thanks @fabpot just updated the PR to reflect the change and extracting accessor / mutator with access flags.

I also begin to add consistency to the extraction, but not sure this is wanted, here is a list of different behavior between ReflectionExtractor and PropertyAccessor class where have i a question about what do to:

• ReflectionExtractor use a configurable list of accessor and mutator prefixes to know if the property exist and can be read / write where PropertyAccessor has a hard coded list: I don't think it's a problem as default list is the same as the hard coded list of PropertyAccessor: Should we use the ReflectionExtractor list and not the hard coded one ?

• If using this list: PropertyAccessor allows to get / set on methods having the same name (jQuery style public function foo(string $foo = null): string)), where ReflectionExtractor does not allow it, and adding an empty prefix to the list breaks some tests, Should we add support for this or change the code to allow empty prefix ?

• PropertyAccessor limit add / remove mutator only when type of the property is a traversable / array and only when both methods are available, where ReflectionExtractor does the reverse, it suppose that type is array if the method is present, and made the property writable if one of the add or remove method if available: Should we add support for restricting those mutators like PropertyAccessor do ?

• PropertyAccessor has support for magic call where ReflectionExtractor does not have it by default (private property + __get or __set = not writable or readable): Should we add support for magic set / get in ReflectionExtractor ?

Support for each of this behavior can be done in constructor arguments or context, would prefer to avoid context and, in the future, use different ReflectionExtractor service for each use case, as most of the time those behavior are domain specific (for a set of class, not for one only).

[dunglas]

I fully agree with this move. I also think we should make PropertyInfo a hard dependency of PropertyAccess (and PropertyAccess a hard dependency of Serializer) to prevent code duplication. It's ok for Symfony components to depend of other lower level Symfony components.

Should we use the ReflectionExtractor list and not the hard coded one ?

I think so.

Should we add support for this or change the code to allow empty prefix.

I'm not sure that the jQuery-style is still very used. jQuery itself is not used anymore in modern JS projects. It's why I not implemented it (and AFAIK nobody complained since the creation of this library). As a an alternative, we couldn't just deprecate support for this notation. WDYT?

Should we add support for restricting those mutators like PropertyAccessor do ?

It looks reasonable.

Should we add support for magic set / get?

Yes, it could be nice as long as it's optional. I'm not sure that this feature is used a lot, but it could be useful for advanced use cases.

[fabpot]

I agree with @dunglas, that having hard dependencies between such components make sense (especially as they are low-level building blocks).

[dbu]

nice, i like these very limited information models as building blocks.

i guess a serializer would use these as one element along with other things like the target name (serializer) resp source name (deserialize)?

i commented some random thoughts i had while reading this. lets discuss tomorrow.

[joelwurtz]

Thanks @dbu for the review

i guess a serializer would use these as one element along with other things like the target name (serializer) resp source name (deserialize)?

With the global vision, serializer will never use this :)

It intents to work as the following schema:

• Serializer use the PropertyAccess component to read / write a value for a data
• PropertyAccess use this extractor to know how to read / write a value for an object

This will mask the complexity of accessing / setting value on the serializer component and also ensure that we can also read / write to / from an array or other dynamic object that does not have metadatas.

[dbu]

With the global vision, serializer will never use this

thanks for the clarifications. and i think that makes sense. the only concern i have is with performance. large object graphs with many fields can mean A LOT of method calls. if there are several 10k of single fields to handle, method calls for each property start to add up. this is what prompted us to write https://github.com/liip/serializer/ .
i agree that for the normal case, a bit of overhead is worth it when it avoids a lot of duplication in the codebase. but i'd love to check how we can make code generation possible so that it can cleanly replace the slower implementation keeping the exact same behaviour.

[joelwurtz]

but i'd love to check how we can make code generation possible so that it can cleanly replace the slower implementation keeping the exact same behaviour.

This is clearly something that i want to make it possible with new organisation / extension point. As we could do a specific PropertyAccess implementation that generate code given accessor / mutator (see the automapper pull request #30248 that have something like this), which will boost serializer by a large factor.

[Korbeil]

@lyrixx @dunglas ping, fixed all comments.

About errors, I had to update PropertyWriteInfo DTO to store errors and use theses in PropertyAccessor class 😉

[bastnic]

This is huge, thanks @Korbeil!

[lyrixx]

Thank you @joelwurtz and @Korbeil.