8000 [PropertyInfo] Add accessor and mutator extractor interface and implementation on reflection by joelwurtz · Pull Request #30704 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 31, 2020
Merged

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

merged 2 commits into from
Jan 31, 2020

Conversation

joelwurtz
Copy link
Contributor
@joelwurtz joelwurtz commented Mar 26, 2019 •
edited
Loading
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 joelwurtz requested a review from dunglas as a code owner March 26, 2019 12:05
@joelwurtz joelwurtz force-pushed the feature/accessor-mutator-extractor branch from 03052ee to 4f81ee7 Compare March 26, 2019 12:08
@joelwurtz joelwurtz mentioned this pull request Mar 26, 2019
Closed
@joelwurtz

This comment has been minimized.

Sign in to view
@nicolas-grekas nicolas-grekas added this to the next milestone Mar 26, 2019
@joelwurtz joelwurtz force-pushed the feature/accessor-mutator-extractor branch 2 times, most recently from 8a6690b to 95c8905 Compare March 26, 2019 23:26
@fabpot

This comment has been minimized.

Sign in to view
@joelwurtz joelwurtz force-pushed the feature/accessor-mutator-extractor branch 2 times, most recently from 3877283 to 4e17c0a Compare March 27, 2019 09:15
@joelwurtz
Copy link
Contributor Author
joelwurtz commented Mar 27, 2019 •
edited
Loading

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).

@joelwurtz joelwurtz mentioned this pull request Mar 27, 2019
Merged
1 task
@fabpot

This comment has been minimized.

Sign in to view
@dunglas
Copy link
Member
dunglas commented Mar 31, 2019 •
edited
Loading

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
Copy link
Member
fabpot commented Mar 31, 2019

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

@joelwurtz joelwurtz mentioned this pull request Apr 1, 2019
Open
16 tasks
dbu
dbu reviewed Apr 5, 2019
View reviewed changes
Copy link
Contributor
@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Contributor
dbu commented Apr 5, 2019

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
Copy link
Contributor Author

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.

@joelwurtz joelwurtz force-pushed the feature/accessor-mutator-extractor branch 2 times, most recently from b75cf43 to f1c69ad Compare April 6, 2019 15:46
@joelwurtz

This comment has been minimized.

Sign in to view
@Korbeil Korbeil force-pushed the feature/accessor-mutator-extractor branch from dca94fd to 52b6fad Compare January 23, 2020 17:20
@Korbeil
Copy link
Contributor
Korbeil commented Jan 23, 2020 •
edited
Loading

@lyrixx @dunglas ping, fixed all comments.

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

@Korbeil Korbeil force-pushed the feature/accessor-mutator-extractor branch 2 times, most recently from 98e5554 to 37fdded Compare January 24, 2020 09:45
@bastnic
Copy link
Contributor
bastnic commented Jan 27, 2020

This is huge, thanks @Korbeil!

@Korbeil Korbeil force-pushed the feature/accessor-mutator-extractor branch from 37fdded to 2fef84c Compare January 28, 2020 09:04
@Korbeil Korbeil force-pushed the feature/accessor-mutator-extractor branch from 2fef84c to 0a92dab Compare January 28, 2020 09:55
@Korbeil Korbeil mentioned this pull request Jan 28, 2020
Closed
@lyrixx
Copy link
Member
lyrixx commented Jan 31, 2020 •
edited
Loading

Thank you @joelwurtz and @Korbeil.

lyrixx added a commit that referenced this pull request Jan 31, 2020
@lyrixx
feature #30704 [PropertyInfo] Add accessor and mutator extractor inte…
a1e4222 
…rface and implementation on reflection (joelwurtz, Korbeil)

This PR was merged into the 5.1-dev branch.

Discussion
----------

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

| 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

Commits
-------

0a92dab Rebase, fix tests, review & update CHANGELOG
fc25086 [PropertyInfo] Add accessor and mutator extractor interface and implementation on reflection
@lyrixx lyrixx mentioned this pull request Jan 31, 2020
Closed
@lyrixx lyrixx merged commit 0a92dab into symfony:master Jan 31, 2020
This was referenced Jan 31, 2020
Closed
Closed
@joelwurtz joelwurtz deleted the feature/accessor-mutator-extractor branch February 3, 2020 07:45
This was referenced Mar 17, 2020
Merged
Merged
fabpot added a commit that referenced this pull request Mar 18, 2020
@fabpot
bug #36119 [PropertyAccess] Added missing new args in `PropertyAccess…
6902a56 
…orBuilder` (HeahDude)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[PropertyAccess] Added missing new args in `PropertyAccessorBuilder`

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | ~
| License       | MIT
| Doc PR        | ~

Following #30704.

Commits
-------

e1be8cd [PropertyAccess] Added missing new args in PropertyAccessorBuilder
fabpot added a commit that referenced this pull request Mar 18, 2020
@fabpot
minor #36120 [FrameworkBundle][PropertyAccess] Use injection for info…
e383b41 
… extractors (HeahDude)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[FrameworkBundle][PropertyAccess] Use injection for info extractors

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | ~
| License       | MIT
| Doc PR        | ~

Follows #30704.

Commits
-------

693d4c0 [FrameworkBundle][PropertyAccess] Use injection for info extractors
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
@1ed 1ed mentioned this pull request Jun 1, 2020
Closed
@Korbeil Korbeil mentioned this pull request Oct 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbu dbu dbu left review comments

@webmozart webmozart webmozart requested changes

@Korbeil Korbeil Korbeil left review comments

@dunglas dunglas dunglas approved these changes

@lyrixx lyrixx lyrixx approved these changes

@maxhelias maxhelias maxhelias approved these changes

Assignees
No one assigned
Labels
Feature PropertyInfo Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
0