[PropertyAccess] Allow custom methods on property accesses #18016

lrlopez · 2016-03-05T00:43:42Z

Q	A
Branch	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#5013, #9336, #5219, among others
License	MIT
Doc PR	symfony/symfony-docs#6400

Some tests do not pass but failures seem not be related to the changes of this PR.

Some examples of how to use the new feature:

Annotations:

/**
 * @Property(getter="getCurrentValue", setter="updateValue")
 */
protected $value;

/**
 * @Property(adder="joinMember", remover="detachMember")
 */
protected $members;

public function getCurrentValue()
{
    return $this->value;
}

public function updateValue($newValue)
{
    $this->value = $newValue;
    return $this;
}

public function joinMember($newMember)
{
    // Add $newMember to the collection
}

public function detachMember($oldMember)
{
    // Remove $oldMember from the collection
}

/**
 * @PropertyGetter(property="calculated")
 */
public function getCalculated()
{
    // whatever...
}

// You can omit 'property' in method annotations
/**
 * @PropertySetter("calculated")
 */
public function setCalculated($calculated)
{
    // whatever...
}

YAML:

'AppBundle\Entity\Example':
  properties:
    value:
      getter: getCurrentValue
      setter: updateValue
    members:
      adder: joinMember
      remover: detachMember
    calculated:
      getter: getCalculated
      setter: setCalculated

XML:

<?xml version="1.0" ?>

<property-access xmlns="http://symfony.com/schema/dic/property-access-mapping"
                    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                    xsi:schemaLocation="http://symfony.com/schema/dic/property-access-mapping http://symfony.com/schema/dic/property-access-mapping/property-access-mapping-1.0.xsd">

    <class name="AppBundle\Entity\Example">
        <property name="value" getter="getCurrentValue" setter="updateValue" />
        <property name="members" adder="joinMember" remover="detachMember" />
        <property name="calculated" getter="getCalculated" setter="setCalculated" />
    </class>

</property-access>

Check-list:

Implement YAML & XML mappings
Address @dunglas and @webmozart comments
Implement method annotations for virtual properties
Write documentation

GuilhemN · 2016-03-05T07:55:57Z

src/Symfony/Component/PropertyAccess/composer.json

@@ -17,7 +17,8 @@
    ],
    "require": {
        "php": ">=5.5.9",
-        "symfony/polyfill-php70": "~1.0"
+        "symfony/polyfill-php70": "~1.0",
+        "doctrine/annotations": "~1.2"


I don't think that's a good idea to add a new dependency to this component.
Maybe put it in require-dev / suggest instead ?

Yep, you're right. Thank you!

TomasVotruba · 2016-03-05T23:09:35Z

This looks really good.

lrlopez · 2016-03-06T02:40:49Z

YAML and XML mapping should be working now...

dunglas · 2016-03-06T19:43:48Z

I like the idea, it will be an alternative to #15200

dunglas · 2016-03-06T19:44:43Z

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

+        $serializerLoaders = array();
+        if (isset($config['enable_annotations']) && $config['enable_annotations']) {
+            $annotationLoader = new Definition(
+                'Symfony\Component\PropertyAccess\Mapping\Loader\AnnotationLoader',


Prefer AnnotationLoader::class

Ah, ok. That change would make the PR incompatible with PHP 5.4, but if its headed to Symfony 3.1 that shouldn't be a problem

dunglas · 2016-03-06T19:56:20Z

It's probably out of scope for this PR, but the code for handling metadata is duplicated in most components. Maybe can we introduce a new Metadata component to factorize this code?

lrlopez · 2016-03-06T21:29:14Z

@dunglas: Yes, I agree. Removing all the repeated and overlapped functionality is the next logical step. There is some discussion about a new Metadata component at the end of PR #9336

GuilhemN · 2016-07-21T09:29:58Z

WDYT about only supporting annotations at first and split xml/yaml support in another PR ?
This should make this PR easier to review.

GuilhemN · 2016-07-21T09:32:24Z

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

+     * @param bool                          $magicCall
+     * @param bool                          $throwExceptionOnInvalidIndex
+     * @param CacheItemPoolInterface        $cacheItemPool
+     * @param ClassMetadataFactoryInterface $classMetadataFactory


The type hint is not same (MetadataFactoryInterface|null)

lrlopez · 2016-09-25T11:41:05Z

Hi again guys,

I intend to resume (and hopefully finish) this PR in October so I'm starting to plan how to address all the pointed issues. I still have two questions on how to proceed, though:

Should I split the PR into two pieces? One with the PropertyAccess changes and another with the FrameworkBundle integration. Reviewing the changes could be easier this way.
@dunglas suggested to implement LazyLoadingMetadataFactory cache as a decorator. I agree that, on one hand, it allows a better understanding of the inner workings but, on the other hand, it will make the implementation diverge from the Validator component and will make more difficult to migrate both components when the future Metadata component is integrated into Symfony.

I hope some of you can help me to decide 😜

xabbuh · 2016-10-21T07:01:56Z

@lrlopez Please make the split. :) It indeed makes reviewing a lot easier. I also agree with @dunglas about implementing the cached metadata factory as a decorator. The code would be easier to understand, its architecture is more clean and hopefully better maintainable.

fabpot · 2017-03-22T16:59:34Z

@lrlopez Still interested in finishing this one?

GuilhemN · 2017-03-22T17:03:44Z

Note that having a big caching layer would be less critical with #22051 and would probably allow a simpler implementation here.

lrlopez · 2017-03-22T18:23:04Z

@fabpot Sure! We haven't reached the 3.3 feature-freeze milestone, have we? I'll try to get this ready in time.

@GuilhemN, looks nice, we can refactor later if your PR is merged

fabpot · 2017-03-22T18:28:19Z

3.3 feature-freeze in 7 days :)

lrlopez · 2017-03-22T20:53:42Z

Then I'd better hurry up in order to make the cut :)

lrlopez · 2017-03-27T17:57:26Z

Quick status report:

I'm currently splitting this PR into two: the first covers the changes applied to the PropertyAccess component and the second one integrates them into FrameworkBundle. Once everything is finished I will close this issue and move discussion there.

The new Cache component is awesome so I don't think it will be painful to support it. Anyway, sooner or later Symfony will need a Metadata component that allows to remove duplicate and redundant code in many components (it feels bad to borrow so much code).

Should I find no problems, I expect to publish the PRs tonight in order to start receiving feedback.

lrlopez · 2017-03-29T20:35:07Z

The new code is ready for review at PR #22190. Somewhere along the way the component implemented a PropertyPath cache. At the end I've decided to reuse the existing cache pool as using an independent one could be confusing.

@fabpot , I hope I'm still on time for 3.3 feature-freeze :)

fabpot · 2017-04-01T09:04:27Z

Closing this one then. Thanks.

…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

carsonbot added the Status: Needs Review label Mar 5, 2016

lrlopez force-pushed the property-accessor-config branch from e461527 to 8d700aa Compare March 5, 2016 00:46

lrlopez mentioned this pull request Mar 5, 2016

[PropertyAccess] RFC: PropertyPath options #5013

Closed

lrlopez changed the title ~~[PropertyAccessor] WIP: Allow customizing which methods get called~~ [PropertyAccess] WIP: Allow customizing which methods get called Mar 5, 2016

GuilhemN reviewed Mar 5, 2016
View reviewed changes

javiereguiluz added Feature PropertyAccess labels Mar 5, 2016

lrlopez force-pushed the property-accessor-config branch from 8d700aa to 0f303e9 Compare March 5, 2016 09:45

lrlopez changed the title ~~[PropertyAccess] WIP: Allow customizing which methods get called~~ [PropertyAccess] [RFC] [WIP] Allow custom methods on property accesses Mar 5, 2016

lrlopez force-pushed the property-accessor-config branch from 830fee6 to 5bd8b55 Compare March 5, 2016 20:10

lrlopez force-pushed the property-accessor-config branch from 9b55f63 to 18ad328 Compare March 6, 2016 02:42

dunglas reviewed Mar 6, 2016
View reviewed changes

GuilhemN mentioned this pull request Jul 21, 2016

[Serializer] [WIP] Added annotations and MetadataAwareNormalizer #19374

Closed

5 tasks

GuilhemN reviewed Jul 21, 2016
View reviewed changes

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

magarzon mentioned this pull request Jan 18, 2017

[Serializer] Alias for attributes #15171

Closed

fabpot added Status: Waiting feedback and removed Status: Needs Review labels Mar 22, 2017

This was referenced Mar 27, 2017

[PropertyAccess] Allow custom property accessors #22190

Closed

[FrameworkBundle] Integrate custom property accessors into the framework #22191

Closed

fabpot closed this Apr 1, 2017

joelwurtz mentioned this pull request Mar 26, 2019

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

Merged

javiereguiluz added the ⭐️ EU-FOSSA Hackathon label Apr 1, 2019

lrlopez mentioned this pull request Oct 11, 2020

[PropertyAccess] [For reference only] Allow custom property accessors #38515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PropertyAccess] Allow custom methods on property accesses #18016

[PropertyAccess] Allow custom methods on property accesses #18016

lrlopez commented Mar 5, 2016

GuilhemN Mar 5, 2016

lrlopez Mar 5, 2016

TomasVotruba commented Mar 5, 2016

lrlopez commented Mar 6, 2016

dunglas commented Mar 6, 2016

dunglas Mar 6, 2016

lrlopez Mar 6, 2016

dunglas commented Mar 6, 2016

lrlopez commented Mar 6, 2016

GuilhemN commented Jul 21, 2016 •

edited

Loading

GuilhemN Jul 21, 2016

lrlopez commented Sep 25, 2016

xabbuh commented Oct 21, 2016

fabpot commented Mar 22, 2017

GuilhemN commented Mar 22, 2017

lrlopez commented Mar 22, 2017

fabpot commented Mar 22, 2017

lrlopez commented Mar 22, 2017

lrlopez commented Mar 27, 2017

lrlopez commented Mar 29, 2017 •

edited

Loading

fabpot commented Apr 1, 2017

[PropertyAccess] Allow custom methods on property accesses #18016

[PropertyAccess] Allow custom methods on property accesses #18016

Conversation

lrlopez commented Mar 5, 2016

GuilhemN Mar 5, 2016

Choose a reason for hiding this comment

lrlopez Mar 5, 2016

Choose a reason for hiding this comment

TomasVotruba commented Mar 5, 2016

lrlopez commented Mar 6, 2016

dunglas commented Mar 6, 2016

dunglas Mar 6, 2016

Choose a reason for hiding this comment

lrlopez Mar 6, 2016

Choose a reason for hiding this comment

dunglas commented Mar 6, 2016

lrlopez commented Mar 6, 2016

GuilhemN commented Jul 21, 2016 • edited Loading

GuilhemN Jul 21, 2016

Choose a reason for hiding this comment

lrlopez commented Sep 25, 2016

xabbuh commented Oct 21, 2016

fabpot commented Mar 22, 2017

GuilhemN commented Mar 22, 2017

lrlopez commented Mar 22, 2017

fabpot commented Mar 22, 2017

lrlopez commented Mar 22, 2017

lrlopez commented Mar 27, 2017

lrlopez commented Mar 29, 2017 • edited Loading

fabpot commented Apr 1, 2017

GuilhemN commented Jul 21, 2016 •

edited

Loading

lrlopez commented Mar 29, 2017 •

edited

Loading