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

Bogdanp · 2012-08-21T16:24:19Z

When using Symfony Forms, it permits the implementation of magic getter methods in those Entities by using the __call magic method.

It basically allows one to do the following in an Entity:

public function __call($method, $arguments) {
    if (preg_match('/^(get|is|has)([A-Z].*)$/', $method, $matches)) {
        return $this->{lcfirst($matches[2])};
    }
    throw new \Exception(sprintf(
        'Method does not exist for class "%s".', get_class($this)
    ));
}

When using Symfony Forms, it permits the implementation of magic getter methods in those Entities by using the `__call` magic method. It basically allows one to do the following in an Entity: public function __call($method, $arguments) { if (preg_match('/^(get|is|has)([A-Z].*)$/', $method, $matches)) { return $this->{lcfirst($matches[2])}; } throw new \Exception(sprintf( 'Method does not exist for class "%s".', get_class($this) )); }

travisbot · 2012-08-21T16:24:41Z

This pull request fails (merged 6b92474 into 173f23d).

Bogdanp · 2012-08-21T16:31:23Z

All three travisbot machines seems to have failed due to powerDown and not the patch. Is there any way to have it re-run them?

stof · 2012-08-21T16:32:57Z

Please add some tests (for each cases of your elseif)

Tobion · 2012-08-21T17:02:43Z

src/Symfony/Component/Form/Util/PropertyPath.php

+                    $result[self::VALUE] = $objectOrArray->$hasser();
+                } else {
+                    throw new InvalidPropertyException(sprintf('Neither property "%s" nor method "%s()" nor method "%s()" exists in class "%s"', $property, $getter, $isser, $reflClass->name));
+                }


Exception message is wrong (property).

Would you mind explaining? I don't understand.

Edit: Actually, I think I get what you mean.

I just meant, that the exception message mentions the "property %s" but __call is not for properties but methods.
Btw, why do you need the is_callable test when you already have tested for __call?

The is_callable calls are needed to be able to tell if those magic "methods" were actually implemented or not on that class. You should see the latest commit, it's probably a bit clearer in that :).

Edit: For clarification,method_exists or .hasMethod can't tell whether or not those methods exist.
Edit2: Thinking about it, that was a bit of a brain fart since Is_callable will always return true due to the __call method.

Bogdanp · 2012-08-21T17:10:19Z

Added some tests.

A note: even after running composer install I was unable to run the tests since PHP would fail because it would try to load something from Doctrine/orm/lib which isn't listed as a dependency in composer.json. In the end, I added it as a dependecy and everything ran smoothly. Am I missing something?

travisbot · 2012-08-21T17:15:23Z

This pull request passes (merged 3252a3b into 173f23d).

stof · 2012-08-21T17:16:21Z

@Bogdanp to run the testsuite, you need to ask composer to install the dev requirements too by adding the --dev option (dev requirements are dependencies needing by the testsuite without being mandatory dependencies of the library)

This allows Entities to have __call methods that don't necessarily "implement" any of the magic getter methods.

Bogdanp · 2012-08-21T17:18:42Z

Alright, got it :). Thanks. Is there some place where this stuff is documented? I had checked the getting started contributing page but it said nothing about the --dev option.

stof · 2012-08-21T17:22:31Z

then it should be added in the doc.

Btw, a good way to know what is required to setup the testsuite when a project uses travis is to look at the .travis.yml file :)

Bogdanp · 2012-08-21T17:25:02Z

Good to know!

travisbot · 2012-08-21T17:30:37Z

This pull request passes (merged 2894aa0 into 173f23d).

stof · 2012-08-21T18:08:28Z

Please also test the case of an unsupported method (which will not behave properly IMO) and fix the coding standards

Bogdanp · 2012-08-21T18:26:03Z

You are right, it won't. I hadn't considered that is_callable will always pass because of the __call method (I actually had a "d'oh" moment when I realized that). I'll have to come up with a better solution after I get home since I'm at the office right now.

stof · 2012-08-21T19:35:38Z

the point is, as soon as __call is used, there is no way to determine if the method can be handled by the magic method or no

Bogdanp · 2012-08-21T20:25:33Z

Yes, that is what I was saying in my previous comment.

The fix should be in the last two commits, simply checking for the methods themselves should be fine and moving them to the bottom of the condition stack should prevent any exceptions or errors generated by __call to be thrown before all the other options have been exhausted. There is no need (at least IMO) to check for the case in which __call is defined but its purpose is not generating magic getter methods since that case should be handled by the author of the Entity anyway (they should throw an error if the method call doesn't match their specific case).

travisbot · 2012-08-21T20:28:02Z

This pull request passes (merged 73ecbf1 into 173f23d).

travisbot · 2012-08-21T20:33:31Z

This pull request passes (merged 616f121 into 173f23d).

stof · 2012-08-21T21:44:07Z

Please add a test for a class which does not support the magic getter (throwing an exception in it) but support the magic isser. I can assure you it will fail by throwing you the exception

stof · 2012-08-21T21:45:49Z

in fact, you test does not allow testing all cases: you use the same class supporting all 3 magic methods. But this does not allow you to know which case was called. You need 3 classes, one for each case

Bogdanp · 2012-08-22T04:32:01Z

You are correct, there is no proper way to determine which method should be called or if they are callable and in the case that the author of the Entity has written a __call method which throws an error it will fail. The only solution in my mind is to go back to testing if the object has a __call method and calling the getter, isser and hasser each inside of a try-catch block. The problem with this case is that there is no guarantee that the author of the Entity has made is so that an actual exception is thrown when a "method" does not exist. Would you consider this to be a valid solution?

Tobion · 2012-08-22T04:48:54Z

Please add a test for a class which does not support the magic getter (throwing an exception in it) but support the magic isser. I can assure you it will fail by throwing you the exception

@stof I don't think we should support a magic isser when is magic getter already throwed an exception. The only solution that would support this, is catching exceptions as @Bogdanp suggested and assuming __call would raise an exception in this case.

I'd suggest to simply invoke _call with the plain property value (no getter or isser variation) when there is a __call defined.

stof · 2012-08-22T07:25:45Z

@Tobion It makes no sense to call it with the plain property value. If you use magic methods to implement getters, we cannot assume you implemented it by calling the method the same way than the property (which would not be consistent with what we do for other cases btw).

The point is, we cannot support __call without making a bunch of assumptions about the way this method was implemented (expecting it to throw an exception for invalid methods, and being side-effect free)

Bogdanp · 2012-08-22T07:44:42Z

My question is, should I implement it the way I described above?

I think that in order to avoid confusion we can either keep the Exception that was thrown by __call and then throw it ourselves after none of the methods were found (though in this case there might be some confusion since the exception thrown for each of the three methods will probably be different) or we can throw a custom one describing what has likely happened.

Regarding the __call method as a whole, simply implementing it in a complex system requires some extra attention on the part of the developer doing it and as such I don't think it's outlandish to expect them to throw an exception when their specific case was not met.

webmozart · 2013-04-18T13:26:23Z

This implementation unfortunately is obsolete since the PropertyAccess component was introduced. Please feel welcome to reimplement it for that component. Simply open a new PR and we will check it again. :)

See also #7263.

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

This PR was merged into the 7.1 branch. Discussion ---------- [Notifier] Add Unifonic notifier bridge Add docs for symfony/symfony#5309 Fixes #19301 Commits ------- 9e47f5f [Notifier] Add Unifonic notifier bridge

Tobion reviewed Aug 21, 2012
View reviewed changes

Add a test for each of the getter magic method cases.

3252a3b

Un-nest the __call conditionals.

2894aa0

This allows Entities to have __call methods that don't necessarily "implement" any of the magic getter methods.

Bogdanp added 2 commits August 21, 2012 23:20

Remove the __call check.

73ecbf1

Move the magic "methods" to the bottom so the other getters run first.

616f121

rnijveld mentioned this pull request Oct 28, 2012

[Form] __call to get property value #5850

Closed

jaugustin mentioned this pull request Mar 4, 2013

[PropertyAccess] add support for magic call #7263

Merged

1 task

Tobion mentioned this pull request Mar 26, 2013

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

Closed

webmozart closed this Apr 18, 2013

OskarStark mentioned this pull request Dec 17, 2023

[Notifier] Add Unifonic notifier bridge symfony/symfony-docs#19300

Merged

Uh oh!

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

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

Uh oh!

Conversation

Bogdanp commented Aug 21, 2012

Uh oh!

travisbot commented Aug 21, 2012

Uh oh!

Bogdanp commented Aug 21, 2012

Uh oh!

stof commented Aug 21, 2012

Uh oh!

Tobion Aug 21, 2012

Choose a reason for hiding this comment

Uh oh!

Bogdanp Aug 21, 2012

Choose a reason for hiding this comment

Uh oh!

Tobion Aug 21, 2012

Choose a reason for hiding this comment

Uh oh!

Bogdanp Aug 21, 2012

Choose a reason for hiding this comment

Uh oh!

Bogdanp commented Aug 21, 2012

Uh oh!

travisbot commented Aug 21, 2012

Uh oh!

stof commented Aug 21, 2012

Uh oh!

Bogdanp commented Aug 21, 2012

Uh oh!

stof commented Aug 21, 2012

Uh oh!

Bogdanp commented Aug 21, 2012

Uh oh!

travisbot commented Aug 21, 2012

Uh oh!

stof commented Aug 21, 2012

Uh oh!

Bogdanp commented Aug 21, 2012

Uh oh!

stof commented Aug 21, 2012

Uh oh!

Bogdanp commented Aug 21, 2012

Uh oh!

travisbot commented Aug 21, 2012

Uh oh!

travisbot commented Aug 21, 2012

Uh oh!

stof commented Aug 21, 2012

Uh oh!

stof commented Aug 21, 2012

Uh oh!

Bogdanp commented Aug 22, 2012

Uh oh!

Tobion commented Aug 22, 2012

Uh oh!

stof commented Aug 22, 2012

Uh oh!

Bogdanp commented Aug 22, 2012

Uh oh!

webmozart commented Apr 18, 2013

Uh oh!

Uh oh!