diff --git a/src/View/ViewableData.php b/src/View/ViewableData.php index a8340536f..9dcea9267 100644 --- a/src/View/ViewableData.php +++ b/src/View/ViewableData.php @@ -7,7 +7,9 @@ use Exception; use InvalidArgumentException; use IteratorAggregate; use LogicException; +use ReflectionMethod; use ReflectionObject; +use ReflectionProperty; use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Config\Config; use SilverStripe\Core\Config\Configurable; @@ -111,7 +113,7 @@ class ViewableData implements IteratorAggregate public function __isset($property) { // getField() isn't a field-specific getter and shouldn't be treated as such - if (strtolower($property ?? '') !== 'field' && $this->hasMethod($method = "get$property")) { + if (strtolower($property ?? '') !== 'field' && $this->hasMethod("get$property")) { return true; } if ($this->hasField($property)) { @@ -134,7 +136,8 @@ class ViewableData implements IteratorAggregate public function __get($property) { // getField() isn't a field-specific getter and shouldn't be treated as such - if (strtolower($property ?? '') !== 'field' && $this->hasMethod($method = "get$property")) { + $method = "get$property"; + if (strtolower($property ?? '') !== 'field' && $this->hasMethod($method) && $this->isAccessibleMethod($method)) { return $this->$method(); } if ($this->hasField($property)) { @@ -159,20 +162,13 @@ class ViewableData implements IteratorAggregate $this->objCacheClear(); $method = "set$property"; - if ($this->hasMethod($method) && !$this->isPrivate($this, $method)) { + if ($this->hasMethod($method) && $this->isAccessibleMethod($method)) { $this->$method($value); } else { $this->setField($property, $value); } } - private function isPrivate(object $class, string $method): bool - { - $class = new ReflectionObject($class); - - return $class->getMethod($method)->isPrivate(); - } - /** * Set a failover object to attempt to get data from if it is not present on this object. * @@ -218,7 +214,7 @@ class ViewableData implements IteratorAggregate */ public function getField($field) { - if (property_exists($this, $field)) { + if ($this->isAccessibleProperty($field)) { return $this->$field; } return $this->data[$field]; @@ -237,13 +233,45 @@ class ViewableData implements IteratorAggregate // prior to PHP 8.2 support ViewableData::setField() simply used `$this->field = $value;` // so the following logic essentially mimics this behaviour, though without the use // of now deprecated dynamic properties - if (property_exists($this, $field)) { + if ($this->isAccessibleProperty($field)) { $this->$field = $value; } $this->data[$field] = $value; return $this; } + /** + * Returns true if a method exists for the current class which isn't private. + * Also returns true for private methods if $this is ViewableData (not a subclass) + */ + private function isAccessibleMethod(string $method): bool + { + if (!method_exists($this, $method)) { + return false; + } + if (static::class === self::class) { + return true; + } + $reflectionMethod = new ReflectionMethod($this, $method); + return !$reflectionMethod->isPrivate(); + } + + /** + * Returns true if a property exists for the current class which isn't private. + * Also returns true for private properties if $this is ViewableData (not a subclass) + */ + private function isAccessibleProperty(string $property): bool + { + if (!property_exists($this, $property)) { + return false; + } + if (static::class === self::class) { + return true; + } + $reflectionProperty = new ReflectionProperty($this, $property); + return !$reflectionProperty->isPrivate(); + } + // ----------------------------------------------------------------------------------------------------------------- /** diff --git a/tests/php/View/ViewableDataTest.php b/tests/php/View/ViewableDataTest.php index 9c4e3d48e..c536b9ad1 100644 --- a/tests/php/View/ViewableDataTest.php +++ b/tests/php/View/ViewableDataTest.php @@ -208,16 +208,47 @@ class ViewableDataTest extends SapphireTest $this->assertFalse($container->hasMethod('testMethod'), 'testMethod() incorrectly reported as existing'); } - public function testIsPrivate() + public function testIsAccessibleMethod() { - $reflectionMethod = new ReflectionMethod(ViewableData::class, 'isPrivate'); + $reflectionMethod = new ReflectionMethod(ViewableData::class, 'isAccessibleMethod'); $reflectionMethod->setAccessible(true); $object = new ViewableDataTestObject(); - - $output = $reflectionMethod->invokeArgs($object, [$object, 'privateMethod']); - $this->assertTrue($output, 'Method is not private'); - - $output = $reflectionMethod->invokeArgs($object, [$object, 'publicMethod']); - $this->assertFalse($output, 'Method is private'); + + $output = $reflectionMethod->invokeArgs($object, ['privateMethod']); + $this->assertFalse($output, 'Method should not be accessible'); + + $output = $reflectionMethod->invokeArgs($object, ['protectedMethod']); + $this->assertTrue($output, 'Method should be accessible'); + + $output = $reflectionMethod->invokeArgs($object, ['publicMethod']); + $this->assertTrue($output, 'Method should be accessible'); + + $output = $reflectionMethod->invokeArgs($object, ['missingMethod']); + $this->assertFalse($output, 'Method should not be accessible'); + + $output = $reflectionMethod->invokeArgs(new ViewableData(), ['isAccessibleProperty']); + $this->assertTrue($output, 'Method should be accessible'); + } + + public function testIsAccessibleProperty() + { + $reflectionMethod = new ReflectionMethod(ViewableData::class, 'isAccessibleProperty'); + $reflectionMethod->setAccessible(true); + $object = new ViewableDataTestObject(); + + $output = $reflectionMethod->invokeArgs($object, ['privateProperty']); + $this->assertFalse($output, 'Property should not be accessible'); + + $output = $reflectionMethod->invokeArgs($object, ['protectedProperty']); + $this->assertTrue($output, 'Property should be accessible'); + + $output = $reflectionMethod->invokeArgs($object, ['publicProperty']); + $this->assertTrue($output, 'Property should be accessible'); + + $output = $reflectionMethod->invokeArgs($object, ['missingProperty']); + $this->assertFalse($output, 'Property should not be accessible'); + + $output = $reflectionMethod->invokeArgs(new ViewableData(), ['objCache']); + $this->assertTrue($output, 'Property should be accessible'); } } diff --git a/tests/php/View/ViewableDataTestObject.php b/tests/php/View/ViewableDataTestObject.php index 08ad8fe96..90878d80f 100644 --- a/tests/php/View/ViewableDataTestObject.php +++ b/tests/php/View/ViewableDataTestObject.php @@ -7,11 +7,22 @@ use SilverStripe\ORM\DataObject; class ViewableDataTestObject extends DataObject implements TestOnly { + private string $privateProperty = 'private property'; + + protected string $protectedProperty = 'protected property'; + + public string $publicProperty = 'public property'; + private function privateMethod(): string { return 'Private function'; } + protected function protectedMethod(): string + { + return 'Protected function'; + } + public function publicMethod(): string { return 'Public function';