From a443a52e33d0dbe1fa7d5919eb839da05d8753af Mon Sep 17 00:00:00 2001 From: WalterWoshid Date: Sat, 5 Sep 2026 17:59:59 +0200 Subject: [PATCH 1/3] Preserve private property scope in woven class hierarchies --- PROPERTY_ACCESS.md | 60 +++++++ README.md | 7 +- .../plans/2026-09-05-private-properties.md | 34 ++++ src/Core/Transform/ProxiedClassModifier.php | 18 ++ src/Core/Transform/WovenClassBuilder.php | 17 +- src/PropertyAccess.php | 75 ++++++++ .../Include/Aspect/DatabaseModifierAspect.php | 7 +- .../PrivateProperties/EverythingAspect.php | 17 ++ .../PrivateProperties/Kernel.php | 11 ++ .../PrivatePropertiesTest.php | 161 ++++++++++++++++++ .../PrivateProperties/Target/ChildInput.php | 8 + .../PrivateProperties/Target/MagicInput.php | 11 ++ .../PrivateProperties/Target/ParentInput.php | 11 ++ .../Target/PromotedInput.php | 8 + .../Target/PublicGrandchild.php | 7 + .../PrivateProperties/Target/PublicInput.php | 9 + .../Target/SameTypeInput.php | 8 + .../PrivateProperties/Target/StaticChild.php | 8 + .../PrivateProperties/Target/StaticParent.php | 8 + .../PrivateProperties/Target/TokenTrait.php | 8 + .../PrivateProperties/Target/TraitInput.php | 7 + 21 files changed, 493 insertions(+), 7 deletions(-) create mode 100644 PROPERTY_ACCESS.md create mode 100644 docs/superpowers/plans/2026-09-05-private-properties.md create mode 100644 src/PropertyAccess.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/EverythingAspect.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Kernel.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/PrivatePropertiesTest.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/ChildInput.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/MagicInput.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/ParentInput.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/PromotedInput.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/PublicGrandchild.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/PublicInput.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/SameTypeInput.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/StaticChild.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/StaticParent.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/TokenTrait.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/TraitInput.php diff --git a/PROPERTY_ACCESS.md b/PROPERTY_ACCESS.md new file mode 100644 index 0000000..4f0ba54 --- /dev/null +++ b/PROPERTY_ACCESS.md @@ -0,0 +1,60 @@ +# Accessing private properties from advice + +Private properties keep their original visibility and declaring scope when a class +is woven. This fixes [#6](https://github.com/okapi-web/php-aop/issues/6): a parent and +child may legally declare private properties with the same name and different +types. They also retain independent values when their types are identical. + +## Migration + +Advice that previously read or wrote a private property directly through +`$invocation->getSubject()` must use `Okapi\Aop\PropertyAccess` instead. This applies +to all private properties, including properties whose names are currently unique. +Public and protected properties retain their existing behavior. Method interception +is unchanged. + +```php +use Okapi\Aop\PropertyAccess; + +$subject = $invocation->getSubject(); + +// Before: $subject->data = ['updated']; +PropertyAccess::set($subject, 'data', ['updated'], DatabaseService::class); +$data = PropertyAccess::get($subject, 'data', DatabaseService::class); +``` + +Use the original class that declares the property, without `__AopProxied`. For a +property supplied by a trait, use the class that uses the trait. The scope argument +can be omitted if the name identifies one property in the object's hierarchy. +Duplicate independent declarations require an explicit scope: + +```php +PropertyAccess::set($input, 'tokens', ['parent'], ArgvInput::class); +PropertyAccess::set($input, 'tokens', 'child', CompletionInput::class); +``` + +For static properties, pass an object or class name as the first argument: + +```php +$value = PropertyAccess::get(Service::class, 'configuration', Service::class); +PropertyAccess::set(Service::class, 'configuration', $value, Service::class); +``` + +The API uses PHP reflection, preserves declared types, and bypasses user-defined +`__get`/`__set` methods. It returns values, not references; to change an array, read +it, modify the local array, then write it back. A missing property or an invalid +declaring scope throws `ReflectionException`. An ambiguous name or a class-name +argument for an instance property throws `LogicException`. Uninitialized typed +properties still throw `Error` on read. + +## Why private declarations must remain private + +A parent can be loaded before its descendants are known. Making even a currently +unique private property public can invalidate a child loaded later. Checking only +known collisions would make behavior depend on load order and cached code. +Generated magic accessors are also unsafe as a general compatibility layer: their +signatures can conflict with a descendant's own magic methods. Explicit access +avoids changing either inheritance contract. + +Clear the configured AOP cache when upgrading so existing generated classes are +rebuilt. Deploy this change with the advice migration above. diff --git a/README.md b/README.md index 8d2d6da..f74f1e4 100644 --- a/README.md +++ b/README.md @@ -689,8 +689,11 @@ $firstLog = $logs[0]; - Intercept "private" and "protected" methods (Will show errors in IDEs) -- Access "private" and "protected" properties and methods of the subject - (Will show errors in IDEs) +- Access private properties through [`PropertyAccess`](PROPERTY_ACCESS.md), + with an explicit declaring class when names overlap in an inheritance hierarchy + +- Access protected properties and private/protected methods of the subject + (Direct access may show errors in IDEs) - Intercept "final" methods and classes diff --git a/docs/superpowers/plans/2026-09-05-private-properties.md b/docs/superpowers/plans/2026-09-05-private-properties.md new file mode 100644 index 0000000..b49d626 --- /dev/null +++ b/docs/superpowers/plans/2026-09-05-private-properties.md @@ -0,0 +1,34 @@ +# Private property inheritance implementation plan + +## Design + +Fix #6 without merging independent private property slots. A parent can be loaded +before any descendant is known, so private declarations must remain private even +when no collision is currently visible. An explicit PropertyAccess API selects an +original declaring class +for ambiguous names and supports static properties. Public/protected properties and +method interception retain their existing behavior. Generated constructors must not +declare promoted properties a second time. + +## Tasks + +- [x] Add failing functional tests for different/same types, parent-first loading, + promoted properties, property mutation, and explicit scoped access. +- [x] Preserve private property declarations in ProxiedClassModifier; remove + promotion from generated forwarding constructors in WovenClassBuilder. +- [x] Add PropertyAccess, reporting ambiguous names instead of selecting silently. + Independent review showed generated magic accessors could introduce child-method + signature fatals; the safe candidate omits them and documents migration of advice. +- [x] Cover traits, static properties, errors, and existing magic behavior. +- [x] Document access semantics and migration and obtain independent review. +- [x] Run Tests and Performance on PHP 8.1–8.5: 73 functional/integration tests + and 45 performance tests per version, no failures. Existing incomplete tests, + PHP 8.1 readonly-class skip, and dependency deprecations remain. +- [ ] Verify origin and upstream CI after the publishing decision. +- [ ] Merge the upstream PR referencing #6 only after successful checks. + +## Publishing decision + +User confirmation is pending for the documented compatibility change: advice must +use PropertyAccess instead of direct access to private properties. No automatic +magic accessor workaround is included because it can invalidate child classes. diff --git a/src/Core/Transform/ProxiedClassModifier.php b/src/Core/Transform/ProxiedClassModifier.php index 7ac4d01..affde9f 100644 --- a/src/Core/Transform/ProxiedClassModifier.php +++ b/src/Core/Transform/ProxiedClassModifier.php @@ -191,6 +191,24 @@ private function unReadOnlyClasses(): void */ private function changeVisibility(): void { + // A descendant may be loaded after this class. Keep every private property + // in its declaring scope, even when no same-name property is known yet. + foreach ($this->sourceFileNode->getDescendantNodes() as $node) { + $modifiers = match (true) { + $node instanceof Node\PropertyDeclaration => $node->modifiers ?? [], + $node instanceof Node\Parameter => array_filter([ + $node->visibilityToken, + ...($node->modifiers ?? []), + ]), + default => [], + }; + foreach ($modifiers as $modifier) { + if ($modifier->kind === TokenKind::PrivateKeyword) { + $this->alreadyProcessed[] = $modifier; + } + } + } + $this->tokenCallbacks[] = function (Token $token) { if ($token->kind === TokenKind::PrivateKeyword || $token->kind === TokenKind::ProtectedKeyword diff --git a/src/Core/Transform/WovenClassBuilder.php b/src/Core/Transform/WovenClassBuilder.php index ff5b714..31f0b0a 100644 --- a/src/Core/Transform/WovenClassBuilder.php +++ b/src/Core/Transform/WovenClassBuilder.php @@ -7,6 +7,7 @@ use Nette\PhpGenerator\ClassType; use Nette\PhpGenerator\Factory; use Nette\PhpGenerator\Method; +use Nette\PhpGenerator\Parameter; use Nette\PhpGenerator\PhpNamespace; use Nette\PhpGenerator\PromotedParameter; use Nette\PhpGenerator\Property; @@ -225,11 +226,23 @@ private function buildMethod(BetterReflectionMethod $refMethod): Method $methodName = $refMethod->getName(); - foreach ($method->getParameters() as $parameter) { + $parameters = $method->getParameters(); + foreach ($parameters as $name => $parameter) { if ($parameter instanceof PromotedParameter) { - $parameter->setReadOnly(false); + // Promotion belongs to the original constructor, which the + // interceptor invokes. A forwarding method must not own a second slot. + $plain = new Parameter($parameter->getName()); + $plain->setType($parameter->getType()); + $plain->setNullable($parameter->isNullable()); + $plain->setReference($parameter->isReference()); + $plain->setAttributes($parameter->getAttributes()); + if ($parameter->hasDefaultValue()) { + $plain->setDefaultValue($parameter->getDefaultValue()); + } + $parameters[$name] = $plain; } } + $method->setParameters($parameters); // Add "return" if the method has a return type $return = (string)$method->getReturnType() !== 'void' ? 'return ' : ''; diff --git a/src/PropertyAccess.php b/src/PropertyAccess.php new file mode 100644 index 0000000..211001b --- /dev/null +++ b/src/PropertyAccess.php @@ -0,0 +1,75 @@ +getValue(is_object($subject) ? $subject : null); + } + + /** + * Write a value using PHP's property type checks. + * + * @throws ReflectionException If the property or declaring scope does not exist. + * @throws LogicException If the name is ambiguous or an instance is required. + */ + public static function set(object|string $subject, string $name, mixed $value, ?string $declaringClass = null): void + { + $property = self::resolve($subject, $name, $declaringClass); + $property->setValue(is_object($subject) ? $subject : null, $value); + } + + private static function resolve(object|string $subject, string $name, ?string $declaringClass = null): ReflectionProperty + { + $matches = []; + $scope = $declaringClass === null ? null : ltrim($declaringClass, '\\'); + $class = new ReflectionClass($subject); + do { + $originalName = $class->getName(); + if (str_ends_with($originalName, CachePaths::PROXIED_SUFFIX)) { + $originalName = substr($originalName, 0, -strlen(CachePaths::PROXIED_SUFFIX)); + } + if ($scope !== null && strcasecmp($scope, $originalName) !== 0) { + continue; + } + foreach ($class->getProperties() as $property) { + if ($property->getName() !== $name || $property->getDeclaringClass()->getName() !== $class->getName()) { + continue; + } + // Non-private overrides share storage. Private declarations do not. + if (!$property->isPrivate() && isset($matches['inherited'])) { + continue; + } + $key = $property->isPrivate() ? $class->getName() : 'inherited'; + $matches[$key] = $property; + } + } while ($class = $class->getParentClass()); + + if (!$matches) { + throw new ReflectionException("Property \$$name does not exist in the requested scope."); + } + if (count($matches) > 1) { + throw new LogicException("Property \$$name is ambiguous; pass its original declaring class to PropertyAccess::get()/set()."); + } + $property = reset($matches); + if (is_string($subject) && !$property->isStatic()) { + throw new LogicException("An object is required to access instance property \$$name."); + } + return $property; + } +} diff --git a/tests/Functional/AdviceBehavior/Include/Aspect/DatabaseModifierAspect.php b/tests/Functional/AdviceBehavior/Include/Aspect/DatabaseModifierAspect.php index 2534cfd..81cea47 100644 --- a/tests/Functional/AdviceBehavior/Include/Aspect/DatabaseModifierAspect.php +++ b/tests/Functional/AdviceBehavior/Include/Aspect/DatabaseModifierAspect.php @@ -4,7 +4,8 @@ use Okapi\Aop\Attributes\After; use Okapi\Aop\Attributes\Aspect; -use Okapi\Aop\Invocation\AfterMethodInvocation; +use Okapi\Aop\Invocation\AfterMethodInvocation; +use Okapi\Aop\PropertyAccess; use Okapi\Aop\Tests\Functional\AdviceBehavior\Include\Target\SecureDatabaseService; #[Aspect] @@ -19,10 +20,10 @@ public function modifyData(AfterMethodInvocation $invocation): void /** @var SecureDatabaseService $subject */ $subject = $invocation->getSubject(); - $subject->data = [ + PropertyAccess::set($subject, 'data', [ 'd' => 4, 'e' => 5, 'f' => 6, - ]; + ], SecureDatabaseService::class); } } diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/EverythingAspect.php b/tests/Functional/AdviceBehavior/PrivateProperties/EverythingAspect.php new file mode 100644 index 0000000..07a72dc --- /dev/null +++ b/tests/Functional/AdviceBehavior/PrivateProperties/EverythingAspect.php @@ -0,0 +1,17 @@ +parentTokens()); + $child = new ChildInput(); + self::assertSame(['parent'], $child->parentTokens()); + self::assertSame('child', $child->childTokens()); + self::assertGreaterThanOrEqual(3, EverythingAspect::$calls); + } + + public function testSameTypesRetainIndependentValues(): void + { + $child = new SameTypeInput(); + self::assertSame(['parent'], $child->parentTokens()); + self::assertSame(['child'], $child->childTokens()); + } + + public function testPromotedPropertyIsNotDuplicatedByForwardingConstructor(): void + { + $child = new PromotedInput('initial'); + PropertyAccess::set($child, 'tokens', 'changed', PromotedInput::class); + self::assertSame('changed', $child->childTokens()); + self::assertSame(['parent'], $child->parentTokens()); + } + + public function testUnambiguousPropertyAccessDoesNotRequireScope(): void + { + $parent = new ParentInput(); + $values = PropertyAccess::get($parent, 'unique'); + $values[] = 'appended'; + PropertyAccess::set($parent, 'unique', $values); + self::assertSame(['unique', 'appended'], $parent->unique()); + PropertyAccess::set($parent, 'unique', ['assigned']); + self::assertSame(['assigned'], $parent->unique()); + } + + public function testExplicitScopeSelectsTheOriginalDeclaration(): void + { + $child = new ChildInput(); + PropertyAccess::set($child, 'tokens', ['updated parent'], ParentInput::class); + PropertyAccess::set($child, 'tokens', 'updated child', ChildInput::class); + self::assertSame(['updated parent'], $child->parentTokens()); + self::assertSame('updated child', $child->childTokens()); + self::assertSame(['updated parent'], PropertyAccess::get($child, 'tokens', ParentInput::class)); + } + + public function testAmbiguousAccessRequiresExplicitScope(): void + { + $child = new SameTypeInput(); + $this->expectException(\LogicException::class); + $this->expectExceptionMessage('ambiguous'); + PropertyAccess::get($child, 'tokens'); + } + + public function testTraitPropertyRemainsSeparate(): void + { + $child = new TraitInput(); + self::assertSame(['parent'], $child->parentTokens()); + self::assertSame('trait', $child->childTokens()); + PropertyAccess::set($child, 'tokens', 'updated trait', TraitInput::class); + self::assertSame('updated trait', $child->childTokens()); + } + + public function testCustomMagicAccessorsKeepVirtualPropertyBehavior(): void + { + $input = new MagicInput(); + self::assertSame('virtual', $input->example); + $input->example = 'assigned'; + self::assertSame('assigned', $input->example); + self::assertTrue(isset($input->example)); + self::assertSame(['example' => 'assigned'], PropertyAccess::get($input, 'values', MagicInput::class)); + unset($input->example); + self::assertFalse(isset($input->example)); + } + + public function testStaticPrivatePropertiesHaveIndependentStorage(): void + { + self::assertSame(['parent'], StaticChild::parentTokens()); + self::assertSame('child', StaticChild::childTokens()); + PropertyAccess::set(StaticChild::class, 'tokens', ['updated'], StaticParent::class); + PropertyAccess::set(StaticChild::class, 'tokens', 'updated child', StaticChild::class); + self::assertSame(['updated'], StaticChild::parentTokens()); + self::assertSame('updated child', PropertyAccess::get(StaticChild::class, 'tokens', StaticChild::class)); + } + + public function testUnknownScopeDoesNotFallBackToAnotherDeclaration(): void + { + $this->expectException(\ReflectionException::class); + PropertyAccess::get(new ParentInput(), 'tokens', \stdClass::class); + } + + public function testInstanceAccessRequiresAnObject(): void + { + $this->expectException(\LogicException::class); + PropertyAccess::get(ParentInput::class, 'tokens'); + } + + public function testScopedWritesEnforceDeclaredType(): void + { + $this->expectException(\TypeError::class); + PropertyAccess::set(new ParentInput(), 'tokens', 'not an array', ParentInput::class); + } + + public function testPrivateParentAndPublicChildStayIndependent(): void + { + $child = new PublicInput(); + $child->tokens = 'direct write'; + self::assertSame(['parent'], $child->parentTokens()); + self::assertSame('direct write', $child->childTokens()); + self::assertSame('direct write', PropertyAccess::get($child, 'tokens', PublicInput::class)); + } + + public function testNonPrivateOverridesShareStorage(): void + { + $child = new PublicGrandchild(); + PropertyAccess::set($child, 'tokens', 'shared', PublicInput::class); + self::assertSame('shared', $child->tokens); + self::assertSame('shared', PropertyAccess::get($child, 'tokens', PublicGrandchild::class)); + self::assertSame(['parent'], $child->parentTokens()); + } + + public function testUninitializedNullableReadThrowsWithoutInitializingProperty(): void + { + $child = new PublicInput(); + $property = new \ReflectionProperty(PublicInput::class . '__AopProxied', 'uninitialized'); + self::assertFalse($property->isInitialized($child)); + try { + PropertyAccess::get($child, 'uninitialized', PublicInput::class); + self::fail('Expected an uninitialized property error.'); + } catch (\Error $error) { + self::assertStringContainsString('must not be accessed before initialization', $error->getMessage()); + } + self::assertFalse($property->isInitialized($child)); + } +} diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/Target/ChildInput.php b/tests/Functional/AdviceBehavior/PrivateProperties/Target/ChildInput.php new file mode 100644 index 0000000..606a56e --- /dev/null +++ b/tests/Functional/AdviceBehavior/PrivateProperties/Target/ChildInput.php @@ -0,0 +1,8 @@ +tokens; } +} diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/Target/MagicInput.php b/tests/Functional/AdviceBehavior/PrivateProperties/Target/MagicInput.php new file mode 100644 index 0000000..7929e1c --- /dev/null +++ b/tests/Functional/AdviceBehavior/PrivateProperties/Target/MagicInput.php @@ -0,0 +1,11 @@ +values[$name] ?? 'virtual'; } + public function __set(string $name, mixed $value): void { $this->values[$name] = $value; } + public function __isset(string $name): bool { return isset($this->values[$name]); } + public function __unset(string $name): void { unset($this->values[$name]); } +} diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/Target/ParentInput.php b/tests/Functional/AdviceBehavior/PrivateProperties/Target/ParentInput.php new file mode 100644 index 0000000..4081e05 --- /dev/null +++ b/tests/Functional/AdviceBehavior/PrivateProperties/Target/ParentInput.php @@ -0,0 +1,11 @@ +tokens; } + public function unique(): array { return $this->unique; } +} diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/Target/PromotedInput.php b/tests/Functional/AdviceBehavior/PrivateProperties/Target/PromotedInput.php new file mode 100644 index 0000000..a1c7e7e --- /dev/null +++ b/tests/Functional/AdviceBehavior/PrivateProperties/Target/PromotedInput.php @@ -0,0 +1,8 @@ +tokens; } +} diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/Target/PublicGrandchild.php b/tests/Functional/AdviceBehavior/PrivateProperties/Target/PublicGrandchild.php new file mode 100644 index 0000000..1bce681 --- /dev/null +++ b/tests/Functional/AdviceBehavior/PrivateProperties/Target/PublicGrandchild.php @@ -0,0 +1,7 @@ +tokens; } +} diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/Target/SameTypeInput.php b/tests/Functional/AdviceBehavior/PrivateProperties/Target/SameTypeInput.php new file mode 100644 index 0000000..4191e49 --- /dev/null +++ b/tests/Functional/AdviceBehavior/PrivateProperties/Target/SameTypeInput.php @@ -0,0 +1,8 @@ +tokens; } +} diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/Target/StaticChild.php b/tests/Functional/AdviceBehavior/PrivateProperties/Target/StaticChild.php new file mode 100644 index 0000000..92745a5 --- /dev/null +++ b/tests/Functional/AdviceBehavior/PrivateProperties/Target/StaticChild.php @@ -0,0 +1,8 @@ +tokens; } +} diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/Target/TraitInput.php b/tests/Functional/AdviceBehavior/PrivateProperties/Target/TraitInput.php new file mode 100644 index 0000000..2b3d550 --- /dev/null +++ b/tests/Functional/AdviceBehavior/PrivateProperties/Target/TraitInput.php @@ -0,0 +1,7 @@ + Date: Sat, 5 Sep 2026 18:10:28 +0200 Subject: [PATCH 2/3] Expose scoped subject properties through advice invocation accessors --- PROPERTY_ACCESS.md | 47 +++++++++++----- README.md | 2 +- .../plans/2026-09-05-private-properties.md | 7 ++- src/Invocation/MethodInvocation.php | 6 ++ src/Invocation/PropertyAccessor.php | 34 ++++++++++++ src/PropertyAccess.php | 46 ++++++++++++++++ .../Include/Aspect/DatabaseModifierAspect.php | 8 +-- .../PrivateProperties/EverythingAspect.php | 5 +- .../PrivatePropertiesTest.php | 55 +++++++++++++++++++ 9 files changed, 185 insertions(+), 25 deletions(-) create mode 100644 src/Invocation/PropertyAccessor.php diff --git a/PROPERTY_ACCESS.md b/PROPERTY_ACCESS.md index 4f0ba54..a356f9c 100644 --- a/PROPERTY_ACCESS.md +++ b/PROPERTY_ACCESS.md @@ -8,19 +8,15 @@ types. They also retain independent values when their types are identical. ## Migration Advice that previously read or wrote a private property directly through -`$invocation->getSubject()` must use `Okapi\Aop\PropertyAccess` instead. This applies +`$invocation->getSubject()` should use `$invocation->properties()` instead. This applies to all private properties, including properties whose names are currently unique. Public and protected properties retain their existing behavior. Method interception is unchanged. ```php -use Okapi\Aop\PropertyAccess; - -$subject = $invocation->getSubject(); - // Before: $subject->data = ['updated']; -PropertyAccess::set($subject, 'data', ['updated'], DatabaseService::class); -$data = PropertyAccess::get($subject, 'data', DatabaseService::class); +$invocation->properties()->data = ['updated']; +$data = $invocation->properties()->data; ``` Use the original class that declares the property, without `__AopProxied`. For a @@ -29,23 +25,46 @@ can be omitted if the name identifies one property in the object's hierarchy. Duplicate independent declarations require an explicit scope: ```php -PropertyAccess::set($input, 'tokens', ['parent'], ArgvInput::class); -PropertyAccess::set($input, 'tokens', 'child', CompletionInput::class); +$invocation->properties(ArgvInput::class)->tokens = ['parent']; +$invocation->properties(CompletionInput::class)->tokens = 'child'; +``` + +The accessor supports array mutation, references, `isset`, and `unset`: + +```php +$properties = $invocation->properties(); +$properties->data[] = 'appended'; +$reference =& $properties->data; +isset($properties->data); +unset($properties->data); ``` -For static properties, pass an object or class name as the first argument: +`properties()` is a view of the existing subject, not a replacement for it. +`getSubject()` still returns the same object. Subject type identity, internal method +calls, and method interception are unchanged. In static advice, the accessor uses +the invocation's class; instance properties require an object. + +The lower-level `PropertyAccess` API is also available outside an invocation. For +static properties, pass an object or class name as the first argument: ```php +use Okapi\Aop\PropertyAccess; + $value = PropertyAccess::get(Service::class, 'configuration', Service::class); PropertyAccess::set(Service::class, 'configuration', $value, Service::class); ``` -The API uses PHP reflection, preserves declared types, and bypasses user-defined -`__get`/`__set` methods. It returns values, not references; to change an array, read -it, modify the local array, then write it back. A missing property or an invalid +Property access uses PHP reflection and closures bound to the declaring scope, +preserves declared types, and bypasses user-defined `__get`/`__set` methods. +The lower-level `get()` returns a value; the invocation accessor supports references. +A missing property or an invalid declaring scope throws `ReflectionException`. An ambiguous name or a class-name argument for an instance property throws `LogicException`. Uninitialized typed -properties still throw `Error` on read. +properties still throw `Error` on read without initializing them. `isset` returns +false and `unset` does nothing for absent properties; ambiguous names still throw. +Static properties cannot be unset. Writes must name a declared property; the +accessor does not create dynamic properties. PHP also prevents taking references +to readonly properties on subjects whose readonly declarations remain intact. ## Why private declarations must remain private diff --git a/README.md b/README.md index f74f1e4..c8196c6 100644 --- a/README.md +++ b/README.md @@ -689,7 +689,7 @@ $firstLog = $logs[0]; - Intercept "private" and "protected" methods (Will show errors in IDEs) -- Access private properties through [`PropertyAccess`](PROPERTY_ACCESS.md), +- Access private properties through [`$invocation->properties()`](PROPERTY_ACCESS.md), with an explicit declaring class when names overlap in an inheritance hierarchy - Access protected properties and private/protected methods of the subject diff --git a/docs/superpowers/plans/2026-09-05-private-properties.md b/docs/superpowers/plans/2026-09-05-private-properties.md index b49d626..f481ae0 100644 --- a/docs/superpowers/plans/2026-09-05-private-properties.md +++ b/docs/superpowers/plans/2026-09-05-private-properties.md @@ -29,6 +29,7 @@ declare promoted properties a second time. ## Publishing decision -User confirmation is pending for the documented compatibility change: advice must -use PropertyAccess instead of direct access to private properties. No automatic -magic accessor workaround is included because it can invalidate child classes. +The user approved migration to `$invocation->properties($declaringClass)`. +The accessor wraps property access only; it never replaces the subject or adds +magic methods to the subject's inheritance hierarchy. Reads/writes, array mutation, +references, isset/unset, and static invocation access have functional coverage. diff --git a/src/Invocation/MethodInvocation.php b/src/Invocation/MethodInvocation.php index cf1adca..b2779d5 100644 --- a/src/Invocation/MethodInvocation.php +++ b/src/Invocation/MethodInvocation.php @@ -115,6 +115,12 @@ public function getSubject(): ?object return $this->subject; } + /** Access the subject's properties, optionally in an original declaring scope. */ + public function properties(?string $declaringClass = null): PropertyAccessor + { + return new PropertyAccessor($this->subject ?? $this->className, $declaringClass); + } + /** * Get the original subject class name of the invocation. * diff --git a/src/Invocation/PropertyAccessor.php b/src/Invocation/PropertyAccessor.php new file mode 100644 index 0000000..ab39185 --- /dev/null +++ b/src/Invocation/PropertyAccessor.php @@ -0,0 +1,34 @@ +subject, $name, $this->declaringClass); + return $value; + } + + public function __set(string $name, mixed $value): void + { + PropertyAccess::set($this->subject, $name, $value, $this->declaringClass); + } + + public function __isset(string $name): bool + { + return PropertyAccess::isSet($this->subject, $name, $this->declaringClass); + } + + public function __unset(string $name): void + { + PropertyAccess::remove($this->subject, $name, $this->declaringClass); + } +} diff --git a/src/PropertyAccess.php b/src/PropertyAccess.php index 211001b..14e5c63 100644 --- a/src/PropertyAccess.php +++ b/src/PropertyAccess.php @@ -2,6 +2,8 @@ namespace Okapi\Aop; use LogicException; +use Closure; +use Error; use Okapi\Aop\Core\Cache\CachePaths; use ReflectionClass; use ReflectionException; @@ -34,6 +36,50 @@ public static function set(object|string $subject, string $name, mixed $value, ? $property->setValue(is_object($subject) ? $subject : null, $value); } + /** @internal Support indirect writes through an invocation's property accessor. */ + public static function &reference(object|string $subject, string $name, ?string $declaringClass = null): mixed + { + $property = self::resolve($subject, $name, $declaringClass); + // Acquiring a reference to an uninitialized nullable property initializes + // it to null. Reading through the accessor must not introduce that side effect. + $property->getValue(is_object($subject) ? $subject : null); + $scope = $property->getDeclaringClass()->getName(); + $read = $property->isStatic() + ? Closure::bind(static function &() use ($name) { return self::$$name; }, null, $scope) + : Closure::bind(function &() use ($name) { return $this->$name; }, $subject, $scope); + $value =& $read(); + return $value; + } + + /** @internal */ + public static function isSet(object|string $subject, string $name, ?string $declaringClass = null): bool + { + try { + $property = self::resolve($subject, $name, $declaringClass); + } catch (ReflectionException) { + return false; + } + $object = is_object($subject) ? $subject : null; + return $property->isInitialized($object) && $property->getValue($object) !== null; + } + + /** @internal */ + public static function remove(object|string $subject, string $name, ?string $declaringClass = null): void + { + try { + $property = self::resolve($subject, $name, $declaringClass); + } catch (ReflectionException) { + return; + } + if ($property->isStatic()) { + throw new Error("Cannot unset static property \$$name."); + } + $remove = Closure::bind(function () use ($name): void { + unset($this->$name); + }, $subject, $property->getDeclaringClass()->getName()); + $remove(); + } + private static function resolve(object|string $subject, string $name, ?string $declaringClass = null): ReflectionProperty { $matches = []; diff --git a/tests/Functional/AdviceBehavior/Include/Aspect/DatabaseModifierAspect.php b/tests/Functional/AdviceBehavior/Include/Aspect/DatabaseModifierAspect.php index 81cea47..68535ca 100644 --- a/tests/Functional/AdviceBehavior/Include/Aspect/DatabaseModifierAspect.php +++ b/tests/Functional/AdviceBehavior/Include/Aspect/DatabaseModifierAspect.php @@ -5,7 +5,6 @@ use Okapi\Aop\Attributes\After; use Okapi\Aop\Attributes\Aspect; use Okapi\Aop\Invocation\AfterMethodInvocation; -use Okapi\Aop\PropertyAccess; use Okapi\Aop\Tests\Functional\AdviceBehavior\Include\Target\SecureDatabaseService; #[Aspect] @@ -17,13 +16,10 @@ class: SecureDatabaseService::class, )] public function modifyData(AfterMethodInvocation $invocation): void { - /** @var SecureDatabaseService $subject */ - $subject = $invocation->getSubject(); - - PropertyAccess::set($subject, 'data', [ + $invocation->properties()->data = [ 'd' => 4, 'e' => 5, 'f' => 6, - ], SecureDatabaseService::class); + ]; } } diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/EverythingAspect.php b/tests/Functional/AdviceBehavior/PrivateProperties/EverythingAspect.php index 07a72dc..104d1c5 100644 --- a/tests/Functional/AdviceBehavior/PrivateProperties/EverythingAspect.php +++ b/tests/Functional/AdviceBehavior/PrivateProperties/EverythingAspect.php @@ -3,15 +3,18 @@ use Okapi\Aop\Attributes\Aspect; use Okapi\Aop\Attributes\Before; +use Okapi\Aop\Invocation\BeforeMethodInvocation; #[Aspect] class EverythingAspect { public static int $calls = 0; + public static BeforeMethodInvocation $invocation; #[Before(class: 'Okapi\Aop\Tests\Functional\AdviceBehavior\PrivateProperties\Target\*', method: '*')] - public function before(): void + public function before(BeforeMethodInvocation $invocation): void { self::$calls++; + self::$invocation = $invocation; } } diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/PrivatePropertiesTest.php b/tests/Functional/AdviceBehavior/PrivateProperties/PrivatePropertiesTest.php index 4583802..21f265d 100644 --- a/tests/Functional/AdviceBehavior/PrivateProperties/PrivatePropertiesTest.php +++ b/tests/Functional/AdviceBehavior/PrivateProperties/PrivatePropertiesTest.php @@ -158,4 +158,59 @@ public function testUninitializedNullableReadThrowsWithoutInitializingProperty() } self::assertFalse($property->isInitialized($child)); } + + public function testInvocationAccessorMutatesActualSubjectAndSupportsReferences(): void + { + $subject = new ParentInput(); + $subject->unique(); + $invocation = EverythingAspect::$invocation; + self::assertSame($subject, $invocation->getSubject()); + $properties = $invocation->properties(); + $properties->unique[] = 'appended'; + $reference =& $properties->unique; + $reference[] = 'reference'; + self::assertSame(['unique', 'appended', 'reference'], $subject->unique()); + self::assertTrue(isset($properties->unique)); + $properties->unique = ['assigned']; + self::assertSame(['assigned'], $subject->unique()); + unset($properties->unique); + self::assertFalse(isset($properties->unique)); + self::assertFalse(isset($properties->missing)); + } + + public function testInvocationAccessorSelectsParentAndChildScope(): void + { + $subject = new ChildInput(); + $subject->childTokens(); + $invocation = EverythingAspect::$invocation; + $invocation->properties(ParentInput::class)->tokens = ['changed parent']; + $invocation->properties(ChildInput::class)->tokens = 'changed child'; + self::assertSame(['changed parent'], $subject->parentTokens()); + self::assertSame('changed child', $subject->childTokens()); + $this->expectException(\LogicException::class); + $invocation->properties()->tokens; + } + + public function testStaticInvocationAccessor(): void + { + StaticChild::childTokens(); + $invocation = EverythingAspect::$invocation; + self::assertNull($invocation->getSubject()); + $properties = $invocation->properties(StaticChild::class); + $properties->tokens = 'assigned'; + self::assertSame('assigned', $properties->tokens); + self::assertSame('assigned', StaticChild::childTokens()); + $this->expectException(\Error::class); + unset($properties->tokens); + } + + public function testAccessorDoesNotInitializeNullablePropertyOnRead(): void + { + $subject = new PublicInput(); + $subject->childTokens(); + $properties = EverythingAspect::$invocation->properties(PublicInput::class); + self::assertFalse(isset($properties->uninitialized)); + $this->expectException(\Error::class); + $properties->uninitialized; + } } From 74b5024937a1fa49e92f3f3feca110ac4e2091cf Mon Sep 17 00:00:00 2001 From: WalterWoshid Date: Sat, 5 Sep 2026 18:12:52 +0200 Subject: [PATCH 3/3] Handle uninitialized and redeclared static properties explicitly --- PROPERTY_ACCESS.md | 5 ++- src/PropertyAccess.php | 23 +++++++--- .../PrivatePropertiesTest.php | 43 +++++++++++++++++++ .../Target/SharedStaticChild.php | 7 +++ .../Target/SharedStaticParent.php | 8 ++++ 5 files changed, 79 insertions(+), 7 deletions(-) create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/SharedStaticChild.php create mode 100644 tests/Functional/AdviceBehavior/PrivateProperties/Target/SharedStaticParent.php diff --git a/PROPERTY_ACCESS.md b/PROPERTY_ACCESS.md index a356f9c..cb95f98 100644 --- a/PROPERTY_ACCESS.md +++ b/PROPERTY_ACCESS.md @@ -55,7 +55,10 @@ PropertyAccess::set(Service::class, 'configuration', $value, Service::class); ``` Property access uses PHP reflection and closures bound to the declaring scope, -preserves declared types, and bypasses user-defined `__get`/`__set` methods. +preserves declared types, and accesses declared storage directly for initialized +properties. After explicitly unsetting a property, PHP may invoke the subject's +`__set` when writing it again; the accessor preserves that native behavior, which +may leave the declared storage uninitialized if the setter does not restore it. The lower-level `get()` returns a value; the invocation accessor supports references. A missing property or an invalid declaring scope throws `ReflectionException`. An ambiguous name or a class-name diff --git a/src/PropertyAccess.php b/src/PropertyAccess.php index 14e5c63..9f0572e 100644 --- a/src/PropertyAccess.php +++ b/src/PropertyAccess.php @@ -21,6 +21,10 @@ final class PropertyAccess public static function get(object|string $subject, string $name, ?string $declaringClass = null): mixed { $property = self::resolve($subject, $name, $declaringClass); + if (!$property->isInitialized(is_object($subject) ? $subject : null)) { + throw new Error('Property ' . $property->getDeclaringClass()->getName() + . '::$' . $name . ' must not be accessed before initialization'); + } return $property->getValue(is_object($subject) ? $subject : null); } @@ -40,9 +44,11 @@ public static function set(object|string $subject, string $name, mixed $value, ? public static function &reference(object|string $subject, string $name, ?string $declaringClass = null): mixed { $property = self::resolve($subject, $name, $declaringClass); - // Acquiring a reference to an uninitialized nullable property initializes - // it to null. Reading through the accessor must not introduce that side effect. - $property->getValue(is_object($subject) ? $subject : null); + // Do not initialize nullable properties or invoke __get after explicit unset. + if (!$property->isInitialized(is_object($subject) ? $subject : null)) { + throw new Error('Typed property ' . $property->getDeclaringClass()->getName() + . '::$' . $name . ' must not be accessed before initialization'); + } $scope = $property->getDeclaringClass()->getName(); $read = $property->isStatic() ? Closure::bind(static function &() use ($name) { return self::$$name; }, null, $scope) @@ -74,6 +80,9 @@ public static function remove(object|string $subject, string $name, ?string $dec if ($property->isStatic()) { throw new Error("Cannot unset static property \$$name."); } + if (!$property->isInitialized($subject)) { + return; + } $remove = Closure::bind(function () use ($name): void { unset($this->$name); }, $subject, $property->getDeclaringClass()->getName()); @@ -97,11 +106,13 @@ private static function resolve(object|string $subject, string $name, ?string $d if ($property->getName() !== $name || $property->getDeclaringClass()->getName() !== $class->getName()) { continue; } - // Non-private overrides share storage. Private declarations do not. - if (!$property->isPrivate() && isset($matches['inherited'])) { + // Non-private instance overrides share storage. Redeclared static + // properties and private declarations each have independent slots. + $independent = $property->isPrivate() || $property->isStatic(); + if (!$independent && isset($matches['inherited'])) { continue; } - $key = $property->isPrivate() ? $class->getName() : 'inherited'; + $key = $independent ? $class->getName() : 'inherited'; $matches[$key] = $property; } } while ($class = $class->getParentClass()); diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/PrivatePropertiesTest.php b/tests/Functional/AdviceBehavior/PrivateProperties/PrivatePropertiesTest.php index 21f265d..206cb59 100644 --- a/tests/Functional/AdviceBehavior/PrivateProperties/PrivatePropertiesTest.php +++ b/tests/Functional/AdviceBehavior/PrivateProperties/PrivatePropertiesTest.php @@ -213,4 +213,47 @@ public function testAccessorDoesNotInitializeNullablePropertyOnRead(): void $this->expectException(\Error::class); $properties->uninitialized; } + + public function testUnsetPropertyReadDoesNotInvokeSubjectMagicGetter(): void + { + $subject = new class { + private string $value = 'initial'; + public int $calls = 0; + public function __get(string $name): mixed { $this->calls++; return 'magic'; } + }; + $properties = new \Okapi\Aop\Invocation\PropertyAccessor($subject); + unset($properties->value); + try { + $properties->value; + self::fail('Expected uninitialized property error.'); + } catch (\Error) { + self::assertSame(0, $subject->calls); + } + } + + public function testRedeclaredPublicStaticPropertiesRequireScope(): void + { + $parent = Target\SharedStaticParent::class; + $child = Target\SharedStaticChild::class; + $properties = new \Okapi\Aop\Invocation\PropertyAccessor($child, $parent); + $properties->value = 3; + self::assertSame(3, $parent::$value); + self::assertSame(2, $child::$value); + $this->expectException(\LogicException::class); + PropertyAccess::get($child, 'value'); + } + + public function testWriteAfterUnsetPreservesNativeMagicSetterBehavior(): void + { + $subject = new class { + private string $value = 'initial'; + public int $calls = 0; + public function __set(string $name, mixed $value): void { $this->calls++; } + }; + $properties = new \Okapi\Aop\Invocation\PropertyAccessor($subject); + unset($properties->value); + $properties->value = 'new'; + self::assertSame(1, $subject->calls); + self::assertFalse(isset($properties->value)); + } } diff --git a/tests/Functional/AdviceBehavior/PrivateProperties/Target/SharedStaticChild.php b/tests/Functional/AdviceBehavior/PrivateProperties/Target/SharedStaticChild.php new file mode 100644 index 0000000..454564e --- /dev/null +++ b/tests/Functional/AdviceBehavior/PrivateProperties/Target/SharedStaticChild.php @@ -0,0 +1,7 @@ +