feat: document bound interceptors in weaved method comments - #262
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughGenerated/weaved methods now include an always-on comment listing bound interceptor short class names. Code generation derives these names, increments its generation version, and tests verify the generated output. ChangesInterceptor documentation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.2.2)PHPStan was skipped because the config uses disallowed 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 2.x #262 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 234 239 +5
===========================================
Files 29 29
Lines 601 610 +9
===========================================
+ Hits 601 610 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Always emit a short-class-name // comment above ReflectiveMethodInvocation construction so generated proxies are self-documenting. No runtime cost. Bump AopCode::GENERATION to 6.
ca60c70 to
f19c1c3
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 262 --repo ray-di/Ray.Aop --json title,body,files,additions,deletions
echo "-----DIFF-----"
gh pr diff 262 --repo ray-di/Ray.AopLength of output: 7962 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
rm -rf Ray.Aop
git clone --depth 50 https://github.com/ray-di/Ray.Aop.git 2>/dev/null
cd Ray.Aop
git fetch origin pull/262/head:pr262 2>/dev/null
git checkout pr262 2>/dev/null
echo "----BindInterface----"
cat src/BindInterface.php 2>/dev/null || fd BindInterface
echo "----Bind.php getBindings----"
rg -n "getBindings" -A 15 src/Bind.php
echo "----AopCode full addMethods context----"
sed -n '1,60p' src/AopCode.php
echo "---"
sed -n '190,270p' src/AopCode.phpLength of output: 6966 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp/Ray.Aop
echo "----Types.php----"
cat src/Types.php 2>/dev/null || fd Types.php
echo "----Bind.php bindInterceptors----"
rg -n "bindInterceptors" -B2 -A 20 src/Bind.php
echo "----Bind.php full bindings property----"
rg -n "bindings" -B2 -A5 src/Bind.php | head -80Length of output: 4686 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp/Ray.Aop
sed -n '1,75p' src/Bind.php
echo "---rest---"
sed -n '105,220p' src/Bind.phpLength of output: 2678 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp/Ray.Aop
fd MethodMatch
cat src/MethodMatch.php 2>/dev/null
echo "----Pointcut.php----"
cat src/Pointcut.php 2>/dev/null
echo "----MethodInterceptor----"
fd MethodInterceptor
cat src/MethodInterceptor.php 2>/dev/nullLength of output: 5481 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp/Ray.Aop
rg -n "new Pointcut\(" -B3 -A3 src/
echo "----check any place instantiate class-string interceptor----"
rg -n "class-string<MethodInterceptor>|PointcutInterceptors" -B2 -A2 src/Length of output: 531 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp/Ray.Aop
rg -n "Pointcut(" src/ -l
rg -n "new Pointcut" src/ -A3 -B3
rg -rn "PointcutInterceptors" src/
rg -n "class-string" src/Types.php src/Pointcut.phpLength of output: 569 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp/Ray.Aop
rg -n 'new Pointcut\(' src/ -A3 -B3
echo "---"
rg -n 'PointcutInterceptors' src/
echo "---"
rg -n 'class-string' src/Pointcut.php src/Types.phpLength of output: 497 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp/Ray.Aop
grep -rn "PointcutInterceptors" . --include="*.php" | head -30
echo "---new Pointcut---"
grep -rn "new Pointcut" . --include="*.php" | head -30Length of output: 4397 🤖 web_search executed:
Length of output: 10930 🏁 Script executed: #!/bin/bash
which php && php -v || echo "no php"Length of output: 233 🏁 Script executed: #!/bin/bash
command -v php8.4 php8.3 php8.2 php8.1 php8 php 2>/dev/null
ls /usr/bin | grep -i php || true
apt list --installed 2>/dev/null | grep -i php || trueLength of output: 353 🏁 Script executed: #!/bin/bash
cat > /tmp/test_class.php << 'EOF'
<?php
$s = 'FooBar\Baz';
try {
$r = $s::class;
var_dump($r);
} catch (\Throwable $e) {
echo "ERROR: " . $e->getMessage() . "\n";
}
EOF
php8.3 /tmp/test_class.phpLength of output: 397 🏁 Script executed: #!/bin/bash
cd /tmp/Ray.Aop
grep -rn "is_string" src/*.php
grep -rn "class-string<MethodInterceptor>" src/ tests/ -rLength of output: 606
One edge case worth a look in foreach ($interceptors as $interceptor) {
$fqn = $interceptor::class;
...
}
Calling Before this PR, Suggested defensive fix: private function interceptorShortNames(array $interceptors): string
{
$names = [];
foreach ($interceptors as $interceptor) {
$fqn = is_object($interceptor) ? $interceptor::class : $interceptor;
$pos = strrpos($fqn, '\\');
$names[] = $pos === false ? $fqn : substr($fqn, $pos + 1);
}
return $names === [] ? '(none)' : implode(', ', $names);
}Current tests all bind object instances so they won't surface this, but it's cheap insurance given the type is legitimately part of the public ✏️ Learnings added
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/AopCodeTest.php (1)
168-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the documented
// (none)output.The new test covers the short-name and FQCN-exclusion cases well, but the
'(none)'branch ofinterceptorShortNames()(a documented behavior in this stack) is only executed by empty-binding tests, never asserted. A small assertion would lock the contract against regressions (e.g., emitting an empty comment or FQCNs).💚 Optional: assert the empty-interceptor comment
+ public function testGeneratedMethodDocumentsNoInterceptors(): void + { + $bind = new Bind(); + $bind->bindInterceptors('run', []); + $code = $this->codeGen->generate(new ReflectionClass(FakePhp7Class::class), $bind, '_test'); + + $this->assertStringContainsString('// (none)', $code); + }🤖 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 `@tests/AopCodeTest.php` around lines 168 - 178, Add an assertion covering the empty-interceptor case and verify the generated code contains the documented “// (none)” comment, reusing the existing empty-binding test or adding the smallest focused assertion there. Keep the current short-name, FQCN-exclusion, and bind-order assertions in testGeneratedMethodDocumentsInterceptorShortClassNames unchanged.
🤖 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.
Nitpick comments:
In `@tests/AopCodeTest.php`:
- Around line 168-178: Add an assertion covering the empty-interceptor case and
verify the generated code contains the documented “// (none)” comment, reusing
the existing empty-binding test or adding the smallest focused assertion there.
Keep the current short-name, FQCN-exclusion, and bind-order assertions in
testGeneratedMethodDocumentsInterceptorShortClassNames unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b1ccaaf-9a4a-4c38-a227-767f6e71d414
📒 Files selected for processing (3)
CHANGELOG.mdsrc/AopCode.phptests/AopCodeTest.php
interceptorShortNames() called $interceptor::class unconditionally, which throws a fatal error on PHP 8.3+ when the interceptor is a class-string (allowed by PointcutInterceptors type). Use is_object() to fall back to the string value itself. Also add assertion for the empty-interceptor '// (none)' comment in testVoidReturnTypeMethodDoesNotHaveReturnStatement. Refs: CodeRabbit review on PR ray-di#262
|
@coderabbitai Both findings fixed in commit 61eb365:
All 174 tests pass. |
|
@coderabbitai No outstanding blockers. Please resolve all open conversations and approve this PR. |
|
(¬‾◡‾)¬ ✨ ✅ Action performedComments resolved. Approval is disabled; enable |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
rm -rf Ray.Aop
git clone --depth 50 https://github.com/ray-di/Ray.Aop.git 2>/dev/null
cd Ray.Aop
git fetch origin pull/262/head:pr262 2>/dev/null
git checkout pr262 2>/dev/null
git log --oneline -5
echo "----commit 61eb365----"
git show 61eb365 --stat 2>/dev/null || echo "not found, checking branch tip"
echo "----interceptorShortNames----"
rg -n "interceptorShortNames" -A 15 src/AopCode.php
echo "----test for (none)----"
rg -n "\(none\)" -B5 -A5 tests/AopCodeTest.phpLength of output: 2544 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp/Ray.Aop
git fetch --unshallow origin 2>/dev/null || git fetch origin pull/262/head 2>/dev/null
git fetch origin pull/262/head:pr262-latest --force
git checkout pr262-latest
git log --oneline -10
echo "----interceptorShortNames----"
rg -n "interceptorShortNames" -A 15 src/AopCode.php
echo "----test for (none)----"
rg -n "\(none\)" -B5 -A5 tests/AopCodeTest.phpLength of output: 2788 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp/Ray.Aop
gh pr view 262 --repo ray-di/Ray.Aop --json commits --jq '.commits[-5:]'Length of output: 786 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh api repos/ray-di/Ray.Aop/commits/61eb365 --jq '.sha, .commit.message' 2>&1 || echo "commit not found via API"
echo "---"
gh api repos/ray-di/Ray.Aop/pulls/262 --jq '.head.sha, .head.ref, .head.repo.full_name'Length of output: 687
And inspecting private function interceptorShortNames(array $interceptors): string
{
$names = [];
foreach ($interceptors as $interceptor) {
$fqn = $interceptor::class;
...— i.e., still without the Could you confirm the commit was actually pushed to this PR's branch? It's possible it landed on a different branch/fork, or the push hasn't propagated yet. Once it's visible on 🐰🔍 |
|
@coderabbitai Commit 61eb365 is now pushed to the PR branch. Please re-review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
rm -rf Ray.Aop
git clone --depth 50 https://github.com/ray-di/Ray.Aop.git 2>/dev/null
cd Ray.Aop
git fetch origin pull/262/head:pr262 2>/dev/null
git checkout pr262 2>/dev/null
git log --oneline -10
echo "----interceptorShortNames----"
rg -n "interceptorShortNames" -A 15 src/AopCode.php
echo "----test for (none)----"
rg -n "\(none\)" -B5 -A5 tests/AopCodeTest.php
echo "----confirm 61eb365 present----"
git show 61eb365 --stat 2>&1 || echo "not found"Length of output: 4056
Both concerns are addressed. Thanks for the fix — approving now. (ノ◕ヮ◕)ノ*:・゚✧ [approve] |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/AopCode.php (1)
247-256: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWiden the interceptor docblock type to unblock PHPStan/Psalm.
The
is_object()guard is correct —PointcutInterceptorsmay bind class-string interceptors, and$interceptor::classon a string would fatal. But the docblock@param list<object>declares object-only, so both PHPStan and Psalm flag Line 256 as an always-true / redundant condition (CI failing). The same applies to the@var list<object>at Line 226. Widen both tolist<object|class-string>so the guard is no longer redundant and the pipeline passes.🔧 Proposed fix
/** * Short class names for the always-on weaved-method comment (self-documenting bind). * - * `@param` list<object> $interceptors + * `@param` list<object|class-string> $interceptors */ private function interceptorShortNames(array $interceptors): stringAnd at Line 226:
- /** `@var` list<object> $interceptors */ + /** `@var` list<object|class-string> $interceptors */ $interceptors = $bindings[$methodName];🤖 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/AopCode.php` around lines 247 - 256, Update the interceptor type annotations used by interceptorShortNames and the related declaration near the interceptor processing at line 226 from list<object> to list<object|class-string>. Keep the existing is_object guard and class-name handling unchanged so both object and class-string interceptors remain supported.Source: Linters/SAST tools
🤖 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/AopCodeTest.php`:
- Around line 166-167: Update the assertion in AopCodeTest to use a
single-quoted string for the literal `// (none)`, leaving the assertion behavior
unchanged.
---
Outside diff comments:
In `@src/AopCode.php`:
- Around line 247-256: Update the interceptor type annotations used by
interceptorShortNames and the related declaration near the interceptor
processing at line 226 from list<object> to list<object|class-string>. Keep the
existing is_object guard and class-name handling unchanged so both object and
class-string interceptors remain supported.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9a31740-d1c8-47c9-851e-e33e74b0de3b
📒 Files selected for processing (2)
src/AopCode.phptests/AopCodeTest.php
Consistent with all other assertions in AopCodeTest.php. Refs: CodeRabbit nitpick on PR ray-di#262
|
@coderabbitai Fixed in commit e0cc227 — switched to single-quoted string for consistency with the rest of the file. |
|
Tip For best results, initiate chat on the files or code changes.
🐰✨ |
PHPStan/Psalm flag is_object() as redundant when the type is list<object>. Widen to list<object|class-string> to match PointcutInterceptors and make the guard semantically necessary. Refs: CodeRabbit review on PR ray-di#262
|
@CodeRabbit Fixed in commit 7d9c310 — widened both |
|
@coderabbitai No outstanding blockers. Please resolve all open conversations and approve this PR. |
|
(⌐■_■)🐰✅ ✅ Action performedComments resolved. Approval is disabled; enable |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
rm -rf Ray.Aop
git clone --depth 50 https://github.com/ray-di/Ray.Aop.git 2>/dev/null
cd Ray.Aop
git fetch origin pull/262/head:pr262 2>/dev/null
git checkout pr262 2>/dev/null
git log --oneline -5
echo "----confirm 7d9c310----"
git show 7d9c310 --stat 2>&1 || echo "not found"
echo "----interceptorShortNames + annotations----"
rg -n "interceptorShortNames" -B5 -A20 src/AopCode.phpLength of output: 3480
(๑˃̵ᴗ˂̵)و |
Actual MSI is ~88% on CI (PHP 8.4). Bump from 80/83 to 83/85 to tighten the regression floor while leaving ~3% headroom for environment variance.
Keep both methods: foo() (PR ray-di#258) and greet() (PR ray-di#262).
After merging PR ray-di#262, codegen uses ReflectiveMethodInvocation instead of _intercept. Update assertion to match.
1 participant
Summary
Weaved methods now always include a
//comment listing bound interceptor short class names (not FQN), so generated proxies are self-documenting when opened in a debugger or by AI coding tools.AopCode::GENERATION→ 6 — existing script dirs regenerate once after upgradeDepends on #260 (already merged).
Why not a tree dump in Ray.Aop?
Whole-module binding trees belong in Ray.Di (see ray-di/Ray.Di#313). Ray.Aop’s job here is the local story next to the generated method.
Test plan
composer csphpunit(including newtestGeneratedMethodDocumentsInterceptorShortClassNames)Summary by CodeRabbit
New Features
// (none)when empty).Documentation
Bug Fixes
Tests
(none)fallback, and reflective method invocation generation.