Conversation
- Update vendor-bin/tools dependencies to latest versions - Remove Doctrine annotations from DefaultSchemeHost, use PHP 8 attributes only - Add #[Override] attributes and final keywords to reduce Psalm errors from 62 to 27 Fix all Psalm errors and improve type safety - Update Psalm target PHP version from 7.4 to 8.2 - Suppress Unused* errors in psalm.xml (library code) - Fix null/false handling in WebRouter::match() - Replace ?: with ?? operator in HttpResponder for type safety - Psalm errors reduced from 27 to 0 Fix all PHPStan errors in src directory - Remove unnecessary assert() from ThrowableHandler - Add proper type annotations and @phpstan-ignore for parse_str() - Suppress trait.unused errors in phpstan.neon for deprecated traits - PHPStan errors in src: 10 → 0 - All tests passing (28 tests, 59 assertions)
Add symfony/polyfill-php83 and update dev dependencies - Add symfony/polyfill-php83 ^1.31 to support #[Override] in PHP 8.2 - Update vimeo/psalm from ^4.2 to ^6.0 - Update phpstan/phpstan from ^1.3 to ^2.0 - Update rector/rector from ^1.2.4 to ^2.0 - Update psalm/plugin-phpunit from ^0.13 to ^0.19 - All tests passing
- Remove static analysis and dev tools from main composer.json - Keep only bamarni/composer-bin-plugin and phpunit in require-dev - Tools (psalm, phpstan, phpcs, rector, etc.) managed in vendor-bin/tools - Add post-install/update commands with --ignore-platform-req=php - Set platform.php to 8.4.99 in vendor-bin/tools for compatibility
- Set build image to default-jammy. - Update environment to use PHP 8.4. - Simplify php-scrutinizer-run command. - Retain src/* path filter.
- Simplify `catch` blocks by omitting unused exception variables.
Reviewer's GuideThis PR replaces Doctrine annotations with native PHP attributes, raises the minimum PHP requirement to 8.2 (with polyfill support through 8.5), restructures development tooling and composer configurations, and resolves all static analysis issues by improving type safety and configuration files. Entity relationship diagram for updated composer dependencieserDiagram
COMPOSER_JSON {
string php
string bear_resource
string psr_log
string ray_aop
string ray_di
string symfony_polyfill_php83
}
COMPOSER_JSON ||--o| COMPOSER_DEV_DEP : has
COMPOSER_DEV_DEP {
string bamarni_composer_bin_plugin
string phpunit_phpunit
}
TOOLS_VENDOR_BIN {
string doctrine_coding_standard
string phpmd_phpmd
string phpmetrics_phpmetrics
string phpstan_phpstan
string ray_rector_ray
string rector_rector
string squizlabs_php_codesniffer
string vimeo_psalm
}
COMPOSER_DEV_DEP ||--o| TOOLS_VENDOR_BIN : manages
Class diagram for migration from Doctrine annotations to PHP 8 attributesclassDiagram
class DefaultSchemeHost {
<<Attribute>>
<<Qualifier>>
}
class ThrowableHandler {
+handle(Throwable $e, Request $request): ThrowableHandlerInterface
+transfer(): void
}
class WebRouter {
+match(array $globals, array $server)
+generate($name, $data)
-getQuery(string $method, array $globals, array $server): array
}
class NullHttpCache {
+isNotModified(array $server): bool
+transfer(): void
}
class NamedModule {
+__construct(array $names)
+configure(): void
}
class SundayModule {
+configure(): void
}
class ErrorModule {
+configure(): void
}
class RepresentationModule {
+configure(): void
}
class RouterModule {
+configure(): void
}
class HttpCacheModule {
+configure(): void
}
class HttpResponder {
+__invoke(ResourceObject $ro, array $server): void
-getOutput(ResourceObject $ro, array $server): Output
}
class HttpResponderModule {
+configure(): void
}
class NullError {
+handle(Exception $e, Request $request)
+transfer(): void
}
class VndError {
+handle(Exception $e, Request $request)
+transfer(): void
}
class ConditionalResponse {
+isModified(ResourceObject $ro, array $server): bool
+getOutput(array $headers): Output
}
class NullMatch {
}
class RouterMatch {
+__toString(): string
}
class NullTransfer {
+__invoke(ResourceObject $ro, array $server): void
}
class ErrorPage {
}
class Header {
+__invoke(ResourceObject $ro, array $server): array
}
class BadRequestJsonException {
}
class LogicException {
}
class RuntimeException {
}
DefaultSchemeHost --|> Attribute
DefaultSchemeHost --|> Qualifier
BadRequestJsonException --|> ExceptionInterface
LogicException --|> ExceptionInterface
RuntimeException --|> ExceptionInterface
NullMatch --|> RouterMatch
ErrorPage --|> ResourceObject
ThrowableHandler --|> ThrowableHandlerInterface
NullHttpCache --|> HttpCacheInterface
NullError --|> ErrorInterface
VndError --|> ErrorInterface
ConditionalResponse --|> ConditionalResponseInterface
NullTransfer --|> TransferInterface
Header --|> HeaderInterface
HttpResponder --|> TransferInterface
WebRouter --|> RouterInterface
NamedModule --|> AbstractModule
SundayModule --|> AbstractModule
ErrorModule --|> AbstractModule
RepresentationModule --|> AbstractModule
RouterModule --|> AbstractModule
HttpCacheModule --|> AbstractModule
HttpResponderModule --|> AbstractModule
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughPHP minimum version bumped to 8.2 with updated configurations. Classes made final throughout, #[Override] attributes added to override methods. Development dependencies moved to vendor-bin tools directory with bamarni/composer-bin-plugin. Minor code cleanup: removed unused catch variables, legacy annotations, and rector configuration. Changes
Sequence DiagramsequenceDiagram
participant Client
participant WebRouter
participant PathParser as Path Parser
participant Matcher as Route Matcher
participant Handler as Error Handler
Note over Client,Handler: WebRouter::match() - Path Extraction Refactoring
Client->>WebRouter: match($globals, $server)
WebRouter->>PathParser: parse_url($uri, PHP_URL_PATH)
PathParser-->>WebRouter: $path (or null)
alt Path is null/false
WebRouter->>WebRouter: $path = '/'
end
WebRouter->>Matcher: match with $path
Matcher-->>WebRouter: route match result
WebRouter-->>Client: matched route
Note over Client,Handler: ThrowableHandler - Error Type Checking
Client->>Handler: handle($e, $request)
alt $e instanceof Error
Handler->>Handler: wrap Error in ErrorException
else
Handler->>Handler: use Exception as-is
end
Handler->>Handler: $this->error->handle($e, $request)
Handler-->>Client: ThrowableHandlerInterface
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 @@
## 1.x #186 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 51 52 +1
===========================================
Files 18 18
Lines 141 123 -18
===========================================
- Hits 141 123 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Make sure the Override attribute has a valid namespace and class definition since
use Override;alone may not resolve to an actual attribute. - Confirm that moving static analysis tools into
vendor-bin/toolsstill installs and executes them properly in your CI and local workflows. - Consider reducing the number of suppressed Psalm issue handlers to avoid masking potential problems introduced in this migration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Make sure the Override attribute has a valid namespace and class definition since `use Override;` alone may not resolve to an actual attribute.
- Confirm that moving static analysis tools into `vendor-bin/tools` still installs and executes them properly in your CI and local workflows.
- Consider reducing the number of suppressed Psalm issue handlers to avoid masking potential problems introduced in this migration.
## Individual Comments
### Comment 1
<location> `src/Provide/Router/WebRouter.php:51-55` </location>
<code_context>
public function match(array $globals, array $server)
{
$method = strtolower($server['REQUEST_METHOD']);
+ $path = parse_url($server['REQUEST_URI'], PHP_URL_PATH);
+ $path = $path === false || $path === null ? '/' : $path;
return new RouterMatch(
</code_context>
<issue_to_address>
**suggestion:** Consider normalizing the path to avoid double slashes when concatenating with schemeHost.
Trim the trailing slash from schemeHost or the leading slash from path before concatenation to prevent double slashes in the URI.
```suggestion
// Normalize schemeHost and path to avoid double slashes
$schemeHost = rtrim($this->schemeHost, '/');
// Only trim leading slash if path is not just "/"
$normalizedPath = $path === '/' ? '' : ltrim($path, '/');
$uri = $schemeHost . '/' . $normalizedPath;
return new RouterMatch(
$method,
$uri,
$this->getQuery($method, $globals, $server),
);
```
</issue_to_address>
### Comment 2
<location> `src/Provide/Transfer/HttpResponder.php:43-45` </location>
<code_context>
{
$ro->toString(); // render and set headers
- return new Output($ro->code, ($this->header)($ro, $server), $ro->view ?: $ro->toString());
+ return new Output($ro->code, ($this->header)($ro, $server), $ro->view ?? $ro->toString());
}
}
</code_context>
<issue_to_address>
**question:** Switching from ?: to ?? may change behavior if $ro->view is set but falsy.
If you intend to fallback only when $ro->view is null, this change is correct. Otherwise, revert to ?: to handle all falsy values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@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.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit 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 (36)
.github/workflows/continuous-integration.yml(1 hunks).scrutinizer.yml(1 hunks)composer.json(2 hunks)demo/public/index.php(1 hunks)phpstan.neon(1 hunks)psalm.xml(2 hunks)rector.php(0 hunks)src/Annotation/DefaultSchemeHost.php(0 hunks)src/Exception/BadRequestJsonException.php(1 hunks)src/Exception/LogicException.php(1 hunks)src/Exception/RuntimeException.php(1 hunks)src/Extension/Error/NullError.php(1 hunks)src/Extension/Router/NullMatch.php(1 hunks)src/Extension/Router/RouterMatch.php(2 hunks)src/Extension/Transfer/NullHttpCache.php(1 hunks)src/Extension/Transfer/NullTransfer.php(2 hunks)src/Module/Constant/NamedModule.php(2 hunks)src/Module/SundayModule.php(1 hunks)src/Provide/Error/ErrorModule.php(1 hunks)src/Provide/Error/ErrorPage.php(1 hunks)src/Provide/Error/ThrowableHandler.php(2 hunks)src/Provide/Error/VndError.php(3 hunks)src/Provide/Representation/RepresentationModule.php(1 hunks)src/Provide/Router/RouterModule.php(2 hunks)src/Provide/Router/WebRouter.php(4 hunks)src/Provide/Transfer/ConditionalResponse.php(3 hunks)src/Provide/Transfer/Header.php(1 hunks)src/Provide/Transfer/HttpCacheModule.php(1 hunks)src/Provide/Transfer/HttpResponder.php(3 hunks)src/Provide/Transfer/HttpResponderModule.php(2 hunks)tests/Fake/Apps/FakeVendor/HelloWorldX/Resource/App/Greeting.php(1 hunks)tests/Inject/PsrLoggerInjectTest.php(0 hunks)tests/Module/Constant/NamedModuleTest.php(0 hunks)tests/Provide/Error/ThrowableHandlerTest.php(1 hunks)tests/bootstrap.php(1 hunks)vendor-bin/tools/composer.json(1 hunks)
💤 Files with no reviewable changes (4)
- tests/Inject/PsrLoggerInjectTest.php
- rector.php
- tests/Module/Constant/NamedModuleTest.php
- src/Annotation/DefaultSchemeHost.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (34)
src/Provide/Transfer/Header.php (1)
8-15: Verified: Polyfill is properly configured and Override attributes are correctly used.The
symfony/polyfill-php83(^1.31) is configured in composer.json, and all 17 files using the#[Override]attribute have the correspondinguse Override;import statement. The implementation insrc/Provide/Transfer/Header.phpis correct and follows the established pattern across the codebase.phpstan.neon (1)
9-11: The original review comment is incorrect. The traits are actively used—specifically intests/Fake/Inject/test files. However, since that directory is explicitly excluded from PHPStan analysis viaexcludePaths: */tests/Fake/*, PHPStan cannot see the usage and incorrectly reports them as unused.The suppression in
phpstan.neonis a legitimate workaround for this false positive, not a mask for genuinely unused code. No changes are needed.Likely an incorrect or invalid review comment.
.github/workflows/continuous-integration.yml (1)
12-12: LGTM! PHP 8.1 removal aligns with minimum version bump.Removing PHP 8.1 from the CI test matrix is consistent with the PR objective to raise the minimum PHP requirement to 8.2.
demo/public/index.php (1)
29-29: LGTM! Non-capturing catch syntax improves clarity.Removing unused exception variables is a good cleanup that leverages PHP 8.0+'s non-capturing catch syntax.
Also applies to: 32-32
psalm.xml (2)
4-4: LGTM! PHP version updated correctly.Updating the Psalm PHP version target to 8.2 aligns with the minimum version requirement.
21-24: Consider reviewing the need for these suppressions.Adding suppressions for
UnusedClass,PossiblyUnusedMethod,PossiblyUnusedProperty, andPossiblyUnusedReturnValuemay hide legitimate dead code or API design issues. These are typically valuable signals during refactoring.Were these suppressions added to work around specific false positives, or to silence warnings on intentionally exported public API? If the latter, consider documenting which public APIs are intentionally unused internally.
tests/bootstrap.php (1)
9-9: LGTM! First-class callable syntax is cleaner.Using
unlink(...)leverages PHP 8.1's first-class callable syntax, which is more explicit and type-safe than passing the function name as a string..scrutinizer.yml (1)
2-4: LGTM! Updated to Ubuntu Jammy with PHP 8.4.The upgrade to
default-jammy(Ubuntu 22.04) with PHP 8.4 aligns with the PHP version modernization in this PR.src/Extension/Router/NullMatch.php (1)
7-7: LGTM! Making leaf class final is good practice.Declaring
NullMatchas final prevents unintended inheritance and clearly communicates design intent.vendor-bin/tools/composer.json (1)
1-22: LGTM with verification suggestion.The tooling configuration follows best practices for isolated vendor-bin setup. All dependency versions are current and appropriate for PHP 8.2+ static analysis.
The platform lock to
"php": "8.4.99"at line 19 is unusually forward-looking. Verify this is intentional for testing against PHP 8.4 features, as it may cause compatibility warnings if any dependencies have strict PHP version constraints.src/Module/Constant/NamedModule.php (1)
7-7: LGTM! Excellent modernization improvements.The addition of
finalclass modifier and#[Override]attribute enhances type safety and prevents unintended inheritance. These changes follow PHP 8.3+ best practices with no behavioral impact.Also applies to: 10-10, 19-19
tests/Fake/Apps/FakeVendor/HelloWorldX/Resource/App/Greeting.php (1)
12-12: LGTM! Good use of readonly for immutability.The
readonlymodifier on the injectedResourceInterfacedependency prevents accidental mutation and leverages PHP 8.1+ immutability features. This is appropriate for constructor-injected dependencies.src/Extension/Transfer/NullHttpCache.php (1)
7-7: LGTM! Proper use of Override attribute.The
#[Override]attributes on both interface implementation methods provide compile-time verification and improve code clarity. No behavioral changes.Also applies to: 14-15, 20-21
src/Exception/LogicException.php (1)
7-9: LGTM! Appropriate use of final modifier.Making the exception class
finalis a best practice that prevents unintended inheritance while preserving functionality. This aligns with the project-wide pattern of finalizing exception classes.src/Provide/Transfer/HttpResponder.php (1)
9-9: LGTM! Proper Override attribute usage.The
#[Override]attribute on the__invoke()method provides compile-time verification of interface implementation with no behavioral changes.Also applies to: 22-23
src/Provide/Transfer/ConditionalResponse.php (1)
8-8: LGTM! Correct Override attribute usage.The
#[Override]attributes on both interface implementation methods (isModified()andgetOutput()) provide compile-time verification with no behavioral impact.Also applies to: 27-28, 36-37
src/Module/SundayModule.php (1)
12-12: LGTM! Excellent modernization improvements.The combination of
finalclass modifier and#[Override]attribute enhances type safety and prevents unintended inheritance. These PHP 8.3+ improvements have no behavioral impact and align with project-wide modernization efforts.Also applies to: 18-18, 20-21
src/Extension/Transfer/NullTransfer.php (1)
8-8: LGTM! Override attribute correctly applied.The
#[Override]attribute properly marks the interface implementation.Also applies to: 17-17
src/Extension/Error/NullError.php (1)
9-9: LGTM! Override attributes correctly applied.The
#[Override]attributes properly mark the interface implementations.Also applies to: 16-16, 22-22
composer.json (2)
24-24: Breaking change: PHP 8.2 minimum requirement.This aligns with the PR objectives but is a breaking change for users on PHP 8.0 or 8.1.
32-33: Vendor-bin tooling setup is correctly configured. The migration is complete with all dev tools properly moved to vendor-bin/tools/composer.json, post-install/post-update hooks correctly configured in the root composer.json to manage the vendor-bin dependencies, and the root composer.json appropriately reduced to only the bin-plugin and phpunit.src/Provide/Router/WebRouter.php (2)
11-11: LGTM! Override attributes correctly applied.The
#[Override]attributes properly mark the interface method implementations.Also applies to: 44-44, 61-61
92-92: PHPStan ignore annotation added for parse_str output.The
@phpstan-ignore varTag.nativeTypeannotation suppresses a type mismatch warning for the$putvariable populated byparse_str(). This is acceptable sinceparse_str()doesn't have precise type information.src/Provide/Router/RouterModule.php (2)
20-20: LGTM! Module class made final.Making the module class final prevents unintended inheritance and aligns with best practices.
9-9: LGTM! Override attribute correctly applied.The
#[Override]attribute properly marks theAbstractModule::configure()override.Also applies to: 22-22
src/Provide/Error/ErrorPage.php (1)
9-9: LGTM! Error page class made final.Making the class final prevents unintended inheritance and is consistent with the broader refactoring in this PR.
tests/Provide/Error/ThrowableHandlerTest.php (1)
43-43: Cosmetic test change:hello→HELLO.Both
echo hello;andecho HELLO;trigger an Error for an undefined constant in PHP 8.2+, so this change doesn't affect the test behavior. The uppercase version may be more conventional for constants.src/Exception/BadRequestJsonException.php (1)
9-9: LGTM! Exception class made final.Making the exception class final prevents unintended inheritance and is consistent with the broader refactoring pattern in this PR.
src/Exception/RuntimeException.php (1)
7-7: LGTM! Final exception class aligns with best practices.Making the exception class
finalprevents unintended inheritance and is a sensible design choice for framework exception types. This breaking change is appropriately documented in the PR objectives.src/Provide/Error/VndError.php (1)
15-15: LGTM! Override attributes correctly applied.The
#[Override]attributes onhandle()andtransfer()correctly indicate these methods implement theErrorInterface. This improves type safety by ensuring the signatures remain synchronized with the interface.Also applies to: 47-47, 65-65
src/Provide/Representation/RepresentationModule.php (1)
9-9: LGTM! Consistent module hardening pattern.The combination of
finalclass declaration and#[Override]attribute onconfigure()follows the project-wide pattern for DI modules. This prevents unintended inheritance while ensuring the method signature stays synchronized withAbstractModule.Also applies to: 13-13, 15-15
src/Provide/Error/ErrorModule.php (1)
9-9: LGTM! Follows the established module pattern.Consistent with other modules in this PR:
finalclass and#[Override]onconfigure().Also applies to: 12-12, 14-14
src/Provide/Transfer/HttpCacheModule.php (1)
9-9: LGTM! Consistent module hardening.Same pattern applied:
finalclass and#[Override]onconfigure().Also applies to: 15-15, 17-17
src/Provide/Error/ThrowableHandler.php (1)
13-13: LGTM! Override attributes correctly applied.The
#[Override]attributes onhandle()andtransfer()correctly indicate these methods implementThrowableHandlerInterface.Also applies to: 25-25, 38-38
Remove trailing slashes from schemeHost in constructor to ensure $this->schemeHost . $path never produces double slashes
Pass original Error as $previous parameter to maintain stack trace and chained exception handling
1 participant
Summary
Changes
Type Safety Improvements
PHP Version Support
symfony/polyfill-php83for PHP 8.3 feature supportold_stableversions in CI workflowDevelopment Tools
vendor-bin/toolsfor PHP 8.5 compatibilitybamarni/composer-bin-pluginandphpunitin mainrequire-devConfiguration Updates
Breaking Changes
Test Plan
Summary by Sourcery
Upgrade the codebase to PHP 8.2+ by replacing Doctrine annotations with native attributes, strengthening type safety, reorganizing development tooling, and updating CI to cover PHP 8.2–8.5.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests: