Cache reflection lookups in ExtendableTrait magic method handling

[austinderrick]

Problem

Every __get/__set/__call on a class using ExtendableTrait (i.e. every attribute access on every Database model) currently:

• builds a fresh ReflectionClass and recursively walks the parent chain in extensionGetParentClass() to find the first non-extendable ancestor,
• resolves the parent's __get/__set/__call ReflectionMethod twice per call — once in extensionMethodExists() and again in extensionCallMethod(),
• and, for classes with behaviors, constructs another ReflectionClass per extension per property read in extendableIsAccessible().

None of this was memoized, although every one of these resolutions depends only on the class, never the instance. A microbenchmark of the __get path shows roughly a 10x overhead per attribute access (~30ns direct vs ~340ns through the reflection path) — and a typical request makes thousands of magic-method calls through models, components and widgets, plus GC pressure from the throwaway reflection objects.

Fix

Three per-class static caches, following the same pattern as the existing extendableCallStatic() cache in the same file:

• extensionGetParentClass() caches the resolved parent reflection (or false) per class; the recursion's intermediate steps are split into extensionResolveParentClass() and are not cached.
• extensionMethodExists() / extensionCallMethod() share a cached ReflectionMethod handle per class and method. extensionCallMethod() keeps a fallback to an uncached getMethod() for non-public methods so its behaviour for direct, unguarded calls is unchanged (including the ReflectionException for unknown methods).
• extendableIsAccessible() caches the visibility check per class and property.

All cached values are class-structure facts that cannot change at runtime, so there is no invalidation concern; dynamic methods and properties continue to live in the per-instance $extensionData and are checked before any of these paths.

Testing

New tests assert the parent reflection is reused across calls and instances (and that subclasses memoize independently), and that repeated magic access behaves consistently across instances. The full test suite passes — the Database model tests exercise the cached __get/__set/__call paths heavily.

Summary by CodeRabbit

• Refactor

  • Improved application performance through internal optimization.
  • Enhanced reliability and consistency of magic method and property access behavior.

• Tests

  • Added comprehensive tests for magic method and property access patterns to ensure consistent behavior across repeated operations.

[coderabbitai]

Walkthrough

This pull request enhances ExtendableTrait with static caching for frequently-called reflection operations. It introduces three protected static cache properties, then refactors property accessibility checks, parent class resolution, and method reflection lookups to store and reuse computed results. The implementation routes method reflection through a new helper that caches public methods while preserving backward-compatible fallback for non-public invocation. Two new tests verify that reflection objects are memoized across repeated calls and that magic property and method access behave consistently.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately captures the primary change: adding caching for reflection lookups in ExtendableTrait's magic method handling. It is concise, specific, and clearly relates to the main objective of the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/Extension/ExtendableTrait.php (2)

596-596: ⚡ Quick win

Strengthen type hint to ReflectionClass.

The method calls $reflector->getParentClass() which requires a ReflectionClass instance. Using object allows any object to be passed, which would cause a runtime error.

-    protected function extensionResolveParentClass(object $reflector)
+    protected function extensionResolveParentClass(ReflectionClass $reflector)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Extension/ExtendableTrait.php` at line 596, The method
extensionResolveParentClass currently types its parameter as object but calls
$reflector->getParentClass(), so strengthen the type hint to ReflectionClass
(use the ReflectionClass type for the $reflector parameter in
extensionResolveParentClass), and add/import the ReflectionClass symbol (or
fully qualify it) to ensure static analysis and runtime correctness when
invoking getParentClass().

---

573-576: Remove dead $instance branch in extensionGetParentClass

src/Extension/ExtendableTrait.php always calls extensionGetParentClass() with no arguments (call sites at lines 393, 420, 464), so the if (!is_null($instance)) { return $this->extensionResolveParentClass($instance); } block at lines 573-576 is unreachable. Consider removing the $instance parameter and that dead branch for clarity.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Extension/ExtendableTrait.php` around lines 573 - 576, The branch
handling a non-null $instance in extensionGetParentClass is dead because callers
always invoke extensionGetParentClass() with no arguments; remove the unused
$instance parameter and the if (!is_null($instance)) { return
$this->extensionResolveParentClass($instance); } block from
ExtendableTrait::extensionGetParentClass, update the method signature and its
docblock to drop $instance, and adjust any callers and related tests/docblocks
to match the new zero-arg signature while keeping extensionResolveParentClass
intact for actual resolution logic.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/Extension/ExtendableTraitTest.php`:
- Around line 66-71: The test currently only asserts that repeated calls on
Level3NonExtendable reuse the same ReflectionClass instance; update the test to
also assert that Level3NonExtendable's cached ReflectionClass is a distinct
instance from the one cached by Level2NonExtendable: call the protected method
extensionGetParentClass on a Level2NonExtendable instance to get $first (the
parent-resolving ReflectionClass), then add an identity assertion that
$level3First !== $first to guarantee per-caller-class memoization (reference the
methods/variables extensionGetParentClass, Level2NonExtendable,
Level3NonExtendable, $first, $level3First).

---

Nitpick comments:
In `@src/Extension/ExtendableTrait.php`:
- Line 596: The method extensionResolveParentClass currently types its parameter
as object but calls $reflector->getParentClass(), so strengthen the type hint to
ReflectionClass (use the ReflectionClass type for the $reflector parameter in
extensionResolveParentClass), and add/import the ReflectionClass symbol (or
fully qualify it) to ensure static analysis and runtime correctness when
invoking getParentClass().
- Around line 573-576: The branch handling a non-null $instance in
extensionGetParentClass is dead because callers always invoke
extensionGetParentClass() with no arguments; remove the unused $instance
parameter and the if (!is_null($instance)) { return
$this->extensionResolveParentClass($instance); } block from
ExtendableTrait::extensionGetParentClass, update the method signature and its
docblock to drop $instance, and adjust any callers and related tests/docblocks
to match the new zero-arg signature while keeping extensionResolveParentClass
intact for actual resolution logic.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 59305219-3747-4f90-95ad-50526f2edfd4

📥 Commits

Reviewing files that changed from the base of the PR and between 989e5b4 and bf25e93.

📒 Files selected for processing (2)

• src/Extension/ExtendableTrait.php
• tests/Extension/ExtendableTraitTest.php