[PropertyAccess] Allow custom property accessors #22190

lrlopez · 2017-03-27T22:17:42Z

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

Here is the first of the two PR related to #18016. This implements changes on the PropertyAccess component and will be followed by another one leveraging the new features into the FrameworkBundle.

Now it is possible to override the property accessors (getters, setters, adders and removers) using annotations or YAML config files. Metadata is automatically cached in the pool configured for the property path cache.

Some examples of how to use the new feature:

Annotations:

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

/**
 * @PropertyAccessor(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
}

// Notice that "calculated" is not a real property!
/**
 * @GetterAccessor(property="calculated")
 */
public function getCalculated()
{
    // whatever...
}

// You can omit 'property' in method annotations
/**
 * @SetterAccessor("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>

lrlopez · 2017-03-27T23:22:02Z

src/Symfony/Component/PropertyAccess/Tests/Mapping/ClassMetadataTest.php

+    {
+        $classMetadata = new ClassMetadata('c');
+
+        $a1 = $this->createMock('Symfony\Component\PropertyAccess\Mapping\PropertyMetadata');


This seems to fail on PHP 5.5 & PHPUnit 4.8.35. ClassMetadataTest extends PHPUnit\Framework\TestCase which implements that method, so I'm not sure on how to solve this. It doesn't fail on any other scenarios. Any ideas?

Koc · 2017-03-28T12:59:25Z

totally agree with you that Symfony needs some component for reading metadata.

lrlopez · 2017-03-29T20:22:52Z

Ok. I think this PR is ready for review. Did we make the cut for Symfony 3.3? 😅

HeahDude · 2017-04-09T14:46:18Z

src/Symfony/Component/PropertyAccess/Mapping/PropertyMetadata.php

+    {
+        // Overwrite only if not defined
+        if (null === $this->getter) {
+            $this->getter = $propertyMetadata->getGetter();


The function call is not needed here.

HeahDude · 2017-04-09T14:47:35Z

src/Symfony/Component/PropertyAccess/Mapping/PropertyMetadata.php

+    /**
+     * Merges another PropertyMetadata with the current one.
+     *
+     * @param PropertyMetadata $propertyMetadata


@param self $propertyMetadata .

Actually it could be self in the signature as well but this is not common in the code base, so I don't if it should be done.

HeahDude · 2017-04-09T14:50:03Z

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

@@ -912,7 +936,7 @@ private function getPropertyPath($propertyPath)
     *
     * @return AdapterInterface
     *
-     * @throws RuntimeException When the Cache Component isn't available
+     * @throws \RuntimeException When the Cache Component isn't available


This change should not target master.

I don't recall making this change, I'll revert it. Thank you!

HeahDude · 2017-04-09T14:50:57Z

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

+    private function getPropertyMetadata($class, $property)
+    {
+        if ($this->cacheItemPool) {
+            $key = $class.'\\'.$property;


Why not using a dot as separator directly since it's gonna be replaced?

HeahDude · 2017-04-09T14:56:11Z

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

+        if ($this->classMetadataFactory) {
+            $classMetadata = $this->classMetadataFactory->getMetadataFor($class);
+            if ($classMetadata) {
+                $metadataCollection = $classMetadata->getPropertyMetadataCollection();


You should add a getMetadataForProperty($property) method that would return the metadata or null and remove the overhead below, the block could then look like:

$metadata = null; if ($this->classMetadataFactory && $classMetadata = $this->classMetadataFactory->getMetadataFor($class) ) { $metadata = $classMetadata->getMetadataForProperty($property); } return $metadata;

HeahDude · 2017-04-09T15:02:47Z

src/Symfony/Component/PropertyAccess/Mapping/ClassMetadata.php

+    /**
+     * Merges a {@link ClassMetadata} into the current one.
+     *
+     * @param ClassMetadata $classMetadata


@param self $classMetadata

HeahDude · 2017-04-09T15:08:13Z

src/Symfony/Component/PropertyAccess/Mapping/Factory/LazyLoadingMetadataFactory.php

+
+        $class = ltrim(is_object($value) ? get_class($value) : $value, '\\');
+
+        if (class_exists($class) || interface_exists($class)) {


return class_exists($class) || interface_exists($class)?

HeahDude · 2017-04-09T15:23:23Z

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

    {
+        if ($metadata && $metadata->getAdder() && $metadata->getRemover()) {


I couldn't find anything in this PR related to this requirement, it should be clear somewhere that in one is set this other is required.
Although I feel like one should should be enough, perhaps allowing to mix adder and remover guessing?

I'm just mimicking the actual PropertyAccess behavior, in which both methods must be defined to activate this kind of accessors. You're right, docs should warn about this.

I'll try to fix one of the accessor and let the component guess the other one, seems reasonable

Thanks! It would be awesome if it was covered by tests too to prevent regression.

lrlopez · 2017-04-09T17:56:23Z

@HeahDude, thank you very much for the review!

HeahDude · 2017-04-09T19:16:48Z

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

@@ -133,6 +141,11 @@ class PropertyAccessor implements PropertyAccessorInterface
    private $cacheItemPool;

    /**
+     * @var MetadataFactoryInterface


Missing |null.

It is really necessary? Then it should be also added to $classMetadataFactory above...

HeahDude · 2017-04-09T21:39:51Z

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

@@ -158,11 +171,12 @@ class PropertyAccessor implements PropertyAccessorInterface
     * @param bool                   $throwExceptionOnInvalidIndex
     * @param CacheItemPoolInterface $cacheItemPool


You're right, same here, missing the new argument by the way ;).

Fixed! Thanks

lrlopez · 2017-04-10T00:12:12Z

Annotation names changed as per @HeahDude request. Suggestions welcomed!

xabbuh · 2017-10-03T12:44:40Z

The CI builds don't seem to be happy. Can you have a look at the failing tests?

lrlopez · 2017-10-03T16:50:05Z

Sure!

lrlopez · 2017-10-03T18:40:01Z

Done

nicolas-grekas · 2017-10-08T15:55:35Z

ping @xabbuh @dunglas @symfony/deciders it looks like this one is ready, any review or should we move to 4.1?

dunglas · 2017-10-08T17:09:07Z

src/Symfony/Component/PropertyAccess/Mapping/ClassMetadata.php

+     *
+     * @param PropertyMetadata $propertyMetadata
+     */
+    public function addPropertyMetadata(PropertyMetadata $propertyMetadata)


Do you think we can make this value object immutable? We do that for new VO (PSR-7 style, see the PropertyInfo component for instance).

It could be done. But I'm curious about what would be the benefits. This class is used internally only, it's not exposed outside the component. Some parts of this code are borrowed from the Serializer component but I infer that the current implementation intent is to allow fast & easy merging of metadata from different sources and classes in the hierarchy.

xabbuh · 2017-10-08T17:33:12Z

I wonder if we should also take the changes proposed in #23789 into account.

chalasr · 2017-11-09T12:38:30Z

3.4 does not accept features anymore.
Moving to 4.1, allowing to use PHP7.1 features such as scalar typehints.

lrlopez · 2017-11-09T15:12:03Z

It's a shame. Well, if someone wants to take this PR over, please go ahead. I don't think I'll be able to invest more time into it (too many rebases and changes since 3.1) at least until next summer.

joelwurtz · 2019-03-26T10:54:14Z

I believe #30248 will help here, as it introduce the concept of extracting Accessor / Extractor from the metadata

…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 · 2020-01-31T14:09:25Z

#30704 has been merged

fabpot · 2020-10-07T15:55:41Z

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

carsonbot added Status: Needs Review PropertyAccess Feature labels Mar 27, 2017

lrlopez force-pushed the property-access-component branch 4 times, most recently from 77b0366 to 66e6b48 Compare March 27, 2017 23:10

lrlopez commented Mar 27, 2017

View reviewed changes

lrlopez mentioned this pull request Mar 27, 2017

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

Closed

nicolas-grekas added this to the 3.x milestone Mar 28, 2017

lrlopez changed the title ~~[PropertyAccess] Allow custom property accessors (WIP)~~ [PropertyAccess] Allow custom property accessors Mar 29, 2017

lrlopez mentioned this pull request Mar 29, 2017

[PropertyAccess] Allow custom methods on property accesses #18016

Closed

4 tasks

HeahDude reviewed Apr 9, 2017

View reviewed changes

lrlopez force-pushed the property-access-component branch from 66e6b48 to d3eb07e Compare April 9, 2017 17:55

lrlopez force-pushed the property-access-component branch 5 times, most recently from ce94b7d to 0691fb2 Compare April 9, 2017 18:54

HeahDude reviewed Apr 9, 2017

View reviewed changes

lrlopez force-pushed the property-access-component branch from 0691fb2 to 58aad1f Compare April 9, 2017 21:37

HeahDude reviewed Apr 9, 2017

View reviewed changes

lrlopez force-pushed the property-access-component branch from 58aad1f to 5e8f0dd Compare April 9, 2017 21:56

lrlopez mentioned this pull request Apr 9, 2017

[PropertyAccess] Custom methods on property accesses symfony/symfony-docs#6400

Closed

lrlopez force-pushed the property-access-component branch 2 times, most recently from 3ec7ce5 to 2d9fc7a Compare April 10, 2017 00:10

lrlopez force-pushed the property-access-component branch from e27b1b6 to 72943a1 Compare October 3, 2017 18:37

lrlopez force-pushed the property-access-component branch from 72943a1 to 2f2ad49 Compare October 3, 2017 18:40

dunglas reviewed Oct 8, 2017

View reviewed changes

lrlopez force-pushed the property-access-component branch 2 times, most recently from c228dbd to 146916b Compare November 6, 2017 08:54

Added custom property accessors

e6aa7f1

lrlopez force-pushed the property-access-component branch from 146916b to e6aa7f1 Compare November 6, 2017 09:01

chalasr modified the milestones: 3.4, 4.1 Nov 9, 2017

nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

ogizanagi mentioned this pull request Jul 12, 2018

[Serializer] Allow to provide a "normalizer" callable to normalize a property during denormalization #27933

Open

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

fabpot removed the ⭐️ EUFOSSA Hackathon label Apr 8, 2019

nicolas-grekas changed the base branch from 3.4 to master June 15, 2020 07:08

fabpot closed this Oct 7, 2020

nicolas-grekas added the for 5.x label Oct 7, 2020

lrlopez mentioned this pull request Oct 11, 2020

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

Closed


		$class = ltrim(is_object($value) ? get_class($value) : $value, '\\');

		if (class_exists($class) \|\| interface_exists($class)) {

		{
		if ($metadata && $metadata->getAdder() && $metadata->getRemover()) {

		@@ -158,11 +171,12 @@ class PropertyAccessor implements PropertyAccessorInterface
		* @param bool $throwExceptionOnInvalidIndex
		* @param CacheItemPoolInterface $cacheItemPool

Uh oh!

[PropertyAccess] Allow custom property accessors #22190

[PropertyAccess] Allow custom property accessors #22190

Uh oh!

Conversation

lrlopez commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Koc commented Mar 28, 2017

Uh oh!

lrlopez commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeahDude Apr 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrlopez commented Apr 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrlopez commented Apr 10, 2017

Uh oh!

xabbuh commented Oct 3, 2017

Uh oh!

lrlopez commented Oct 3, 2017

Uh oh!

lrlopez commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Oct 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Oct 8, 2017

Uh oh!

chalasr commented Nov 9, 2017

Uh oh!

lrlopez commented Nov 9, 2017

Uh oh!

joelwurtz commented Mar 26, 2019

Uh oh!

lrlopez commented Mar 27, 2017 •

edited

Loading

HeahDude Apr 9, 2017 •

edited

Loading

lrlopez commented Apr 9, 2017 •

edited

Loading

lrlopez commented Oct 3, 2017 •

edited

Loading