Fix inheritance bug where property attributes werent inherited

* Fix bug, where @Property[<-read>|<-write>] tags were not inherited. * Remove support for providing accessors to private properties. * Remove support for private endpoints.
marguskaidja · Nov 7, 2022 · fe5cce2 · fe5cce2
1 parent 295c166
commit fe5cce2
Show file tree

Hide file tree

Showing 5 changed files with 89 additions and 17 deletions.
diff --git a/README.md b/README.md
@@ -1,7 +1,7 @@
 [![Tests](https://github.com/marguskaidja/php-accessors/actions/workflows/tests.yml/badge.svg)](https://github.com/marguskaidja/php-accessors/actions/workflows/tests.yml)
 # Accessors
 
-Current library can create automatic accessors (e.g. _getters_ and _setters_) for object properties. It works by injecting a trait with [magic methods for property overloading](https://www.php.net/manual/en/language.oop5.overloading.php#language.oop5.overloading.members) into desired class, which will then handle situations, where _inaccessible_ (`private`/`protected`) property is accessed.
+Current library can create automatic accessors (e.g. _getters_ and _setters_) for object properties. It works by injecting a trait with [magic methods for property overloading](https://www.php.net/manual/en/language.oop5.overloading.php#language.oop5.overloading.members) into desired class, which will then handle situations, where _inaccessible_ (aka `protected`) property is accessed.
 
 #### Features
 * Multiple accessor **syntaxes**:
@@ -361,11 +361,12 @@ The 2 endpoints (`getFoo`/`setFoo`) will be called in every situation:
 * either when property is accessed with direct assignment syntax (e.g. `$a->foo`)
 * or when property is accessed with method syntax (e.g. `$a->getFoo()`)
     * if the visibility of endpoint is `public`, then it's naturally called by PHP engine.
-    * if it's `private`/`protected`, then it goes through `__call` magic method provided by `Accessible` trait.
+    * if it's `protected`, then it goes through `__call` magic method provided by `Accessible` trait.
 
 Notes:
 * Endpoint methods must start with string `set`, `get`, `isset`, `unset` or `with` and followed with property name.
 * Only instance methods are detected, `static` methods wont work.
+* Only `public` or `protected` methods are detected, `private` methods wont work.
 * _mutator_ is bypassed and should be done inside the setter endpoint itself.
 * Return values from endpoints are handled as following. Values from:
     * `get` and `isset` are handed over to caller.
@@ -572,6 +573,9 @@ Since `@property[<-read>|<-write>]` tags act also in exposing properties, you ge
    * `#[Immutable]`: turns an property or whole class immutable. When used on a class, then it must be defined on top of hierarchy. Once defined, it can't be disabled later.
    * `#[Format(class-string<FormatContract>)]`: allows customization of accessor method names.
 
+Notes:
+* Only `protected` properties are supported. Due current library's implementation, `private` properties may introduce unexpected behaviour and anomalies in inheritance chain and are disregarded this time.
+
 ### Accessor methods:
 
 #### Reading properties:

diff --git a/src/ClassConf.php b/src/ClassConf.php
@@ -115,6 +115,7 @@ private function __construct(
                 }
 
                 $this->attributes = $this->attributes->mergeWithParent($parent->attributes);
+                $parentProperties = $parent->properties;
             } else {
                 /**
                  * Since this is the top of the hierachy using "Accessible" trait, assign default instance
@@ -124,9 +125,11 @@ private function __construct(
                     Format::class,
                     new Format(Standard::class)
                 );
+
+                $parentProperties = null;
             }
 
-            $this->properties = Properties::fromReflection($rfClass, $this->attributes);
+            $this->properties = Properties::fromReflection($rfClass, $this->attributes, $parentProperties);
 
             $this->getter = $this->createGetter();
             $this->setter = $this->createSetter();

diff --git a/src/Properties.php b/src/Properties.php
@@ -39,10 +39,15 @@ class Properties
     /**
      * @param  ReflectionClass<object>  $rfClass
      * @param  Attributes               $classAttributes
+     * @param  Properties|null          $parentProperties
      *
      * @return self
      */
-    public static function fromReflection(ReflectionClass $rfClass, Attributes $classAttributes): self
+    public static function fromReflection(
+        ReflectionClass $rfClass,
+        Attributes $classAttributes,
+        ?Properties $parentProperties
+    ): self
     {
         $that = new self();
 
@@ -62,10 +67,13 @@ public static function fromReflection(ReflectionClass $rfClass, Attributes $clas
 
         foreach (
             $rfClass->getMethods(
-                ReflectionMethod::IS_PROTECTED | ReflectionMethod::IS_PRIVATE | ReflectionMethod::IS_PUBLIC
+                ReflectionMethod::IS_PROTECTED | ReflectionMethod::IS_PUBLIC
             ) as $rfMethod
         ) {
-            if ($rfMethod->isStatic()) {
+            if (
+                $rfMethod->isStatic()
+                || $rfClass->name !== $rfMethod->getDeclaringClass()->name
+            ) {
                 continue;
             }
 
@@ -87,25 +95,34 @@ public static function fromReflection(ReflectionClass $rfClass, Attributes $clas
          */
         foreach (
             $rfClass->getProperties(
-                ReflectionProperty::IS_PRIVATE | ReflectionProperty::IS_PROTECTED | ReflectionProperty::IS_PUBLIC
+                ReflectionProperty::IS_PROTECTED | ReflectionProperty::IS_PUBLIC
             ) as $rfProperty
         ) {
             $name = $rfProperty->getName();
             $nameLowerCase = strtolower($name);
 
-            if (isset($docBlockAttributes[$name])) {
-                $attributes = $docBlockAttributes[$name]->mergeWithParent($classAttributes);
+            if (
+                null === $parentProperties
+                || $rfClass->name === $rfProperty->getDeclaringClass()->name
+            ) {
+                if (isset($docBlockAttributes[$name])) {
+                    $attributes = $docBlockAttributes[$name]->mergeWithParent($classAttributes);
+                } else {
+                    $attributes = $classAttributes;
+                }
+
+                $p = new Property(
+                    $rfProperty,
+                    $attributes,
+                    ($accessorEndpoints[$nameLowerCase] ?? [])
+                );
             } else {
-                $attributes = $classAttributes;
+                $p = $parentProperties->findConf($name);
             }
 
-            $p = new Property(
-                $rfProperty,
-                $attributes,
-                ($accessorEndpoints[$nameLowerCase] ?? [])
-            );
-
-            $that->propertiesByLowerCase[$nameLowerCase] = ($that->properties[$name] = $p);
+            if (null !== $p) {
+                $that->propertiesByLowerCase[$nameLowerCase] = ($that->properties[$name] = $p);
+            }
         }
 
         return $that;

diff --git a/tests/DocBlockTest.php b/tests/DocBlockTest.php
@@ -102,4 +102,19 @@ public function getP1Value(): string
          */
         $obj->p1;
     }
+
+    public function test_tag_in_parent_class_must_be_inherited(): void
+    {
+        $obj = new class extends ParentTestClassWithPropertyAttributes {
+        };
+
+        $expectedValue = 'new value';
+        $obj->setParentProperty($expectedValue);
+
+        $this->assertEquals(
+            $expectedValue,
+            $obj->getParentProperty()
+        );
+    }
+
 }
diff --git a/tests/ParentTestClassWithPropertyAttributes.php b/tests/ParentTestClassWithPropertyAttributes.php
@@ -0,0 +1,33 @@
+<?php
+
+/**
+ * This file is part of the margusk/accessors package.
+ *
+ * @author  Margus Kaidja <[email protected]>
+ * @link    https://github.com/marguskaidja/php-accessors
+ * @license http://www.opensource.org/licenses/mit-license.php MIT (see the LICENSE file)
+ */
+
+declare(strict_types=1);
+
+namespace margusk\Accessors\Tests;
+
+use margusk\Accessors\Accessible;
+use margusk\Accessors\Attr\Delete;
+use margusk\Accessors\Attr\Get;
+use margusk\Accessors\Attr\Set;
+
+use function htmlspecialchars;
+
+/**
+ * @property string $parentProperty
+ *
+ * @method string getParentProperty()
+ * @method self setParentProperty(string $value)
+ */
+class ParentTestClassWithPropertyAttributes
+{
+    use Accessible;
+
+    protected string $parentProperty;
+}