fix: re-emit weaved class file when declared in-process but missing on disk#261
Conversation
…n disk
Ray\Aop\Compiler::compile() skipped requireFile() whenever the weaved class
was already declared in the process, even if the generated file had been
removed. A compile pipeline that weaves, cleans the script dir, and
recompiles (e.g. BEAR.Package's Compiler::__invoke flow: getInstance weaves
+ declares, clean() wipes, recompile skips) then shipped output without the
weaved class definition files. Runtime loaders that resolve weaved classes
by name (CompiledInjector) cannot regenerate them — there is no AOP weaver
at runtime — so resolving an intercepted resource throws "Class not found".
The weaved class file is part of the compiled output contract ("Generates a
new class file"). Always emit it when it is missing, regardless of whether
the class is already declared; require_once is a no-op if the file was
already loaded, so there is no redeclaration risk.
Added tests/WeavedFileReemissionTest.php: weave, delete the file, re-weave,
assert the file is re-emitted. Fails without the fix.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 2.x #261 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 227 228 +1
===========================================
Files 29 29
Lines 584 584
===========================================
Hits 584 584 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesWeaved file re-emission
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/WeavedFileReemissionTest.php (1)
46-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: assert class remains declared after re-weave.
Adding a
class_exists($fqn, false)assertion after the re-weave would explicitly document that the already-declared class is not disrupted by re-emission, complementing the file-existence assertion.♻️ Proposed addition
// Re-weave: must re-emit the file even though the class is declared. $compiler->compile(FakeMock::class, $bind); $this->assertTrue(file_exists($file), 're-weave must re-emit the weaved file when it is missing'); + $this->assertTrue(class_exists($fqn, false), 're-weave must not unset the already-declared class');🤖 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/WeavedFileReemissionTest.php` around lines 46 - 48, Add a post-re-weave assertion in the test around compile(FakeMock::class, $bind) that verifies class_exists(FakeMock::class, false) remains true, alongside the existing file_exists assertion.src/Compiler.php (1)
87-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: avoid duplicate
getFileNamecall.
getFileName($className->fqn)is now computed at line 87 incompile()and again at line 133 inrequireFile()with identical arguments. Passing$fileas a parameter torequireFile()would eliminate the redundant computation and keep the path logic in one place.♻️ Proposed refactor
$className = new AopPostfixClassName($class, (string) $bind, $this->classDir); $file = $this->getFileName($className->fqn); if (! class_exists($className->fqn, false) || ! file_exists($file)) { try { - $this->requireFile($className, new ReflectionClass($class), $bind); + $this->requireFile($className, new ReflectionClass($class), $bind, $file); // `@codeCoverageIgnoreStart`- private function requireFile(AopPostfixClassName $className, ReflectionClass $sourceClass, BindInterface $bind): void + private function requireFile(AopPostfixClassName $className, ReflectionClass $sourceClass, BindInterface $bind, string $file): void { - $file = $this->getFileName($className->fqn); if (! file_exists($file)) {Also applies to: 131-142
🤖 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/Compiler.php` around lines 87 - 88, Update compile() and requireFile() to compute getFileName($className->fqn) once and pass the resulting $file into requireFile(). Adjust requireFile()’s signature and its callers to use the supplied path, removing the duplicate path calculation while preserving existing behavior.
🤖 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 `@src/Compiler.php`:
- Around line 87-88: Update compile() and requireFile() to compute
getFileName($className->fqn) once and pass the resulting $file into
requireFile(). Adjust requireFile()’s signature and its callers to use the
supplied path, removing the duplicate path calculation while preserving existing
behavior.
In `@tests/WeavedFileReemissionTest.php`:
- Around line 46-48: Add a post-re-weave assertion in the test around
compile(FakeMock::class, $bind) that verifies class_exists(FakeMock::class,
false) remains true, alongside the existing file_exists assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17e6718e-a73e-4e19-9ea5-180cc6fe4333
📒 Files selected for processing (3)
CHANGELOG.mdsrc/Compiler.phptests/WeavedFileReemissionTest.php
- Compiler::compile() computes the weaved file path once and passes it to requireFile(), removing the duplicate getFileName() call. - WeavedFileReemissionTest asserts the class stays declared after the re-weave, alongside the file-existence assertion.
|
@coderabbitai Thanks for the review. Both nitpicks are addressed in f07e24b:
Please re-check when convenient. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 261 --repo ray-di/Ray.Aop --json commits --jq '.commits[-5:][] | {oid, messageHeadline}'
echo "---diff---"
gh pr diff 261 --repo ray-di/Ray.Aop | sed -n '1,200p'Length of output: 5322
Nicely done. Thanks for the quick turnaround! ヽ(=^･ω･^=)ﾉ |
|
@CodeRabbit Both nitpicks addressed in commit f07e24b:
No outstanding blockers. Please resolve all open conversations and approve this PR. |
|
(=^･ω･^=)ノ ✅ Action performedComments resolved. Approval is disabled; enable |
1 participant
Summary
Ray\Aop\Compiler::compile()skippedrequireFile()whenever the weaved class was already declared in the process — even if the generated file had been removed. A compile pipeline that weaves, cleans the script dir, and recompiles then shipped output without the weaved class definition files, and runtime loaders that resolve weaved classes by name cannot regenerate them (there is no AOP weaver at runtime), so resolving an intercepted resource throwsClass not found.Repro (BEAR.Package AOT)
bin/compile.phpdoesCompiler::fromInjector(Injector::getInstance($context), $context)():Injector::getInstance()weaves every intercepted binding → writes the weaved_<postfix>.phpfiles and declares the classes in-process.Compiler::__invoke()→clean()wipes the script dir (files gone, classes stay declared).compile()again, butclass_exists($fqn, false)istrue(declared in step 1) →requireFile()is skipped → the weaved files are never re-emitted.Result: the shipped
di/is missing weaved class files (e.g.FakeDep_3881589280.php,…Resource_App_Emb_….phpfor@Embed). Booting from it withCompiledInjectorand resolving an intercepted resource:@Embed(HATEOAS) weaves resources, so this is not an edge case.Fix
compile()is documented@sideEffect Generates a new class file. Always emit the file when it is missing, regardless of whether the class is already declared:requireFile()writes the file (guarded byif (! file_exists($file))) thenrequire_onces it.require_onceis a no-op when the file was already loaded, so re-emitting after the class is declared carries no redeclaration risk.Why this layer
The weaved class file is part of the compiled-output contract. The bug is that
compile()stops honouring it once the class is declared, which makes the output depend on in-process state rather than on what is on disk. This is the AOP analogue of ray-di/Ray.Compiler#136 (compiled scripts must not bake in-process/build-time state). It unblocks reuse of AOT output for immutable images / read-only filesystems (BEAR.Package #483).Test
tests/WeavedFileReemissionTest.php: weave → delete the file → re-weave → assert the file is re-emitted. Fails without the fix (false is true), passes with it.Checks
phpcs,phpstan,psalm: cleanphpunit: 165 tests (164 existing + new), 296 assertions — no regressionClass not found.🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Summary by CodeRabbit