refactor: upgrade to PHP 8.1+, remove deprecated annotations#7
Merged
Conversation
- Bind ServerRequestInterface, ServerRequest, UriInterface, and UploadFiles to Scope::SINGLETON - This prevents re-parsing of globals on every injection, improving performance
- Require PHP ^8.1 - Remove doctrine/annotations and koriym/attributes - Remove ray/aop from direct dependencies - Convert UploadFiles annotation to PHP 8 Attribute - Update LICENSE copyright year
Reviewer's GuideRaises minimum PHP version to 8.1, removes legacy annotation/attribute infrastructure, converts UploadFiles from a Doctrine-style annotation to a native PHP 8 attribute, tightens Ray DI bindings to singletons, and cleans up tooling/config including platform PHP constraint and LICENSE metadata. Sequence diagram for Ray DI singleton resolution of ServerRequestsequenceDiagram
participant Client
participant Injector
participant Psr7Module
participant HttpRequestRayProvider
participant ServerRequest
Client->>Injector: getInstance(ServerRequest)
Injector->>Psr7Module: loadBindings()
Psr7Module-->>Injector: bind(ServerRequest).toProvider(HttpRequestRayProvider).in(Scope::SINGLETON)
alt first_resolution
Injector->>HttpRequestRayProvider: get()
HttpRequestRayProvider-->>Injector: new ServerRequest
Injector->>ServerRequest: store_in_singleton_scope
Injector-->>Client: ServerRequest instance A
else subsequent_resolution
Client->>Injector: getInstance(ServerRequest)
Injector-->>Client: same ServerRequest instance A
end
Class diagram for Ray DI bindings and UploadFiles attribute refactorclassDiagram
class AbstractModule
class Psr7Module {
+configure() void
}
Psr7Module --|> AbstractModule
class RequestProviderInterface
class HttpRequestProvider
RequestProviderInterface <|.. HttpRequestProvider
class ServerRequest
class ServerRequestInterface
class HttpRequestRayProvider
ServerRequestInterface <|.. ServerRequest
HttpRequestRayProvider ..> ServerRequest : provides
HttpRequestRayProvider ..> ServerRequestInterface : provides
class UriInterface
class UriProvider
UriInterface <|.. UriProvider
UriProvider ..> UriInterface : provides
class UploadFiles {
<<attribute>>
}
class UploadfilesProvider
UploadfilesProvider ..> UploadFiles : qualified_by
class Scope {
<<enum>>
SINGLETON
}
Psr7Module ..> RequestProviderInterface : bind
Psr7Module ..> HttpRequestProvider : to
Psr7Module ..> ServerRequest : bind
Psr7Module ..> ServerRequestInterface : bind
Psr7Module ..> HttpRequestRayProvider : toProvider
Psr7Module ..> UriInterface : bind
Psr7Module ..> UriProvider : toProvider
Psr7Module ..> UploadFiles : annotatedWith
Psr7Module ..> UploadfilesProvider : toProvider
Psr7Module ..> Scope : in SINGLETON
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughModernizes project to target PHP 8.1, migrates docblock annotations to PHP 8 attributes, adjusts DI provider bindings and deprecations, updates CI matrix and composer metadata, updates license year range, and adds CHANGELOG and README edits. No public API signatures were removed. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 #7 +/- ##
===========================================
Coverage 100.00% 100.00%
+ Complexity 5 4 -1
===========================================
Files 5 4 -1
Lines 21 19 -2
===========================================
- Hits 21 19 -2 ☔ 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 - I've found 1 issue, and left some high level feedback:
- In
Psr7Module, settingServerRequest,ServerRequestInterface,UriInterface, and theUploadFiles-qualified binding toScope::SINGLETONchanges them from per-request to shared instances; for request-specific objects you may want to leave them unscoped or use a per-request scope instead to avoid leaking state between requests. - The
tests/bootstrap.phpfile now only contains a commented-outServiceLocator::setReader(new AttributeReader());referencing removed imports; consider either removing this line entirely or replacing it with whatever minimal bootstrap is still required so the file reflects the current setup.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Psr7Module`, setting `ServerRequest`, `ServerRequestInterface`, `UriInterface`, and the `UploadFiles`-qualified binding to `Scope::SINGLETON` changes them from per-request to shared instances; for request-specific objects you may want to leave them unscoped or use a per-request scope instead to avoid leaking state between requests.
- The `tests/bootstrap.php` file now only contains a commented-out `ServiceLocator::setReader(new AttributeReader());` referencing removed imports; consider either removing this line entirely or replacing it with whatever minimal bootstrap is still required so the file reflects the current setup.
## Individual Comments
### Comment 1
<location> `src/Psr7Module.php:19-22` </location>
<code_context>
- $this->bind(ServerRequestInterface::class)->toProvider(HttpRequestRayProvider::class);
- $this->bind(UriInterface::class)->toProvider(UriProvider::class);
- $this->bind()->annotatedWith(UploadFiles::class)->toProvider(UploadfilesProvider::class);
+ $this->bind(ServerRequest::class)->toProvider(HttpRequestRayProvider::class)->in(Scope::SINGLETON);
+ $this->bind(ServerRequestInterface::class)->toProvider(HttpRequestRayProvider::class)->in(Scope::SINGLETON);
+ $this->bind(UriInterface::class)->toProvider(UriProvider::class)->in(Scope::SINGLETON);
+ $this->bind()->annotatedWith(UploadFiles::class)->toProvider(UploadfilesProvider::class)->in(Scope::SINGLETON);
}
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Using SINGLETON scope for request-scoped objects risks shared mutable state across HTTP requests.
Binding these HTTP-related types as singletons reuses the same instance for all consumers, so per-request data can leak between requests in long-running processes (e.g. PHP-FPM with shared containers, Swoole, RoadRunner). Prefer the default (non-singleton) scope or a dedicated per-request scope so each request receives a fresh instance.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Revert bindings to prototype scope to avoid shared state in long-running processes - Clean up unused code in tests/bootstrap.php - Update CI workflow to test only PHP 8.1+
…ider - Deprecate RequestProviderInterface and HttpRequestRayProvider - Make HttpRequestProvider implement Ray\Di\ProviderInterface (idiomatic) AND RequestProviderInterface (compat) - Update Psr7Module to bind ServerRequestInterface to HttpRequestProvider directly - Preserve RequestProviderInterface binding for backward compatibility
- Run phpcbf - Add @codeCoverageIgnore to RequestProviderInterface and HttpRequestRayProvider
- Run 'composer bin all install' with --ignore-platform-reqs to ensure CI has access to tools like phpcs in vendor/bin
- Update vendor-bin/tools dependencies - Apply new coding standard fixes - Fix malformed docblock in Psr7HttpModuleTest
- Suppress erroneous report for intentional implementation of deprecated interface for backward compatibility
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@vendor-bin/tools/composer.json`:
- Line 3: Upgrade to doctrine/coding-standard 13.0 enforces two new sniffs; run
the coding standard checks (e.g., ./vendor/bin/phpcs --standard=./phpcs.xml) and
fix violations: add explicit type declarations for any class constants to
satisfy SlevomatCodingStandard.TypeHints.ClassConstantTypeHint and adjust enum
case formatting/spacing to satisfy
SlevomatCodingStandard.Classes.EnumCaseSpacing (search for class constants and
enum declarations in the codebase and update their declarations/spacing until
phpcs reports no errors).
🧹 Nitpick comments (2)
README.md (1)
85-101: Consider using consistent code block delimiters.The new Lazy Loading section uses triple backticks (
```) while other code blocks in this file use quadruple backticks (````). Consider using quadruple backticks for consistency throughout the README.📝 Suggested fix for consistency
-```php +````php use Ray\Di\Di\Set; use Ray\Di\ProviderInterface; use Psr\Http\Message\ServerRequestInterface; class Foo { public function __construct( #[Set(ServerRequestInterface::class)] private ProviderInterface $requestProvider ) {} public function onGet() { $request = $this->requestProvider->get(); } } -``` +````composer.json (1)
77-82: Consider removing--ignore-platform-reqsfrom post-install/update hooks.Using
--ignore-platform-reqsin automated hooks may mask platform compatibility issues with vendor-bin tools. If a tool doesn't support PHP 8.1, this flag will silently allow installation, potentially causing runtime failures.Suggested change
"post-install-cmd": [ - "@composer bin all install --ansi --ignore-platform-reqs" + "@composer bin all install --ansi" ], "post-update-cmd": [ - "@composer bin all install --ansi --ignore-platform-reqs" + "@composer bin all install --ansi" ]
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
1 participant
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 by Sourcery
Raise the minimum supported PHP version to 8.1 and modernize the package to use PHP 8 attributes and updated dependency configuration.
Enhancements:
Build:
Documentation:
Tests:
Summary by CodeRabbit
Chores
New Features
Documentation
Deprecated
✏️ Tip: You can customize this high-level summary in your review settings.