merged branch jjbohn/feature/property-path-hasser (PR #3549)

fabpot · fabpot · commit 20a9961711a5 · 2012-04-06T15:09:38.000+02:00
Commits ------- b6ac1aa [FORM] Give PropertyPath ability to read hassers Discussion ---------- [Form] Give PropertyPath ability to read hassers Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: - Todo: - Using a `hasser` instead of `isser` for Boolean values is pretty common. I've found myself using `issers` a handful of times just to make an interface play nice with the form component, but the code reads funny now. I don't think we should be accounting for every possible `getter` variation, but I think this one is common enough that it warrants a discussion. --------------------------------------------------------------------------- by fabpot at 2012-03-11T08:25:31Z I tend to agree with with. What do you think @bschussek? --------------------------------------------------------------------------- by kriswallsmith at 2012-03-16T22:42:28Z I'm not so sure. There are lots of reasons to write a *hasser* that accepts an argument (i.e. `User::hasRole($role)`). Doesn't seem as clean as *issers* and *getters*. --------------------------------------------------------------------------- by vicb at 2012-03-16T22:49:14Z > There are lots of reasons to write a hasser that accepts an argument May be can check for 0 args as we are already using reflexion ? --------------------------------------------------------------------------- by kriswallsmith at 2012-03-16T22:55:43Z In that case we should check that there are either 0 arguments or only optional arguments and also consider adding the same logic to the other varieties. --------------------------------------------------------------------------- by jjbohn at 2012-03-16T23:37:47Z Passing arguments seems like a pretty big departure for PropertyPath. How would you annotate that? I'm not sure I see a common use case for needing arguments when mapping data to and from forms. --------------------------------------------------------------------------- by stof at 2012-03-16T23:50:22Z @jjbohn it is not about passing arguments but about using the hasser only if it does not have required arguments --------------------------------------------------------------------------- by jjbohn at 2012-03-17T01:54:18Z Ah. I see. I have a tendency to read @kriswallsmith comments wrong :D. I could see that but iirc, there's not any current check like this on the other accessors. Happy to add it though if there's a consensus. --------------------------------------------------------------------------- by fabpot at 2012-03-17T11:24:34Z What's the point is checking the hasser/getter/isser arguments. It's up to the developer to check if he can use them or not. Let's not complexify the code for this. --------------------------------------------------------------------------- by kriswallsmith at 2012-03-17T15:37:39Z My concern is that someone writes a hasser method on their model that is not intended for use with the form component but it's called anyway, leading to WTFs. --------------------------------------------------------------------------- by stof at 2012-04-03T22:28:21Z @fabpot what's your decision about this ? @jjbohn you need to rebase your PR. It conflicts with master as tests have been moved --------------------------------------------------------------------------- by bschussek at 2012-04-05T14:53:55Z @kriswallsmith is right. The check for 1 === $method->getNumberOfRequiredParameters() can (and should) easily be added to all of the if-clauses here. Apart from that, I'm okay with adding this.
diff --git a/src/Symfony/Component/Form/Tests/Fixtures/Author.php b/src/Symfony/Component/Form/Tests/Fixtures/Author.php
@@ -17,6 +17,7 @@ class Author
     private $lastName;
     private $australian;
     public $child;
+    private $readPermissions;
 
     private $privateProperty;
 
@@ -45,6 +46,16 @@ public function isAustralian()
         return $this->australian;
     }
 
+    public function setReadPermissions($bool)
+    {
+        $this->readPermissions = $bool;
+    }
+
+    public function hasReadPermissions()
+    {
+        return $this->readPermissions;
+    }
+
     private function isPrivateIsser()
     {
         return true;
diff --git a/src/Symfony/Component/Form/Tests/PropertyPathTest.php b/src/Symfony/Component/Form/Tests/PropertyPathTest.php
@@ -169,6 +169,16 @@ public function testGetValueReadsIssers()
         $this->assertFalse($path->getValue($object));
     }
 
+    public function testGetValueReadHassers()
+    {
+        $path = new PropertyPath('read_permissions');
+
+        $object = new Author();
+        $object->setReadPermissions(true);
+
+        $this->assertTrue($path->getValue($object));
+    }
+
     public function testGetValueReadsMagicGet()
     {
         $path = new PropertyPath('magicProperty');
diff --git a/src/Symfony/Component/Form/Util/PropertyPath.php b/src/Symfony/Component/Form/Util/PropertyPath.php
@@ -216,12 +216,12 @@ public function isIndex($index)
      *
      * This method first tries to find a public getter for each property in the
      * path. The name of the getter must be the camel-cased property name
-     * prefixed with "get" or "is".
+     * prefixed with "get", "is", or "has".
      *
      * If the getter does not exist, this method tries to find a public
      * property. The value of the property is then returned.
      *
-     * If neither is found, an exception is thrown.
+     * If none of them are found, an exception is thrown.
      *
      * @param  object|array $objectOrArray   The object or array to traverse
      *
@@ -332,6 +332,7 @@ protected function readProperty($object, $currentIndex)
             $reflClass = new \ReflectionClass($object);
             $getter = 'get'.$camelProp;
             $isser = 'is'.$camelProp;
+            $hasser = 'has'.$camelProp;
 
             if ($reflClass->hasMethod($getter)) {
                 if (!$reflClass->getMethod($getter)->isPublic()) {
@@ -345,6 +346,12 @@ protected function readProperty($object, $currentIndex)
                 }
 
                 return $object->$isser();
+            } elseif ($reflClass->hasMethod($hasser)) {
+                if (!$reflClass->getMethod($hasser)->isPublic()) {
+                    throw new PropertyAccessDeniedException(sprintf('Method "%s()" is not public in class "%s"', $hasser, $reflClass->getName()));
+                }
+
+                return $object->$hasser();
             } elseif ($reflClass->hasMethod('__get')) {
                 // needed to support magic method __get
                 return $object->$property;

Original file line number	Diff line number	Diff line change
`@@ -216,12 +216,12 @@ public function isIndex($index)`
`216`	`216`	`*`
`217`	`217`	`* This method first tries to find a public getter for each property in the`
`218`	`218`	`* path. The name of the getter must be the camel-cased property name`
`219`		`- * prefixed with "get" or "is".`
	`219`	`+ * prefixed with "get", "is", or "has".`
`220`	`220`	`*`
`221`	`221`	`* If the getter does not exist, this method tries to find a public`
`222`	`222`	`* property. The value of the property is then returned.`
`223`	`223`	`*`
`224`		`- * If neither is found, an exception is thrown.`
	`224`	`+ * If none of them are found, an exception is thrown.`
`225`	`225`	`*`
`226`	`226`	`* @param object\|array $objectOrArray The object or array to traverse`
`227`	`227`	`*`
`@@ -332,6 +332,7 @@ protected function readProperty($object, $currentIndex)`
`332`	`332`	`$reflClass = new \ReflectionClass($object);`
`333`	`333`	`$getter = 'get'.$camelProp;`
`334`	`334`	`$isser = 'is'.$camelProp;`
	`335`	`+ $hasser = 'has'.$camelProp;`
`335`	`336`
`336`	`337`	`if ($reflClass->hasMethod($getter)) {`
`337`	`338`	`if (!$reflClass->getMethod($getter)->isPublic()) {`
`@@ -345,6 +346,12 @@ protected function readProperty($object, $currentIndex)`
`345`	`346`	`}`
`346`	`347`
`347`	`348`	`return $object->$isser();`
	`349`	`+ } elseif ($reflClass->hasMethod($hasser)) {`
	`350`	`+ if (!$reflClass->getMethod($hasser)->isPublic()) {`
	`351`	`+ throw new PropertyAccessDeniedException(sprintf('Method "%s()" is not public in class "%s"', $hasser, $reflClass->getName()));`
	`352`	`+ }`
	`353`	`+`
	`354`	+ return $object->$hasser();
`348`	`355`	`} elseif ($reflClass->hasMethod('__get')) {`
`349`	`356`	`// needed to support magic method __get`
`350`	`357`	`return $object->$property;`