[PropertyAccess] add support for magic call #7263

jaugustin · 2013-03-04T23:56:00Z

Hi,

I add support for magic call with the PropertyAccess

the is basic implementation, if no getter, isser, or hasser or _get is found and there is __call then the PropertyAccess call the getter

the same for setter.

This functionality is disable by default

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	seems OK (failure/errors are the same on master)
Fixed tickets	#4683, #6413, #5309
License	MIT
Doc PR	symfony/symfony-docs#2472

submit changes to the documentation

@bschussek is this ok ?

stof · 2013-03-05T02:32:46Z

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

@@ -224,6 +253,9 @@ private function &readProperty(&$object, $property)
        } elseif ($reflClass->hasMethod('__get')) {
            // needed to support magic method __get
            $result[self::VALUE] = $object->$property;
+        } elseif ($this->magicCall && $reflClass->hasMethod('__call')) {


You should first check the public property before __call IMO to be more BC

@stof the problem with that is __call won't be called if you have a private property, because exception is raised.
This is disable by default so this is BC :)
maybe I can talk with @bschussek to refactor the way PropertyAccess throw exceptions without trying next case

vicb · 2013-03-05T20:39:41Z

No need for FQCN in phpdoc.
please use "Boolean" rather than bool
thanks

jaugustin · 2013-03-05T21:45:12Z

@vicb fixed ;)

jaugustin · 2013-04-06T11:03:58Z

I add the documentation for this PR

webmozart · 2013-04-18T14:47:50Z

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

+     *
+     * @return PropertyAccessor
+     */
+    public function enableMagicCall($enable = true)


Please remove the argument $enable and add a method disableMagicCall() instead.

Btw. both enableMagicCall() and disableMagicCall() should be moved to a new class PropertyAccessorBuilder. PropertyAccessor is immutable. (see how the same pattern is implemented in the Validator component)

I agree with @bschussek

jaugustin · 2013-04-20T18:29:58Z

@bschussek I created the PropertyAccessorBuilder, tell me if this is what you expected.

ghost · 2013-04-20T19:28:59Z

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

@@ -24,12 +24,15 @@ class PropertyAccessor implements PropertyAccessorInterface
    const VALUE = 0;
    const IS_REF = 1;

+    private $magicCall = false;


This is redundant if you are defining it in the constructor

webmozart · 2013-04-20T23:30:19Z

Looks good apart from the few mentioned issues! Please do also add this to the component's CHANGELOG.

webmozart · 2013-04-20T23:30:43Z

And please also squash your commits when you're done.

jaugustin · 2013-04-21T15:06:55Z

@bschussek naming fixed, commit squashed :)

fabpot · 2013-04-21T16:21:45Z

I don't understand why we would need PropertyAccessorBuilderInterface. Is it really needed?

jaugustin · 2013-04-21T16:56:17Z

@fabpot I create the builder like ValidatorBuilder was created and it has an interface so I do the same.

@bschussek what is your point of view on this ?

If it's not needed I can remove it :)

fabpot · 2013-04-21T17:03:06Z

Frankly, I don't understand why we have a ValidatorBuilderInterface either. It does not make sense to me.

webmozart · 2013-04-21T17:16:10Z

@fabpot You don't understand why we have the interface or why we have the builder?

We have the interface so we can type-hint against the builder without type-hinting against the implementation, nothing more.

We have the builder so that users can configure Validator/PropertyAccessor objects more easily, without depending on the exact argument order and default values of their constructors (see Builder Pattern). So basically it's the OO equivalent of

$validator = new Validator(array(
    // configuration
));

only a bit more convenient because your IDE tells you how to configure the builder, while it doesn't tell you which entries you can pass to the array.

webmozart · 2013-04-21T17:18:38Z

Also, configuration code can be extracted into the builder, as done in ValidatorBuilder for example.

webmozart · 2013-04-21T17:19:37Z

👍 on this PR

webmozart · 2013-04-21T17:21:58Z

Please add PropertyAccess::getPropertyAccessorBuilder() too.

fabpot · 2013-04-21T17:27:27Z

"We have the interface so we can type-hint against the builder without type-hinting against the implementation, nothing more." <- that part does not make sense to me. Why not type-hinting on the implementation for a builder? You will never have any other implementation anyway.

webmozart · 2013-04-21T17:29:42Z

You will never have any other implementation anyway.

I thought that so many times in other cases, until it happened :P Just to make sure.

add a new propertyAccessorBuilder to enable / disable the use of __call by the PropertyAccessor

jaugustin · 2013-04-21T17:35:19Z

@bschussek done

fabpot · 2013-04-21T17:37:30Z

I thought that so many times in other cases, until it happened :P Just to make sure.

Come on. Have a look at the builder, I can't buy that you will have another implementation. And anyway, even if you can come up with something, you can still extend the existing one. But having such a specialized interface for a builder seems overkill to me.

webmozart · 2013-04-21T17:44:02Z

Have a look at the builder, I can't buy that you will have another implementation.

The builder is very simple as of now because important functionalities are still missing, such as the generation and caching of class-specific property accessors, configurable property paths etc. With these things, the builder code will become more complicated as well.

I don't think that we in the core will have another implementation of the interface, but userland projects might well do if they want to add some functionality. I don't see why they should have to extend a class here when we provide interfaces everywhere else. After all, creating an interface doesn't cost us anything.

webmozart · 2013-04-23T09:06:15Z

I talked with @fabpot again and both of our sides have valid arguments. Apart from this specific case, it's a general decision for our API design, also in other places of the framework. I created a poll to collect feedback from the community.

stof · 2013-04-23T09:08:20Z

The issue with the interface for the builder is that the BC concern will prevent us adding anything new in the builder as it would be a BC break for other implementations.
Creating an interface costs something as soon as we look at BC.
IMO, the builder does not need an interface.
Thus, will you really inject the builder in your objects ? I think you should inject a PropertyAccessorInterface and use the builder outside your object to configure it

ghost · 2013-04-23T12:59:17Z

@stof has nailed the argument very well for this particular case.

webmozart · 2013-04-23T13:59:12Z

The poll ends tomorrow night (April 24 23:55 CET). I'll post the results then.

This PR was merged into the master branch. Discussion ---------- [PropertyAccess] add support for magic call Hi, I add support for magic call with the `PropertyAccess` the is basic implementation, if no `getter`, `isser`, or `hasser` or `_get` is found and there is `__call` then the PropertyAccess call the getter the same for setter. This functionality is disable by default | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | seems OK (failure/errors are the same on master) | Fixed tickets | #4683, #6413, #5309 | License | MIT | Doc PR | symfony/symfony-docs#2472 - [x] submit changes to the documentation @bschussek is this ok ? Commits ------- a785baa [PropertyAccess] add support for magic call, related to #4683

…erge (refs #7263)

stof reviewed Mar 5, 2013
View reviewed changes

Tobion mentioned this pull request Mar 26, 2013

Symfony Form component - not possible to use class with magic get method #7485

Closed

jaugustin mentioned this pull request Apr 6, 2013

Add documentation for the use of __call with propertyAccess symfony/symfony-docs#2472

Closed

webmozart mentioned this pull request Apr 18, 2013

[Form] Add magic getter method support to Entities #5309

Closed

webmozart reviewed Apr 18, 2013
View reviewed changes

webmozart mentioned this pull request Apr 18, 2013

[Form] [Util] [PropertyPath] Added __call method to property path #6413

Closed

ghost reviewed Apr 20, 2013
View reviewed changes

[PropertyAccess] add support for magic call, related to symfony#4683

a785baa

add a new propertyAccessorBuilder to enable / disable the use of __call by the PropertyAccessor

fabpot added a commit that referenced this pull request Apr 25, 2013

[PropertyAccess] removed an interface as discussion in the previous m…

e48e234

…erge (refs #7263)

fabpot merged commit a785baa into symfony:master Apr 25, 2013

Tobion mentioned this pull request Nov 24, 2013

variable functions for getter and setter #9600

Closed

Uh oh!

[PropertyAccess] add support for magic call #7263

[PropertyAccess] add support for magic call #7263

Uh oh!

Conversation

jaugustin commented Mar 4, 2013

Uh oh!

stof Mar 5, 2013

Choose a reason for hiding this comment

Uh oh!

jaugustin Mar 5, 2013

Choose a reason for hiding this comment

Uh oh!

webmozart Apr 18, 2013

Choose a reason for hiding this comment

Uh oh!

vicb commented Mar 5, 2013

Uh oh!

jaugustin commented Mar 5, 2013

Uh oh!

jaugustin commented Apr 6, 2013

Uh oh!

webmozart Apr 18, 2013

Choose a reason for hiding this comment

Uh oh!

webmozart Apr 18, 2013

Choose a reason for hiding this comment

Uh oh!

ghost Apr 20, 2013

Choose a reason for hiding this comment

Uh oh!

jaugustin commented Apr 20, 2013

Uh oh!

ghost Apr 20, 2013

Choose a reason for hiding this comment

Uh oh!

webmozart commented Apr 20, 2013

Uh oh!

webmozart commented Apr 20, 2013

Uh oh!

jaugustin commented Apr 21, 2013

Uh oh!

fabpot commented Apr 21, 2013

Uh oh!

jaugustin commented Apr 21, 2013

Uh oh!

fabpot commented Apr 21, 2013

Uh oh!

webmozart commented Apr 21, 2013

Uh oh!

webmozart commented Apr 21, 2013

Uh oh!

webmozart commented Apr 21, 2013

Uh oh!

webmozart commented Apr 21, 2013

Uh oh!

fabpot commented Apr 21, 2013

Uh oh!

webmozart commented Apr 21, 2013

Uh oh!

jaugustin commented Apr 21, 2013

Uh oh!

fabpot commented Apr 21, 2013

Uh oh!

webmozart commented Apr 21, 2013

Uh oh!

webmozart commented Apr 23, 2013

Uh oh!

stof commented Apr 23, 2013

Uh oh!

ghost commented Apr 23, 2013

Uh oh!

webmozart commented Apr 23, 2013

Uh oh!

Uh oh!