Add PHPStan rule verifying #[DbQuery] contracts (MVP)#105
Open
koriym wants to merge 2 commits into
Open
Conversation
Introduce a self-contained PHPStan extension under tools/phpstan/ that
statically checks the cross-artifact invariants of Ray.MediaQuery
#[DbQuery] interface methods that PHPStan core cannot see:
- rayMediaQuery.sqlFileNotFound / emptySqlFile: the {id}.sql backing a
#[DbQuery('id')] exists in a configured directory and is non-empty.
- rayMediaQuery.pagerReturnMismatch / missingPagerAttribute: #[Pager] is
present iff the return type is a PagesInterface.
- rayMediaQuery.invalidFactory: a declared factory: class exists and
exposes the configured factory method (static or instance).
Invalid type: literals are intentionally left to PHPStan core, which
already reports them via the 'row'|'row_list' constructor type.
The extension ships its own phpstan + phpunit so RuleTestCase can run
(the repo's vendor-bin/tools has phpstan but no phpunit, the root has
phpunit but no phpstan). It is wired into the repo phpstan.neon against
the tutorial SQL set, and rule code self-analyses at level max.
https://claude.ai/code/session_01HWD3hygK9SGrLvZvehW28x
Contributor
|
Warning Review limit reached
More reviews will be available in 16 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (21)
📝 WalkthroughWalkthroughThis PR introduces a complete PHPStan extension package ( ChangesPHPStan DbQuery Contract Extension
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.gitignore:
- Around line 3-4: The .gitignore currently references /tools/phpstan/vendor/
and /tools/phpstan/composer.lock which breaks the repo's tool-isolation
convention; move the PHPStan tool workspace into the isolated path
vendor-bin/tools/phpstan (i.e., install tool deps via
bamarni/composer-bin-plugin per composer.json) and update .gitignore entries to
match vendor-bin/tools/phpstan/vendor/ and
vendor-bin/tools/phpstan/composer.lock, or alternatively update the repository
tooling guideline in composer.json to accept /tools/phpstan/ if this PR
intentionally changes the standard.
In `@tools/phpstan/README.md`:
- Around line 85-98: The fenced code block that lists the tools/phpstan tree
(starting with the line containing "tools/phpstan/") is missing a language tag;
update the opening fence from ``` to ```text so the block becomes a fenced
"text" block (e.g., change the fence that precedes "tools/phpstan/" to ```text)
to satisfy markdownlint MD040 while leaving the inner filenames like
extension.neon, phpstan.neon.dist, src/Rules/DbQueryContractRule.php,
Support/AttributeFinder.php, tests/Rules/DbQueryContractRuleTest.php, and
Fixture/sql/*.sql unchanged.
🪄 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: e26d3027-0ba1-490a-8744-986f5c107171
📒 Files selected for processing (21)
.gitignorecomposer.jsonphpstan.neontools/phpstan/README.mdtools/phpstan/bootstrap.phptools/phpstan/composer.jsontools/phpstan/extension.neontools/phpstan/phpstan.neon.disttools/phpstan/phpunit.xml.disttools/phpstan/src/Rules/DbQueryContractRule.phptools/phpstan/src/Support/AttributeFinder.phptools/phpstan/src/Support/DbQueryAttribute.phptools/phpstan/src/Support/SqlFileResolver.phptools/phpstan/tests/Fixture/Data/FactoryInterface.phptools/phpstan/tests/Fixture/Data/FactoryWithoutFactoryMethod.phptools/phpstan/tests/Fixture/Data/PagerInterface.phptools/phpstan/tests/Fixture/Data/SqlFilesInterface.phptools/phpstan/tests/Fixture/Data/ValidFactory.phptools/phpstan/tests/Fixture/sql/empty_query.sqltools/phpstan/tests/Fixture/sql/existing_query.sqltools/phpstan/tests/Rules/DbQueryContractRuleTest.php
Comment on lines
+3
to
+4
| /tools/phpstan/vendor/ | ||
| /tools/phpstan/composer.lock |
Contributor
There was a problem hiding this comment.
Move PHPStan tool dependency isolation to vendor-bin/tools/.
Line 3 and Line 4 indicate a standalone dependency workspace under tools/phpstan/ (vendor/ and composer.lock), which conflicts with the repository’s declared tool-isolation convention. Please migrate this tooling to vendor-bin/tools/ (or update the guideline/policy in the same PR if this is intentionally changing standards). As per coding guidelines, composer.json: “Use bamarni/composer-bin-plugin for isolated tool dependencies in vendor-bin/tools/”.
🤖 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 @.gitignore around lines 3 - 4, The .gitignore currently references
/tools/phpstan/vendor/ and /tools/phpstan/composer.lock which breaks the repo's
tool-isolation convention; move the PHPStan tool workspace into the isolated
path vendor-bin/tools/phpstan (i.e., install tool deps via
bamarni/composer-bin-plugin per composer.json) and update .gitignore entries to
match vendor-bin/tools/phpstan/vendor/ and
vendor-bin/tools/phpstan/composer.lock, or alternatively update the repository
tooling guideline in composer.json to accept /tools/phpstan/ if this PR
intentionally changes the standard.
Source: Coding guidelines
Comment on lines
+85
to
+98
| ``` | ||
| tools/phpstan/ | ||
| extension.neon # parameters schema + service registration | ||
| phpstan.neon.dist # self-analysis of the rule code | ||
| src/ | ||
| Rules/DbQueryContractRule.php | ||
| Support/AttributeFinder.php | ||
| Support/DbQueryAttribute.php | ||
| Support/SqlFileResolver.php | ||
| tests/ | ||
| Rules/DbQueryContractRuleTest.php | ||
| Fixture/Data/*.php # interfaces/classes analysed by the rule tests | ||
| Fixture/sql/*.sql # SQL fixtures | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the fenced block in Layout section.
The code fence starting at Line 85 has no language tag (MD040). Use something like text to satisfy markdownlint consistently.
Suggested patch
-```
+```text
tools/phpstan/
extension.neon # parameters schema + service registration
phpstan.neon.dist # self-analysis of the rule code
src/
@@
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 85-85: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@tools/phpstan/README.md` around lines 85 - 98, The fenced code block that
lists the tools/phpstan tree (starting with the line containing
"tools/phpstan/") is missing a language tag; update the opening fence from ```
to ```text so the block becomes a fenced "text" block (e.g., change the fence
that precedes "tools/phpstan/" to ```text) to satisfy markdownlint MD040 while
leaving the inner filenames like extension.neon, phpstan.neon.dist,
src/Rules/DbQueryContractRule.php, Support/AttributeFinder.php,
tests/Rules/DbQueryContractRuleTest.php, and Fixture/sql/*.sql unchanged.
Source: Linters/SAST tools
Address review feedback: the extension's isolated dependency set conflicted
with the repo convention (tool deps live under vendor-bin/ via
bamarni/composer-bin-plugin). Relocate the whole package from tools/phpstan/
to vendor-bin/media-query-phpstan/ so it is installed as its own bamarni bin,
matching vendor-bin/tools/.
- Relocate package; root autoload-dev and phpstan.neon include now point at
vendor-bin/media-query-phpstan/.
- .gitignore: drop the bespoke tools/phpstan/* entries; the existing
/vendor-bin/*/vendor/ rule covers the vendor dir, and composer.lock is
committed to match vendor-bin/tools/. Ignore /vendor-bin/*/.phpunit.cache/.
- bootstrap.php still resolves the parent autoloader (two levels up), so the
root binaries can drive the extension's phpunit/phpstan configs:
./vendor/bin/phpunit -c vendor-bin/media-query-phpstan
./vendor/bin/phpstan analyse -c vendor-bin/media-query-phpstan/phpstan.neon.dist
- README: update paths/commands and add the missing fenced-block language tag
(markdownlint MD040).
composer sa, composer test (109), the extension rule tests (3) and the rule
self-analysis all pass after the move.
https://claude.ai/code/session_01HWD3hygK9SGrLvZvehW28x
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a self-contained PHPStan extension under
tools/phpstan/that statically verifies the#[DbQuery]contract of Ray.MediaQuery interfaces — the cross-artifact invariants PHPStan core cannot see. This is the MVP from the agreed plan (Phase 0 de-risking + Phase 1 rule), scoped to checks with near-zero false positives.Checks (error identifiers)
rayMediaQuery.sqlFileNotFound#[DbQuery('id')]has no{sqlDir}/id.sqlin any configured directory.rayMediaQuery.emptySqlFileid.sqlexists but is empty.rayMediaQuery.pagerReturnMismatch#[Pager]present but return type is not aPagesInterface.rayMediaQuery.missingPagerAttributePagesInterfacebut#[Pager]is missing.rayMediaQuery.invalidFactoryfactory:names a missing class or missing factory method.Deliberately not implemented:
type:literal — PHPStan core already reports it via the'row'|'row_list'constructor type (argument.type). Verified with a fixture.#[Input]flattening and#[SqlTemplate]handling first to stay false-positive-free.Phase 0 findings (de-risked before building)
InClassMethodNodedoes fire on bodiless interface methods (every#[DbQuery]consumer is an interface — proven with a PoC before relying on it).RuleTestCaseneeds phpstan and phpunit in the same vendor. The repo'svendor-bin/toolshas phpstan but no phpunit; the root has phpunit but no phpstan. So the extension carries its owncomposer.json/vendor with both — this also matches the eventual standaloneray/media-query-phpstanpackage.Integration
phpstan.neonnowincludestools/phpstan/extension.neonwithrayMediaQuery.sqlDirectoriespointed at the tutorial SQL set (the only#[DbQuery]consumers in the analysed paths, sincetests/Fake/*is excluded). The rule runs against the tutorial's 12#[DbQuery]methods and is green.tools/phpstan/phpstan.neon.dist) and follows PHPStan's API best practices (Type::getConstantStrings()).Verification
composer sa(psalm + phpstan): greencomposer test: 109 passedtools/phpstanrule tests (RuleTestCase): 3 passed — each error case + the valid cases (exact-match, so no false positives).sqlcorrectly raisesrayMediaQuery.sqlFileNotFound; restoring returns to green.Caveat documented
PHPStan's result cache is keyed on PHP files, so a
.sql-only change won't invalidate it locally (CI is unaffected).tools/phpstan/README.mddocuments runningcomposer cleanafter SQL edits.Follow-ups (later phases, not in this PR)
NamedParameterExtractorSQL lexer (string/comment/::cast/:=aware).#[Input]/#[SqlTemplate].strictchecks (e.g.void+ SELECT, single-row nullability, factory return-type alignment).tools/phpstanrule tests in the pipeline (currently run locally via its own vendor).https://claude.ai/code/session_01HWD3hygK9SGrLvZvehW28x
Generated by Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests