Optimize for PHP 8.2 readonly classes#166
Conversation
- Convert classes with all readonly properties to readonly classes - Simplify property declarations by using class-level readonly modifier - Update rector.php configuration for PHP 8.2 optimizations
Reviewer's GuideThis PR refactors the codebase to leverage PHP 8.2 readonly classes by converting existing classes with all readonly properties to Class diagram for updated readonly classesclassDiagram
class DonutRepository {
<<readonly>>
+QueryRepositoryInterface queryRepository
+HeaderSetter headerSetter
+ResourceStorageInterface resourceStorage
+ResourceInterface resource
+CdnCacheControlHeaderSetterInterface cdnCacheControlHeaderSetter
+RepositoryLoggerInterface logger
+DonutRendererInterface renderer
}
class QueryRepository {
<<readonly>>
+RepositoryLoggerInterface logger
+HeaderSetter headerSetter
+ResourceStorageInterface storage
+Expiry expiry
}
class DonutRequest {
<<readonly>>
+AbstractRequest request
+DonutRendererInterface donutStorage
+SurrogateKeys etags
}
class CommandsProvider {
<<readonly>>
+RefreshSameCommand refreshSameCommand
+RefreshAnnotatedCommand refreshAnnotatedCommand
}
class DonutCommandInterceptor {
<<readonly>>
+DonutRepositoryInterface repository
+MatchQueryInterface matchQuery
}
class RefreshAnnotatedCommand {
<<readonly>>
+QueryRepositoryInterface repository
+ResourceInterface resource
}
class RefreshSameCommand {
<<readonly>>
+QueryRepositoryInterface repository
+MatchQueryInterface matchQuery
}
class CacheDependency {
<<readonly>>
+UriTagInterface uriTag
}
class CacheInterceptor {
<<readonly>>
+QueryRepositoryInterface repository
}
class FastlyCachePurger {
<<readonly>>
+FastlyCachePurgerInterface fastlyCachePurger
}
class CliHttpCache {
<<readonly>>
+ResourceStorageInterface storage
}
class CommandInterceptor {
<<readonly>>
+array commands
}
class DevEtagSetter {
<<readonly>>
+CacheDependencyInterface cacheDeperency
}
class EtagSetter {
<<readonly>>
+CacheDependencyInterface cacheDeperency
}
class Expiry {
<<readonly>>
+array time
}
class HeaderSetter {
<<readonly>>
+EtagSetterInterface etagSetter
}
class HttpCache {
<<readonly>>
+ResourceStorageInterface storage
}
class MarshallerProvider {
<<readonly>>
+array options
}
class RefreshInterceptor {
<<readonly>>
+RefreshAnnotatedCommand command
}
class RedisDsnProvider {
<<readonly>>
}
DonutRepository --|> DonutRepositoryInterface
QueryRepository --|> QueryRepositoryInterface
DonutRequest --|> Stringable
CommandsProvider --|> ProviderInterface
DonutCommandInterceptor --|> MethodInterceptor
RefreshAnnotatedCommand --|> CommandInterface
RefreshSameCommand --|> CommandInterface
CacheDependency --|> CacheDependencyInterface
CacheInterceptor --|> MethodInterceptor
FastlyCachePurger --|> PurgerInterface
CliHttpCache --|> HttpCacheInterface
CommandInterceptor --|> MethodInterceptor
DevEtagSetter --|> EtagSetterInterface
EtagSetter --|> EtagSetterInterface
HttpCache --|> HttpCacheInterface
HttpCache --|> DeprecatedHttpCacheInterface
MarshallerProvider --|> ProviderInterface
RefreshInterceptor --|> MethodInterceptor
RedisDsnProvider --|> ProviderInterface
Class diagram for CacheVersion attribute class updateclassDiagram
class CacheVersion {
+string value
CacheVersion(string value)
}
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 📝 WalkthroughWalkthroughSystematic codebase refactoring introducing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Double-check that converting to readonly classes doesn’t inadvertently allow property mutation in methods or via reflection, since individual readonly modifiers are removed.
- Verify that the new RectorConfig fluent setup still registers all the intended rule sets and custom rules—removing explicit sets may change your transformation coverage.
- Review the bootstrap change from array_map('unlink', …) to array_map(unlink(...), …) to ensure the unlink callback is applied correctly and won’t cause unexpected failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Double-check that converting to readonly classes doesn’t inadvertently allow property mutation in methods or via reflection, since individual readonly modifiers are removed.
- Verify that the new RectorConfig fluent setup still registers all the intended rule sets and custom rules—removing explicit sets may change your transformation coverage.
- Review the bootstrap change from array_map('unlink', …) to array_map(unlink(...), …) to ensure the unlink callback is applied correctly and won’t cause unexpected failures.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.x #166 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 243 243
===========================================
Files 52 52
Lines 734 734
===========================================
Hits 734 734 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src-annotation-deprecated/CacheVersion.php (1)
18-20: Constructor property promotion correctly applied.The use of constructor property promotion is correct and simplifies the code. However, consider making the property
readonlyfor immutability and consistency with the broader PR pattern of converting classes to use readonly properties.Since this class is deprecated, this refactor is lower priority.
Apply this diff if you wish to make the property readonly:
- public function __construct(public string $value) + public function __construct(public readonly string $value) { }tests/Fake/fake-app/src/Resource/Page/Html/BlogPostingCacheControl.php (1)
24-25: Remove empty PHPDoc comment.The empty PHPDoc block provides no value. Modern PHP with type hints and attributes makes this unnecessary.
Apply this diff to remove it:
- /** - */ #[Embed(rel: "comment", src: "page://self/html/comment")] public function onGet()src/EtagSetter.php (2)
21-26: LGTM! Readonly class optimization correctly applied.The conversion to
final readonly classwith removal of the explicitreadonlymodifier from the constructor property is correct. In PHP 8.2, declaring a class asreadonlymakes all properties implicitly readonly, so the explicit modifier is redundant.Optional: Fix typo in property name.
The property is named
$cacheDeperency(line 24) but should be$cacheDependency. This typo is also present on line 82 where the property is used. While this doesn't affect functionality, fixing it would improve code maintainability.Apply this diff to fix the typo:
- private CacheDependencyInterface $cacheDeperency, + private CacheDependencyInterface $cacheDependency,And update the usage on line 82:
- $this->cacheDeperency->depends($ro, $body->resourceObject); + $this->cacheDependency->depends($ro, $body->resourceObject);
45-45: Optional: Fix additional typos in method and parameter names.Two additional pre-existing typos could be corrected:
- Line 45: Parameter name
$httpCaccheshould be$httpCache- Line 58: Method name
getEtagByEitireViewshould begetEtagByEntireViewApply this diff to fix these typos:
- public function getEtagByPartialBody(HttpCache $httpCacche, ResourceObject $ro): string + public function getEtagByPartialBody(HttpCache $httpCache, ResourceObject $ro): string { $etag = ''; assert(is_array($ro->body)); - foreach ($httpCacche->etag as $bodyEtag) { + foreach ($httpCache->etag as $bodyEtag) { if (isset($ro->body[$bodyEtag]) && is_string($ro->body[$bodyEtag])) { $etag .= $ro->body[$bodyEtag]; } } return $etag; } - public function getEtagByEitireView(ResourceObject $ro): string + public function getEtagByEntireView(ResourceObject $ro): string { return $ro::class . serialize($ro->view); }And update the call site on line 72:
- $etag = $httpCache instanceof HttpCache && $httpCache->etag ? $this->getEtagByPartialBody($httpCache, $ro) : $this->getEtagByEitireView($ro); + $etag = $httpCache instanceof HttpCache && $httpCache->etag ? $this->getEtagByPartialBody($httpCache, $ro) : $this->getEtagByEntireView($ro);Also applies to: 58-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
rector.php(1 hunks)src-annotation-deprecated/CacheVersion.php(1 hunks)src-annotation/Commands.php(0 hunks)src-annotation/RefreshCache.php(1 hunks)src/CacheDependency.php(1 hunks)src/CacheInterceptor.php(1 hunks)src/Cdn/FastlyCachePurger.php(1 hunks)src/CliHttpCache.php(1 hunks)src/CommandInterceptor.php(1 hunks)src/CommandsProvider.php(1 hunks)src/DevEtagSetter.php(1 hunks)src/DonutCommandInterceptor.php(1 hunks)src/DonutRepository.php(1 hunks)src/DonutRequest.php(1 hunks)src/EtagSetter.php(1 hunks)src/Expiry.php(1 hunks)src/HeaderSetter.php(1 hunks)src/HttpCache.php(1 hunks)src/MarshallerProvider.php(1 hunks)src/QueryRepository.php(1 hunks)src/RedisDsnProvider.php(1 hunks)src/RefreshAnnotatedCommand.php(1 hunks)src/RefreshInterceptor.php(1 hunks)src/RefreshSameCommand.php(1 hunks)tests-pecl-ext/StorageRedisDsnModuleTest.php(1 hunks)tests/Fake/FakeMobileEtagSetter.php(1 hunks)tests/Fake/fake-app/src/Resource/Page/Html/BlogPostingCacheControl.php(1 hunks)tests/ResourceRepositoryTest.php(2 hunks)tests/ResourceStorageTest.php(1 hunks)tests/bootstrap.php(1 hunks)
💤 Files with no reviewable changes (1)
- src-annotation/Commands.php
🧰 Additional context used
📓 Path-based instructions (6)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Adhere to PSR-12 coding standards using PHP_CodeSniffer for all PHP source files
Files:
tests/Fake/fake-app/src/Resource/Page/Html/BlogPostingCacheControl.phptests/bootstrap.phpsrc-annotation-deprecated/CacheVersion.phpsrc/CacheDependency.phptests-pecl-ext/StorageRedisDsnModuleTest.phpsrc/CacheInterceptor.phpsrc/CommandInterceptor.phpsrc/RefreshInterceptor.phpsrc/DonutRequest.phpsrc-annotation/RefreshCache.phptests/ResourceRepositoryTest.phpsrc/EtagSetter.phpsrc/HeaderSetter.phprector.phpsrc/RefreshSameCommand.phpsrc/Cdn/FastlyCachePurger.phpsrc/Expiry.phptests/ResourceStorageTest.phptests/Fake/FakeMobileEtagSetter.phpsrc/RedisDsnProvider.phpsrc/MarshallerProvider.phpsrc/CommandsProvider.phpsrc/HttpCache.phpsrc/DevEtagSetter.phpsrc/DonutRepository.phpsrc/RefreshAnnotatedCommand.phpsrc/CliHttpCache.phpsrc/DonutCommandInterceptor.phpsrc/QueryRepository.php
{src,tests}/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
{src,tests}/**/*.php: Ensure code passes PHPStan at max level as configured by phpstan.neon
Ensure code passes Psalm at error level 1 as configured by psalm.xml
Files:
tests/Fake/fake-app/src/Resource/Page/Html/BlogPostingCacheControl.phptests/bootstrap.phpsrc/CacheDependency.phpsrc/CacheInterceptor.phpsrc/CommandInterceptor.phpsrc/RefreshInterceptor.phpsrc/DonutRequest.phptests/ResourceRepositoryTest.phpsrc/EtagSetter.phpsrc/HeaderSetter.phpsrc/RefreshSameCommand.phpsrc/Cdn/FastlyCachePurger.phpsrc/Expiry.phptests/ResourceStorageTest.phptests/Fake/FakeMobileEtagSetter.phpsrc/RedisDsnProvider.phpsrc/MarshallerProvider.phpsrc/CommandsProvider.phpsrc/HttpCache.phpsrc/DevEtagSetter.phpsrc/DonutRepository.phpsrc/RefreshAnnotatedCommand.phpsrc/CliHttpCache.phpsrc/DonutCommandInterceptor.phpsrc/QueryRepository.php
tests/Fake/fake-app/**
📄 CodeRabbit inference engine (CLAUDE.md)
Exclude fake application test files from static analysis
Files:
tests/Fake/fake-app/src/Resource/Page/Html/BlogPostingCacheControl.php
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under tests/
Files:
tests/Fake/fake-app/src/Resource/Page/Html/BlogPostingCacheControl.phptests/bootstrap.phptests/ResourceRepositoryTest.phptests/ResourceStorageTest.phptests/Fake/FakeMobileEtagSetter.php
tests-pecl-ext/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Place PECL extension-related tests under tests-pecl-ext/
Files:
tests-pecl-ext/StorageRedisDsnModuleTest.php
src-annotation/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Define and maintain cache-related attributes #[Cacheable], #[HttpCache], #[Refresh], #[Purge] in src-annotation/
Files:
src-annotation/RefreshCache.php
🧠 Learnings (6)
📚 Learning: 2025-10-21T05:23:58.622Z
Learnt from: CR
Repo: bearsunday/BEAR.QueryRepository PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-21T05:23:58.622Z
Learning: Applies to {src,tests}/**/*.php : Ensure code passes PHPStan at max level as configured by phpstan.neon
Applied to files:
tests/bootstrap.phprector.php
📚 Learning: 2025-10-21T05:23:58.622Z
Learnt from: CR
Repo: bearsunday/BEAR.QueryRepository PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-21T05:23:58.622Z
Learning: Applies to tests-php8/**/*.php : Place PHP 8-specific tests under tests-php8/
Applied to files:
tests/bootstrap.php
📚 Learning: 2025-10-21T05:23:58.622Z
Learnt from: CR
Repo: bearsunday/BEAR.QueryRepository PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-21T05:23:58.622Z
Learning: Applies to tests/**/*.php : Place unit tests under tests/
Applied to files:
tests/bootstrap.php
📚 Learning: 2025-10-21T05:23:58.622Z
Learnt from: CR
Repo: bearsunday/BEAR.QueryRepository PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-21T05:23:58.622Z
Learning: Applies to src-annotation/**/*.php : Define and maintain cache-related attributes #[Cacheable], #[HttpCache], #[Refresh], #[Purge] in src-annotation/
Applied to files:
src-annotation-deprecated/CacheVersion.phpsrc/CacheDependency.phpsrc-annotation/RefreshCache.phpsrc/HttpCache.phpsrc/CliHttpCache.php
📚 Learning: 2024-11-01T15:45:27.018Z
Learnt from: koriym
Repo: bearsunday/BEAR.QueryRepository PR: 144
File: src/StorageRedisDsnModule.php:53-68
Timestamp: 2024-11-01T15:45:27.018Z
Learning: In `src/StorageRedisDsnModule.php`, lines 58 and 59 may both bind the Redis provider, and this can be necessary in certain cases.
Applied to files:
tests-pecl-ext/StorageRedisDsnModuleTest.phpsrc/RedisDsnProvider.php
📚 Learning: 2024-11-01T14:52:06.297Z
Learnt from: koriym
Repo: bearsunday/BEAR.QueryRepository PR: 144
File: src-deprecated/ResourceStorageCacheableTrait.php:24-24
Timestamp: 2024-11-01T14:52:06.297Z
Learning: When deprecating the `ResourceStorageCacheableTrait` in `src-deprecated/ResourceStorageCacheableTrait.php`, it is acceptable to only add the `deprecated` annotation without adding a runtime deprecation notice using `trigger_deprecation()`.
Applied to files:
src/HttpCache.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 (27)
tests-pecl-ext/StorageRedisDsnModuleTest.php (1)
56-59: Clean simplification of the ternary operator.The use of the Elvis operator (
?:) is a more concise and idiomatic approach than repeatinggetenv('REDIS_SERVER')in both the condition and the true branch. The behavior remains equivalent.src-annotation/RefreshCache.php (1)
10-10: LGTM! Clean and concise documentation.The single-line PHPDoc format is appropriate for a simple
@seereference and improves readability without changing functionality.tests/Fake/fake-app/src/Resource/Page/Html/BlogPostingCacheControl.php (1)
19-22: LGTM! Readonly modifier correctly applied to injected dependency.Making the repository property readonly is appropriate for enforcing immutability of the injected dependency. Note that this class cannot be converted to a full
readonly class(as done elsewhere in the PR) because the public$headersproperty at line 15 must remain mutable for the ResourceObject framework operations.tests/Fake/FakeMobileEtagSetter.php (1)
17-20: Readonly promotion aligns with the immutability goal.The promoted dependency was already treated immutably, so this readonly promotion is a clean, type-safe fit with no behavioral change.
tests/bootstrap.php (1)
5-5: Nice use of first-class callables.Switching to
unlink(...)keeps the cleanup terse while staying consistent with PHP 8.2 syntax; behavior matches the prior'unlink'callable.src/RedisDsnProvider.php (1)
20-41: Readonly class declaration fits the provider usage.The provider has no mutable state beyond construction, so marking it
final readonlytightens contracts without impacting theget()logic.tests/ResourceStorageTest.php (1)
22-31: Readonly promotion keeps the test double honest.The anonymous provider treats the adapter as immutable, so marking the promoted property readonly matches runtime behavior.
src/Expiry.php (1)
7-25: Readonly class captures the intended immutability.Marking
Expiryasfinal readonlyreflects that its time map never mutates after construction, tightening guarantees without altering behavior.src/CliHttpCache.php (1)
19-23: LGTM – class-level readonly fits this cache adapter. Promoting the class tofinal readonlykeeps$storageimmutable without redundant property modifiers, matching PHP 8.2 semantics for readonly classes. (php.watch)src/CacheDependency.php (1)
13-17: LGTM – readonly class keeps dependency guarantees. Declaring the classfinal readonlyensures$uriTagstays write-once while simplifying the constructor promotion. (php.watch)src/CommandInterceptor.php (1)
15-21: LGTM – interceptor immutability stays intact. Switching to afinal readonlyclass maintains the immutability of the injected command array without needing property-level modifiers. (php.watch)rector.php (1)
7-21: Nice config modernization. UsingRectorConfig::configure()withwithPaths()pluswithPhpSets()relies on Rector’s composer-aware PHP set discovery and keeps the level controls explicit. (getrector.com)src/HttpCache.php (1)
14-18: LGTM – readonly aligns with storage usage. Making the HTTP cache classfinal readonlypreserves the intended immutability for$storagewhile matching the broader PHP 8.2 pattern. (php.watch)src/CacheInterceptor.php (1)
20-24: LGTM – interceptor state stays immutable. The class-level readonly declaration keeps the repository dependency write-once while leaving runtime behaviour unchanged. (php.watch)src/RefreshAnnotatedCommand.php (1)
19-24: Readonly class conversion matches PHP 8.2 behavior.Switching to
final readonlywhile removing the per-propertyreadonlykeeps the immutability guarantees intact and aligns with the language feature meant to replace redundant modifiers. (php.watch)tests/ResourceRepositoryTest.php (1)
30-31: Immutability on test providers looks good.Marking the promoted adapter dependencies as
readonlymirrors their single-assignment usage in the tests and keeps the provider state intentional. (stitcher.io)Also applies to: 92-93
src/Cdn/FastlyCachePurger.php (1)
11-14: Readonly upgrade keeps the purger immutable.The move to
final readonlywith plain promoted properties still enforces single assignment while trimming redundant modifiers, matching PHP 8.2 guidance. (php.watch)src/DonutRequest.php (1)
13-18: Readonly class refactor keeps constructor semantics intact.
final readonlyguarantees the injected collaborators remain fixed after construction while letting the syntax stay concise. (php.watch)src/QueryRepository.php (1)
20-26: Repository readonly conversion removes redundant modifiers.Elevating the class to
final readonlymaintains the single-assignment guarantee for the promoted collaborators and aligns with PHP 8.2 best practices. (php.watch)src/MarshallerProvider.php (1)
23-29: Readonly provider setup stays valid with promoted defaults.The class-level
readonlykeeps the options array immutable after construction, and using a promoted default remains allowed under the PHP 8.2 rules. (stitcher.io)src/RefreshSameCommand.php (1)
14-19: LGTM! Correct PHP 8.2 readonly class conversion.The class-level
readonlydeclaration properly replaces individual propertyreadonlymodifiers, maintaining immutability while simplifying the syntax.src/CommandsProvider.php (1)
11-16: LGTM! Consistent readonly class conversion.The transformation correctly applies PHP 8.2's class-level readonly semantics.
src/HeaderSetter.php (1)
14-19: LGTM! Proper readonly class conversion.The class-level readonly declaration correctly supersedes the individual property modifier.
src/DevEtagSetter.php (1)
15-20: LGTM! Readonly class conversion applied correctly.The transformation maintains immutability guarantees while leveraging PHP 8.2's class-level readonly feature.
src/DonutCommandInterceptor.php (1)
22-28: LGTM! Readonly class conversion applied correctly.Both constructor-promoted properties correctly leverage the class-level readonly declaration.
src/RefreshInterceptor.php (1)
14-19: LGTM! Proper readonly class transformation.The class-level readonly declaration correctly enforces immutability for the injected command dependency.
src/DonutRepository.php (1)
15-26: LGTM! Comprehensive readonly class conversion.All seven constructor-promoted dependencies correctly leverage the class-level readonly declaration, maintaining immutability guarantees while simplifying the syntax.
f6cc5e9 to
7445f17
Compare
Summary
readonly classfeaturereadonlymodifiers when the entire class is readonlyChanges
This refactoring leverages PHP 8.2's readonly class feature, which allows marking an entire class as readonly instead of individual properties. This results in:
Example
Before:
After:
Test plan
Summary by Sourcery
Convert classes with exclusively readonly properties to PHP 8.2 readonly classes, remove redundant property modifiers, streamline attribute constructors, and update Rector configuration for PHP 8.2 compliance.
Enhancements:
Build:
Tests: