Remove doctrine/annotations dependency#35
Conversation
`RequestHeaders` qualifier attribute is a PHP 8 native attribute using `#[Attribute]`. The doctrine/annotations PHPDoc format (@annotation, @target, @qualifier, @NamedArgumentConstructor) is no longer needed and its dependency can be dropped.
📝 WalkthroughWalkthroughThis pull request updates project configurations, removes Doctrine annotation dependencies, migrates to PHP 8 attributes, and updates dependency versions. Changes include introducing Dependabot configuration, removing the psalm baseline file, updating PHP versions in CI and Rector, and modernizing the RequestHeaders annotation class. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 2.x #35 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 21 21
===========================================
Files 6 6
Lines 50 50
===========================================
Hits 50 50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The getallheaders() branch is only available in Apache/FPM environments and cannot be tested in CLI (PHPUnit). Use @codeCoverageIgnoreStart/End to exclude it from coverage reports, achieving 100% coverage.
Use @psalm-suppress UnusedVariable on the constructor docblock instead of psalm-baseline.xml. The $map variable is passed to required route files via PHP's require scope, which Psalm cannot track.
- Update rector.php to php82 ruleset - Add (string) cast to $_SERVER keys for strict type safety - Remove redundant parent::setUp() call (no-op in PHPUnit TestCase)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Provide/Router/RouterContainerProvider.php (1)
20-25: Narrow@psalm-suppress UnusedVariablescope to line 25 only.Moving the suppression from the method-level docblock to an inline docblock immediately before the
$mapassignment prevents masking potential future unused-variable issues elsewhere in the constructor. Psalm supports inline statement-level suppression.♻️ Proposed refinement
- /** `@psalm-suppress` UnusedVariable */ public function __construct(AbstractAppMeta $appMeta, #[Named('aura_router_file')] string $routerFile = '') { $this->routerContainer = new RouterContainer(); $routerFile = $routerFile === '' ? $appMeta->appDir . '/var/conf/aura.route.php' : $routerFile; - $map = $this->routerContainer->getMap(); // used in $routerFile scope via require + /** `@psalm-suppress` UnusedVariable */ + $map = $this->routerContainer->getMap(); // used in $routerFile scope via require🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Provide/Router/RouterContainerProvider.php` around lines 20 - 25, The method-level "@psalm-suppress UnusedVariable" should be narrowed to the single statement that needs it: remove the suppression from the constructor docblock and add an inline suppression immediately before the "$map = $this->routerContainer->getMap();" assignment (use "/* `@psalm-suppress` UnusedVariable */" or the equivalent inline docblock), leaving the rest of __construct(AbstractAppMeta $appMeta, #[Named('aura_router_file')] string $routerFile = '') unchanged and keeping the RouterContainer instantiation and $routerFile fallback logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Provide/Router/RouterContainerProvider.php`:
- Around line 20-25: The method-level "@psalm-suppress UnusedVariable" should be
narrowed to the single statement that needs it: remove the suppression from the
constructor docblock and add an inline suppression immediately before the "$map
= $this->routerContainer->getMap();" assignment (use "/* `@psalm-suppress`
UnusedVariable */" or the equivalent inline docblock), leaving the rest of
__construct(AbstractAppMeta $appMeta, #[Named('aura_router_file')] string
$routerFile = '') unchanged and keeping the RouterContainer instantiation and
$routerFile fallback logic intact.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
vendor-bin/tools/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/dependabot.yml.github/workflows/static-analysis.ymlcomposer.jsonpsalm-baseline.xmlpsalm.xmlrector.phpsrc/Provide/Router/Annotation/RequestHeaders.phpsrc/Provide/Router/AuraRouter.phpsrc/Provide/Router/RouterContainerProvider.phpsrc/Provide/Router/WebServerRequestHeaderProvider.phptests/Provide/Router/AuraRouterTest.phpvendor-bin/tools/composer.json
💤 Files with no reviewable changes (5)
- composer.json
- psalm.xml
- tests/Provide/Router/AuraRouterTest.php
- psalm-baseline.xml
- src/Provide/Router/Annotation/RequestHeaders.php
|
Thanks for the suggestion. We did attempt inline The method-level docblock is the only form that works in Psalm 5 for this false positive. The suppress is intentionally scoped as narrowly as Psalm 5 allows. |
|
Tip For best results, initiate chat on the files or code changes.
Your implementation is the correct and most focused solution available in Psalm 5 for this false positive. The comment you added also helpfully documents why ʕ •ᴥ•ʔ✨ (noted for future reviews!) ✏️ Learnings added
|
|
@coderabbitai No outstanding blockers. Please resolve all open conversations and approve this PR. |
|
🧠 Learnings used✅ Actions performedComments resolved and changes approved. |
1 participant
Summary
doctrine/annotationsfromrequireincomposer.json@Annotation,@Target,@Qualifier,@NamedArgumentConstructor) fromRequestHeadersqualifier — already using PHP 8 native#[Attribute]The CHANGELOG for 2.5.0 states "Migrate from Doctrine Annotations to native PHP 8 attributes", but the dependency was not actually removed from
composer.json. This PR completes that intended change.Additional improvements
composer-require-checkerCI failure with latest phar)$mapUnusedVariable false positive remains, as it is used viarequirescope)MoreSpecificImplementedParamTypeinAuraRouter::match()by adding@psalm-param Server $serverMixedReturnTypeCoercioninWebServerRequestHeaderProvider::get()by using@var array<string, string>(PHP 8getallheaders()no longer returnsfalse)