feat: rename — more PHP class kinds (controllers, jobs, services, form requests) - #86
Conversation
The FQCN class-rename engine is class-kind-agnostic: it already rewrites every reference and moves the backing file for any non-vendor project class, not just Eloquent models. Nothing in the rename path gates on model-ness. What was missing was proof. Add tests proving each newly-documented kind — controller, job, service, form request — renames its declaration and every kind-typical reference shape (use import / static dispatch / new / constructor + action type-hints / return type / `[Controller::class, 'index']` route action / docblock), leaving the enclosing class and look-alike members untouched. Existing model tests stay as the no-regression guard. Extract the file-move target into `class_rename::renamed_file_path` so AC's "moves the file" behaviour is unit-testable per repo convention (<module>/tests.rs), instead of an inline expression buried in the rename handler. Refs #54
Rename has always covered any non-vendor project class, but the docs described only "Eloquent model classes". Update them to reflect reality: - README feature table now lists "PHP classes (models, controllers, jobs, services, form requests)". - docs/rename.md broadens the F2 intro and the dedicated class-rename section to cover all common Laravel class kinds, with the route-action `::class` and constructor/action type-hint reference shapes called out. - Drop the now-shipped "More PHP class kinds" item from Planned Features. Refs #54
There was a problem hiding this comment.
✅ Approved
Review Summary
Reviewed PR #86 against the three acceptance criteria of #54, with four blind lens passes (AC-conformance, correctness, security, test-honesty) over the checked-out branch. CI is green across all five jobs.
The PR's thesis holds under scrutiny: the FQCN rename engine in class_rename.rs is genuinely class-kind-agnostic. I traced the LSP dispatch (main.rs ~20155–20203) and confirmed there is no model-only gate — prepare_class_rename/class_rename_edit filter only on is_dependency_path (vendor reject), so any non-vendor project class (controller, job, service, form request) reaches the exact same rename path a model does. This is therefore correctly scoped as tests + a small refactor + docs, not new engine logic.
- AC #1 — controller/job/service/form-request rename + file move ✅ Met. No kind gate on the trigger path;
renamed_file_pathmoves the backing file (same dir, basename swapped,.phppreserved). - AC #2 — no model-class regression ✅ Met. The only model-path change is extracting
renamed_file_pathfrom an inlinedecl_path.with_file_name(format!("{new_basename}.php"))— byte-for-byte identical, with an explicitapp/Models/User.php → Customer.phpcase in the file-move test as a regression guard. - AC #3 — tests per kind, per repo convention ✅ Met. One real test per kind in
class_rename/tests.rs, and therename()helper drives the realreference_spansengine (not a stub), assertinguse-import + reference rewrites and that base classes / look-alikes stay untouched.
What's Good
- The refactor earns its keep — extracting
renamed_file_pathmakes the "moves the file" behaviour unit-testable instead of buried in the rename handler. - Tests assert the negative too:
Controller/FormRequest/ShouldQueuebase classes and the enclosing class are explicitly verified unchanged. That's the part most reviewers skip. - Path traversal is closed —
new_basenameis validated as a bare identifier (alpha/_start, alphanumeric/_body) before the move, withoverwrite: Some(false)as a backstop.
Notes (non-blocking)
- Per-kind consumer fixtures only exercise the
use-import shape; inline-FQCN references (\App\Jobs\Foo::dispatch()) are covered for models viarenames_fully_qualified_referencesbut not re-asserted per new kind. The engine is shared, so coverage is real — just asymmetric. - Service/form-request docblock coverage asserts
@parambut not@return/@var. Harmless given the reference shapes involved.
Ready for @mikebronner to merge.
Summary
Implements #54 — rename for more PHP class kinds (controllers, jobs, services, form requests).
Key finding: the FQCN class-rename engine (
class_rename.rs+ theprepare_class_rename/class_rename_edithandlers) is already class-kind-agnostic. It rewrites every reference (use/ static call /new/ type-hint /::class/extends/implements/instanceof/ docblock) and moves the backing file for any non-vendor project class — nothing in the rename path gates on model-ness, andfind_php_class_fileresolvesapp/Http/Controllers,app/Jobs,app/Services,app/Http/Requestsexactly likeapp/Models. So controllers, jobs, services, and form requests already rename today.What was missing was proof and documentation. This PR adds that, plus a small testability refactor so the file-move behaviour is unit-testable.
Changes
class_rename::renamed_file_path— extracted the declaring-file move target (same dir, basename swapped,.phppreserved) from an inline expression in the rename handler into a documented, testable helper.main.rsnow calls it.class_rename/tests.rs) proving each kind renames its declaration + every kind-typical reference shape:[UserController::class, 'index']/Route::resource(...)route actions,useimport.SendWelcomeEmail::dispatch(...),dispatch(new ...),new ....new.@paramdocblock.renamed_file_pathmove-target cases for all four kinds (+ model regression guard) andclass_at_cursorresolution for a controller declaration and a job static dispatch.docs/rename.md, broadened from "Eloquent model classes" to all common PHP class kinds.Acceptance Criteria
renamed_file_pathmodel case added)<module>/tests.rs).Test Plan
cargo test --lib class_rename— 20 passed (13 pre-existing + 7 new).cargo fmt --check— clean.cargo clippy --all-targets -- -D warnings— clean (matches CI)..envfrom.env.example); the one remaining local failure (test_route_index_resolves_package_route) needs Fortify intest-project/vendor/, which CI installs viacomposer update— unrelated to this change.Fixes #54