Introduce StorageRedisDsnModule #144
Conversation
Introduced `StorageRedisDsnModule` and `RedisDsnProvider` to manage Redis connections using DSN. Added `RedisDsn` annotation for dependency injection. Included a test class `StorageRedisDsnModuleTest` to validate Redis integration.
The StorageRedisModule has been moved to the src-deprecated directory and marked as deprecated. Users should now utilize the StorageRedisDnsModule for related functionality.
Introduce `StorageMemcachedEtagModule` for managing Etag pools using Memcached. This commit includes a unit test to ensure proper instantiation and configuration of the CacheItemPoolInterface with Memcached.
Added type annotations to `RedisDsnProvider` to specify it implements `ProviderInterface` for multiple Redis-related classes. This enhances type safety and code clarity.
The doc comment for the $redisServer parameter was incorrect and has been deleted. This helps in maintaining accurate and clear documentation within the code.
|
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 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes across multiple files, including updates to dependency configurations, the introduction of new classes for Redis and Memcached integration, and the deprecation of certain modules and traits. Key modifications involve enhancing caching strategies through the use of Symfony's caching components, adjustments to dependency injection configurations, and the addition of new test cases to validate these changes. Overall, the updates reflect a significant restructuring aimed at improving resource management and caching mechanisms within the application. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RedisDsnProvider
participant ResourceStorage
participant Cache
User->>RedisDsnProvider: Request Redis Connection
RedisDsnProvider->>Cache: Create Connection
Cache-->>RedisDsnProvider: Return Redis Instance
RedisDsnProvider-->>User: Return Redis Connection
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
koriym
force-pushed
the
redis-dsn
branch
3 times, most recently
from
7bf7388 to
b965ee9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 1.x #144 +/- ##
=============================================
- Coverage 100.00% 93.52% -6.48%
- Complexity 235 248 +13
=============================================
Files 51 54 +3
Lines 730 803 +73
=============================================
+ Hits 730 751 +21
- Misses 0 52 +52 ☔ View full report in Codecov by Sentry. |
This commit introduces the capability to specify additional options for Redis DSN through the `RedisDsnOptions` annotation. The `StorageRedisDsnModule`, `RedisDsnProvider`, and associated tests have been updated to accommodate these options, allowing for more flexible Redis connection configurations.
Added a comment to suppress Psalm warnings for the undefined Relay class in RedisDsnProvider. This ensures static analysis is cleaner and avoids false positives related to the Relay class usage.
This change updates the "ray/di" package to use the "dev-provider-serialisable" branch, aliased as version 2.18.0. This update aims to include necessary fixes and improvements from the selected branch.
Replaced inline array type annotation with a reusable Props type definition. This change reduces redundancy and improves maintainability by ensuring that the data structure is consistent across the class.
Add suppression for unused parameter warnings in NullTagAwareAdapter class. Unset parameters in getItem() and clear() methods to prevent IDE warnings and improve code clarity.
Correct the spelling mistake in the "@psalm-suppress UndefinedClass for Relay" annotation by removing the erroneous "n" at the end. This ensures accurate annotation and helps in maintaining code quality and readability.
The @codeCoverageIgnore annotation was added to suppress coverage checks on the NullTagAwareAdapter class. This change ensures that code coverage tools do not flag this intended mock implementation.
Add a configuration to suppress the UndefinedClass error specifically for the src/RedisDsnProvider.php file. This ensures the psalm static analysis tool does not report this issue, facilitating a smoother development workflow.
Updated method signatures by removing type hints for `$key` parameters in `hasItem` and `deleteItem` methods. This change improves compatibility with different data types that might be passed to these methods.
Upgraded ray/aop from ^2.10 to ^2.16 in composer.json to ensure compatibility with the latest features and fixes. This change helps maintain the application's stability and performance by incorporating essential updates.
Deleted the NullTagAwareAdapter class and changed references to it throughout the codebase. Adjusted the etagPool initialization and modified DI bindings to handle null instances properly. Ensured compatibility with TagAwareAdapterInterface.
This commit updates Dependabot configuration to ignore major version updates for phpunit/phpunit. This ensures we avoid breaking changes from phpunit while still receiving minor and patch updates for other dependencies.
Introduce a new `TagsPool` annotation class within the `BEAR\RepositoryModule\Annotation` namespace. This class is marked as a PHP attribute and qualifier, targeting parameters for dependency injection purposes.
Introduced the TagsPool annotation and updated bindings to include it in QueryRepositoryModule. Now, TagsPool will follow the same null binding logic as ResourceObjectPool. Adjusted TagAwareAdapterInterface to use TagsPool annotation.
This commit eliminates the ApcuAdapter binding for the CacheItemPoolInterface in StorageRedisDsnModule.php. The binding for shared cache using RedisAdapter remains intact, ensuring consistent behavior across shared caches.
Moved StorageRedisMemcachedModule to src-deprecated directory and marked it as deprecated. This change helps to signal the outgoing use of this module and prepares for its removal in future updates.
Moved StorageRedisModuleTest to tests-deprecated folder and added a @deprecated annotation to the class. This change indicates that the test is outdated and is scheduled to be removed in future releases.
Mark StorageApcModuleTest with a deprecation notice. This indicates that the class is outdated and should not be used in future developments.
Removed the unnecessary binding for CacheItemPoolInterface-Shared::class from QueryRepositoryModule.php. This change streamlines the configuration by removing redundant dependencies.
This commit cleans up the StorageRedisDsnModule by removing unnecessary PSR cache annotations and the binding to the deprecated RedisAdapter class. These changes simplify the module and ensure only the required components are included, improving maintainability.
Updated StorageMemcachedModule to bind TagAwareAdapter for better cache handling. Modified corresponding test to reflect and verify these changes, ensuring compatibility with new annotations and dependencies.
Revert "ray/di" package from "dev-provider-serialisable as 2.18.0" to stable "^2.17.2" version. This change ensures compatibility with other dependencies and improves stability.
|
@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: 1
Outside diff range, codebase verification and nitpick comments (1)
tests-pecl-ext/StorageMemcachedEtagModuleTest.php (1)
1-23: LGTM, but consider adding more test cases.The code changes are approved.
The test case checks the expected behavior of the
StorageMemcachedEtagModuleclass. However, consider adding more test cases to cover different scenarios, such as:
- Test case for invalid Memcached server configuration.
- Test case for checking the behavior of the cache adapter (e.g., setting and getting cache items).
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (30)
- .github/dependabot.yml (1 hunks)
- composer.json (1 hunks)
- psalm.xml (1 hunks)
- src-annotation/EtagPool.php (1 hunks)
- src-annotation/RedisDsn.php (1 hunks)
- src-annotation/RedisDsnOptions.php (1 hunks)
- src-annotation/ResourceObjectPool.php (1 hunks)
- src-annotation/TagsPool.php (1 hunks)
- src-deprecated/BcModule.php (1 hunks)
- src-deprecated/NamespacedCacheProvider.php (1 hunks)
- src-deprecated/ResourceStorageCacheableTrait.php (1 hunks)
- src-deprecated/StorageRedisMemcachedModule.php (1 hunks)
- src-deprecated/StorageRedisModule.php (1 hunks)
- src/QueryRepositoryModule.php (3 hunks)
- src/RedisDsnProvider.php (1 hunks)
- src/ResourceStorage.php (5 hunks)
- src/StorageMemcachedEtagModule.php (1 hunks)
- src/StorageMemcachedModule.php (2 hunks)
- src/StorageRedisDsnModule.php (1 hunks)
- tests-deprecated/ResourceStorageCacheableTrait.php (1 hunks)
- tests-deprecated/StorageApcModuleTest.php (1 hunks)
- tests-deprecated/StorageRedisModuleTest.php (1 hunks)
- tests-pecl-ext/StorageMemcachedEtagModuleTest.php (1 hunks)
- tests-pecl-ext/StorageMemcachedModuleTest.php (1 hunks)
- tests-pecl-ext/StorageRedisDsnModuleTest.php (1 hunks)
- tests/HttpCacheTest.php (3 hunks)
- tests/ModuleFactory.php (2 hunks)
- tests/QueryRepositoryTest.php (4 hunks)
- tests/ResourceRepositoryTest.php (3 hunks)
- tests/ResourceStorageTest.php (1 hunks)
Files skipped from review due to trivial changes (6)
- .github/dependabot.yml
- src-annotation/RedisDsnOptions.php
- src-deprecated/BcModule.php
- src-deprecated/ResourceStorageCacheableTrait.php
- src-deprecated/StorageRedisModule.php
- tests-deprecated/StorageApcModuleTest.php
Additional comments not posted (45)
src-annotation/EtagPool.php (1)
10-14: Verify the impact of removing the constructor and thevalueproperty.The following simplifications have been made to the
EtagPoolclass:
- The
@NamedArgumentConstructorannotation has been removed.- The constructor has been removed.
- The
public string $valueproperty has been removed.Verify that these changes do not break any existing code that relies on the constructor for instantiation or expects the
valueproperty to be set.Run the following script to find any usages of the constructor or the
valueproperty:src-annotation/RedisDsn.php (1)
1-14: LGTM!The new
RedisDsnclass is a simple attribute that can be used to qualify parameters. The code changes are approved.src-annotation/TagsPool.php (1)
1-14: LGTM!The new
TagsPoolclass is a simple attribute that can be used to qualify parameters. The code changes are approved.src-annotation/ResourceObjectPool.php (1)
1-14: LGTM!The code changes are approved.
psalm.xml (1)
17-23: Verify the impact of suppressing undefined class errors.The changes suppress undefined class errors for the file
src/RedisDsnProvider.php. This may affect how code quality is assessed during analysis.Please ensure that suppressing these errors does not introduce any unintended consequences or mask potential issues in the codebase.
Run the following script to verify the impact:
If the suppression is necessary, consider adding a comment in the
psalm.xmlfile explaining the reason for suppressing the errors.src-deprecated/NamespacedCacheProvider.php (1)
13-15: LGTM!The
@deprecatedannotation has been correctly added to the class to mark it as deprecated. The removal of the redundant deprecation notice in the method documentation is also appropriate.tests/ModuleFactory.php (2)
22-23: LGTM!The
CacheItemPoolInterfacebinding has been correctly replaced withAdapterInterfacefor theResourceObjectPoolandEtagPoolannotations. The new binding is correctly annotated and bound to theArrayAdapterimplementation.
22-23: Verify the impact of removing the singleton scope.The removal of the
Scope::SINGLETONspecification from the bindings may affect the lifecycle management of the bound services. It is important to ensure that this change does not introduce any unintended side effects or performance issues.Please verify that the removal of the singleton scope is intentional and that it does not negatively impact the application's behavior or performance.
src/RedisDsnProvider.php (1)
1-36: LGTM!The
RedisDsnProviderclass is a well-structured provider that follows the dependency injection principles. The class is properly annotated and the constructor dependencies are clearly defined. Thegetmethod returns the Redis connection based on the provided DSN and options, which is a common pattern for provider classes.The implementation looks good and can be approved.
tests-pecl-ext/StorageMemcachedModuleTest.php (1)
23-33: LGTM!The refactoring of the
testNewmethod enhances the test's coverage and adopts a more modular approach. The use of aQueryRepositoryModuleand the overriding ofStorageMemcachedModuledemonstrate the flexibility of the system. The injector-based retrieval of instances and the updated assertions ensure that the correct types are being used for resource storage, tag-aware adapters, and Memcached adapters.The changes are approved.
src/StorageMemcachedEtagModule.php (1)
1-49: LGTM!The new
StorageMemcachedEtagModuleclass provides a clean and concise implementation of a Memcached EtagPool module. The constructor allows for easy configuration of the Memcached server(s) and an optional additional module. Theconfiguremethod properly sets up the necessary bindings for using Memcached as the caching mechanism.The code is well-structured and follows best practices. The addition of this module enhances the flexibility and extensibility of the caching system.
The new file is approved.
src-deprecated/StorageRedisMemcachedModule.php (1)
20-21: Deprecation notice approved.The addition of the
@deprecatedannotation to theStorageRedisMemcachedModuleclass is a clear indication that this class should no longer be used. Developers should take note of this deprecation and consider alternative implementations or modules for their caching needs.The deprecation notice suggests a shift in the intended usage of the class and encourages developers to explore other options. It is important to update any existing code that relies on this class to ensure compatibility and maintainability in the future.
The deprecation notice is approved.
src/StorageMemcachedModule.php (3)
7-8: LGTM!The added use statements are approved.
Also applies to: 13-15
32-32: LGTM, but verify the commented out code.The changes to the
AdapterInterfaceandTagAwareAdapterInterfacebindings are approved.However, the
CacheItemPoolInterfacebinding forEtagPoolhas been commented out.Please verify if this functionality is no longer needed and can be safely removed. If it's still required, consider updating it to use the new tag-aware caching approach.
Also applies to: 34-41
43-45: This code segment has already been flagged for verification in a previous review comment.tests/ResourceStorageTest.php (2)
19-41: LGTM!The new
getResourceStorageInstancemethod is approved.The method encapsulates the creation of the
ResourceStorageinstance and uses an anonymous class to manage theTagAwareAdapterinstance, which is a good approach.
43-45: LGTM!The changes to the
setUpmethod are approved.Calling the
getResourceStorageInstancemethod ensures that theResourceStorageinstance is initialized correctly before each test.tests-deprecated/StorageRedisModuleTest.php (1)
18-20: LGTM!The deprecation notice is approved.
The comment clearly indicates that the
StorageRedisModuleTestclass is deprecated and should no longer be used.tests/HttpCacheTest.php (3)
18-19: LGTM!The code changes are approved.
31-31: LGTM!The code changes are approved.
53-53: LGTM!The code changes are approved.
src/StorageRedisDsnModule.php (1)
1-68: LGTM!The new module
StorageRedisDsnModuleis well-structured and follows the BEAR.Sunday module conventions. It provides a flexible way to configure Redis connection settings using a DSN format and installs the necessary dependencies and bindings.The file is approved.
tests-pecl-ext/StorageRedisDsnModuleTest.php (1)
1-64: LGTM!The new test case
StorageRedisDsnModuleTestis well-structured and follows the PHPUnit conventions. It ensures that theStorageRedisDsnModulecorrectly instantiates the required dependencies and starts and stops the Redis server process, which is a good practice for integration tests.The file is approved.
src/QueryRepositoryModule.php (4)
8-9: LGTM!The code changes are approved.
14-17: LGTM!The code changes are approved.
46-59: LGTM!The code changes are approved.
69-69: LGTM!The code changes are approved.
composer.json (2)
20-20: LGTM!The code changes are approved.
21-21: LGTM!The code changes are approved.
tests-deprecated/ResourceStorageCacheableTrait.php (1)
1-89: LGTM!The code is well-structured and follows the SOLID principles. The serialization and unserialization methods are correctly implemented, and the use of
Injectannotation for dependency injection is a good practice.However, the file is marked as deprecated, which indicates that it will be removed in a future version. Ensure that the deprecated code is not used in new code and plan for its removal.
tests/ResourceRepositoryTest.php (4)
13-13: LGTM!The import statement for
Ray\Di\ProviderInterfaceis correctly added.
30-40: LGTM!The code changes to create a
TagAwareAdapterinstance wrapped in an anonymous class implementingProviderInterfaceare correctly implemented. This enhances the caching mechanism by utilizing aTagAwareAdapterand providing it through aProviderInterfaceimplementation.
48-50: LGTM!The code changes to update the
ResourceStorageconstructor to accept the$tagAwareAdapterProvideras arguments for both the resource object pool and the ETag pool are correctly implemented. This aligns with the usage of theTagAwareAdapterandProviderInterfacefor the caching mechanism.
119-129: LGTM!The code changes to create a new instance of
TagAwareAdapterusing aNullAdapterand wrapping it in an anonymous class implementingProviderInterfaceare correctly implemented. This follows the same pattern as the previous code segment, utilizing aTagAwareAdapterand providing it through aProviderInterfaceimplementation.tests/QueryRepositoryTest.php (5)
9-9: LGTM!The import statement for
BEAR\RepositoryModule\Annotation\ResourceObjectPoolis correctly added.
26-27: LGTM!The import statements for
Symfony\Component\Cache\Adapter\TagAwareAdapterandSymfony\Component\Cache\Adapter\TagAwareAdapterInterfaceare correctly added.
160-160: LGTM!The comment update provides a clearer explanation of the error handling behavior by stating that returning
truetells PHP to stop normal error handling.
165-165: LGTM!The code change to update the binding of
TagAwareAdapterInterfaceannotated withResourceObjectPoolto useTagAwareAdapterwithFakeErrorCacheis correctly implemented. This allows testing error scenarios with a specific cache implementation.
216-218: LGTM!The code changes to create a new
Injectorinstance and serialize it are correctly implemented. This is likely done for testing purposes.src/ResourceStorage.php (6)
8-8: LGTM!The import statement for
BEAR\RepositoryModule\Annotation\ResourceObjectPoolis correctly added.
12-13: LGTM!The import statements for
Ray\Di\Di\SetandRay\Di\ProviderInterfaceare correctly added.
14-14: LGTM!The import statement for
Symfony\Component\Cache\Adapter\TagAwareAdapterInterfaceis correctly added.
27-36: LGTM!The
Propstype definition is correctly added. It provides a clear structure for the expected array properties, specifying the keys and their corresponding types.
49-55: LGTM!The new properties
$roPoolProvider,$etagPoolProvider,$roPool, and$etagPoolare correctly added to theResourceStorageclass. The$roPoolProviderand$etagPoolProviderproperties are typed asProviderInterface<TagAwareAdapterInterface>, indicating that they provide instances ofTagAwareAdapterInterface. The$roPooland$etagPoolproperties are typed asTagAwareAdapterInterface, representing the actual cache pool instances.
57-71: LGTM!The constructor of the
ResourceStorageclass is correctly modified to accept the$roPoolProviderand$etagPoolProviderparameters. The#[Set]annotations specify the binding ofTagAwareAdapterInterfaceto the respective pool classes. TheinitializePoolsmethod is called with the provided pool providers to initialize the cache pools.
Removed outdated and commented-out bindings for MemcachedAdapter to improve code clarity and maintainability. This cleanup will also help in preventing potential confusion or future maintenance issues.
Bind the ResourceObjectPool annotation to use the LocalCacheProvider, enhancing caching capabilities. This change aims to improve performance by utilizing local cache mechanisms like APCu or filesystem storage.
Updated a comment to ensure consistent capitalization style across the codebase. This minor change helps maintain code readability and standardization.
|
@coderabbitai review again please. |
|
@koriym: Understood, I will review the changes again as per your request. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (38)
src-annotation/EtagPool.php (1)
10-14: LGTM! Good architectural improvement.
The simplification of EtagPool to a pure marker attribute with parameter-only targeting is a good architectural choice. This change:
- Follows PHP 8+ attribute best practices
- Provides clearer dependency injection semantics
- Aligns with the broader caching strategy refactor
src-annotation/RedisDsn.php (1)
12-14: Consider adding class-level documentation.
Since this class is part of a public API for Redis DSN configuration, consider adding PHPDoc comments to explain:
- The purpose of this attribute
- Example usage with DSN format
- Link to related Symfony documentation
Example documentation:
+/**
+ * Marks a parameter as a Redis DSN configuration string.
+ *
+ * Example usage:
+ * ```php
+ * public function __construct(
+ * #[RedisDsn] string $dsn = 'redis://localhost:6379'
+ * ) {}
+ * ```
+ *
+ * @see https://symfony.com/doc/current/components/cache/adapters/redis_adapter.html#configure-the-connection
+ */
final class RedisDsn
{
}src-annotation/TagsPool.php (1)
10-14: LGTM! Well-designed marker attribute for DI qualification.
The class is correctly implemented as a final marker attribute for dependency injection qualification. The attribute target restriction to parameters and the use of #[Qualifier] makes it suitable for type-safe dependency injection in the caching system.
When using this qualifier:
- Apply it to constructor parameters that need tag-aware cache adapters
- Combine with appropriate interface types (e.g.,
TagAwareAdapterInterface) - Example usage:
public function __construct(
#[TagsPool]
TagAwareAdapterInterface $cache
) {
$this->cache = $cache;
}src-annotation/RedisDsnOptions.php (2)
10-11: Consider adding documentation for the attribute usage.
While the code is technically correct, adding a class-level PHPDoc comment would help other developers understand:
- The purpose of this attribute
- Where and how it should be used
- Example usage in the context of Redis DSN configuration
Consider adding documentation like this:
+/**
+ * Attribute to mark Redis DSN configuration options.
+ *
+ * This attribute is used to qualify Redis DSN configuration parameters
+ * in dependency injection contexts.
+ *
+ * @example
+ * #[Inject]
+ * public function __construct(
+ * #[RedisDsnOptions] private array $options
+ * ) {}
+ */
#[Attribute(Attribute::TARGET_PARAMETER)]
#[Qualifier]
final class RedisDsnOptions12-14: Consider adding type validation for Redis DSN options.
Since this class is used for Redis DSN options, consider adding properties or methods to validate the DSN format and supported options at compile time.
Would you like me to help implement type validation for the Redis DSN options? This could include:
- Properties for common Redis options (host, port, auth, etc.)
- Validation methods to ensure DSN format correctness
- Constants for supported connection types (Redis, RedisArray, RedisCluster, etc.)
src-annotation/ResourceObjectPool.php (1)
10-14: LGTM! Well-designed qualifier class.
The class is properly implemented as a qualifier:
- Correctly marked as
final - Uses PHP 8 attributes for parameter targeting
- Implements the Qualifier pattern for dependency injection
- Empty implementation is appropriate for a marker interface
This aligns well with the PR objectives of enhancing the caching mechanism through proper dependency injection.
Consider adding a class-level PHPDoc comment to document:
- The purpose of this qualifier
- Example usage in dependency injection
- Link to related documentation about the resource object pool pattern
tests-deprecated/StorageApcModuleTest.php (1)
13-15: Enhance the deprecation notice with more details.
The deprecation notice should include additional information to guide users:
- Since version
- Reason for deprecation
- Alternative implementation to use
Consider updating the PHPDoc to:
/**
- * @deprecated
+ * @deprecated since 2.0.0. Use StorageRedisDsnModule or StorageMemcachedEtagModule instead as APC storage is being phased out.
*/.github/dependabot.yml (1)
15-16: LGTM! Consider documenting the rationale.
The addition to ignore major version updates for PHPUnit is a sensible precaution, especially during significant architectural changes. This helps maintain test stability while the new Redis and Memcached modules are being introduced.
Consider adding a comment above this rule to document why major PHPUnit updates are being ignored, which will help future maintainers understand the decision.
- dependency-name: "*" # Ignore all dependencies for specific update types
update-types: ["version-update:semver-minor", "version-update:semver-patch"]
+ # Ignore major PHPUnit updates to maintain test stability during architectural changes
- dependency-name: "phpunit/phpunit"
update-types: ["version-update:semver-major"]tests-pecl-ext/StorageMemcachedEtagModuleTest.php (2)
19-19: Replace example domains with standard test values.
The server string uses example domains that could be misleading. Consider using standard test values like localhost or 127.0.0.1.
- $servers = 'mem1.domain.com:11211:33,mem2.domain.com:11211:67';
+ $servers = 'localhost:11211:33,127.0.0.1:11211:67';18-18: Enhance the documentation reference.
The current comment could be more specific about what aspects of the Memcached server configuration are being referenced.
- // @see http://php.net/manual/en/memcached.addservers.php
+ // Server string format: "<host>:<port>:<weight>,..."
+ // @see https://www.php.net/manual/en/memcached.addservers.php#refsect1-memcached.addservers-parameterssrc/ProdQueryRepositoryModule.php (1)
22-24: Consider documenting the caching hierarchy.
The introduction of local caching alongside the new StorageRedisDsnModule creates a multi-level caching strategy. Consider documenting:
- The caching hierarchy (local vs Redis)
- Cache invalidation strategy across layers
- TTL coordination between layers
- Performance implications and recommendations
src-deprecated/NamespacedCacheProvider.php (3)
13-15: Enhance the deprecation notice with migration guidance.
The deprecation notice should include:
- Reason for deprecation
- Alternative implementation to use
- Target version for removal
Apply this diff to improve the deprecation notice:
/**
- * @deprecated
+ * @deprecated Use Symfony's TagAware cache adapters instead. This class will be removed in version X.Y.0
*/19-24: Remove redundant docblock annotations.
Since PHP 8 attributes are now being used, the docblock annotations are redundant and should be removed.
Apply this diff to clean up the annotations:
- /**
- * @CacheEngine("cache")
- * @AppName("appName")
- * @CacheVersion("version")
- */
#[CacheEngine('cache'), AppName('appName'), CacheVersion('version')]Line range hint 1-43: Consider documenting the migration path to RedisTagAwareAdapter.
Since this class is being deprecated as part of the transition to using Symfony's RedisTagAwareAdapter for native tag management, it would be helpful to:
- Add a code example in the class documentation showing how to migrate to the new architecture
- Update the relevant documentation to explain the benefits of using RedisTagAwareAdapter
Would you like me to help draft the migration documentation?
tests/ModuleFactory.php (1)
Line range hint 1-28: Consider documenting the caching strategy transition.
Since this change is part of a larger transition to Symfony's Cache component and tag-aware caching:
- Consider adding a comment in the test factory explaining why ArrayAdapter is suitable for testing the new tag-aware functionality
- Consider documenting the benefits of moving from PSR-6 CacheItemPoolInterface to Symfony's AdapterInterface in the class docblock
src/RedisDsnProvider.php (1)
7-16: Note: Parameter name 'dns' should be 'dsn'.
The imports are well-organized, but I noticed that the codebase uses 'dns' instead of 'dsn' (Data Source Name). This could be confusing as 'DNS' typically refers to Domain Name System.
This change should be applied throughout the file:
- private string $dns,
+ private string $dsn,src-deprecated/BcModule.php (2)
17-18: Enhance the deprecation notice with migration guidance.
The deprecation notice should include:
- Since which version it's deprecated
- What to use instead (StorageRedisDsnModule/StorageMemcachedEtagModule)
- When it will be removed
Consider updating the docblock like this:
/**
- * @deprecated
+ * @deprecated since 2.0, will be removed in 3.0. Use StorageRedisDsnModule or StorageMemcachedEtagModule instead.
*
* Backward Compatibility moduleModule deprecation requires attention in demo and tests
The verification reveals some remaining dependencies that need attention:
demo/AppModule.phpstill uses the deprecated storage system withArrayCacheandStorage::class- Some test files in
tests-deprecated/directory still rely on the old storage system - The deprecated namespace
BEAR\QueryRepositoryis still heavily imported, but mostly in tests and annotations
🔗 Analysis chain
Line range hint 1-47: Verify the impact of deprecating this module.
The class is being deprecated as part of moving to new storage modules. Let's verify if there are any remaining dependencies on this module.
Let me generate another script to check for module installation with proper escaping and also look for any other potential dependencies:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for remaining dependencies on BcModule
# Expected: No direct usage of BcModule in production code
# Test 1: Search for direct class usage
echo "=== Checking for BcModule usage ==="
rg -l "BcModule" --type php
# Test 2: Search for deprecated interface usage
echo -e "\n=== Checking for DeprecatedHttpCacheInterface usage ==="
rg -l "DeprecatedHttpCacheInterface" --type php
# Test 3: Check for module installation in application configs
echo -e "\n=== Checking for module installation in configs ==="
rg -l "->install\(new BcModule\)" --type php
Length of output: 928
Script:
#!/bin/bash
# Description: Additional checks for BcModule dependencies
# Test 1: Check for module installation with proper escaping
echo "=== Checking for module installation in configs ==="
rg "install.*BcModule" --type php
# Test 2: Check for any Storage::class usage which might indicate old storage system
echo -e "\n=== Checking for Storage annotation usage ==="
rg "Storage::class" --type php
# Test 3: Check for imports of deprecated namespace
echo -e "\n=== Checking for deprecated namespace imports ==="
rg "use BEAR\\\\QueryRepository\\\\" --type php
# Test 4: Check for ArrayCache usage which is part of old system
echo -e "\n=== Checking for ArrayCache usage ==="
rg "ArrayCache" --type php
Length of output: 5872
src-deprecated/StorageRedisModule.php (1)
24-25: Enhance the deprecation notice with version and migration details.
Consider providing more detailed deprecation information to help users migrate smoothly.
- * @deprecated Use StorageRedisDnsModule instead
+ * @deprecated since 2.0.0 Use StorageRedisDnsModule instead. See https://github.com/username/repo/blob/main/docs/redis-dsn.md for migration guide.src/StorageMemcachedModule.php (1)
Line range hint 17-43: Consider documenting the tag-based caching strategy.
This module implements a robust tag-aware caching mechanism using Memcached. To ensure proper usage across the application:
- Document the tagging strategy for cache invalidation
- Consider providing examples of how to use tags effectively
- Add cache invalidation patterns to the documentation
This will help maintain consistency as you transition to tag-aware caching with both Memcached and Redis implementations.
src-deprecated/StorageRedisMemcachedModule.php (2)
20-21: Enhance the deprecation notice with migration guidance.
The deprecation notice should include information about the replacement modules and migration path to help users transition smoothly.
Apply this diff to improve the deprecation notice:
/**
- * @deprecated
+ * @deprecated Use StorageRedisDsnModule for Redis connections (with RedisTagAwareAdapter support)
+ * and StorageMemcachedEtagModule for Etag caching instead.
*
* Bind redis to shared storage, Bind memcache to etag pool
*/Migration guide needed for StorageRedisMemcachedModule deprecation
The deprecated StorageRedisMemcachedModule should be replaced with two separate modules:
-
For Redis (shared storage):
// Old format: new StorageRedisMemcachedModule('localhost:6379', $memcacheServer); // New format: new StorageRedisDsnModule('redis://localhost:6379', [ 'timeout' => 10 // Optional Redis options ]);
-
For Memcached (etag pool):
// Old format: new StorageRedisMemcachedModule($redisServer, 'mem1.domain.com:11211,mem2.domain.com:11211'); // New format: new StorageMemcachedEtagModule('mem1.domain.com:11211,mem2.domain.com:11211');
Key changes:
- Redis connection now uses DSN format with optional configuration array
- Memcached configuration remains the same format but is now in a dedicated module
- The modules need to be installed separately as they serve different purposes
Please add this migration guide to the deprecation notice in StorageRedisMemcachedModule.php.
🔗 Analysis chain
Line range hint 1-65: Consider providing a migration guide for existing users.
Since this module is being deprecated in favor of StorageRedisDsnModule and StorageMemcachedEtagModule, it would be beneficial to:
- Document the equivalent DSN format for the current Redis server string format
- Provide examples of how to migrate from the current server configuration to the new modules
- Consider adding a migration script to help users transition their configurations
This will help ensure a smooth transition for existing users of this module.
Let's check if there are any direct usages of this module that need migration:
Let me gather more information about the new modules and their documentation to provide better migration guidance.
Let me check the content of the README and spec files to see if there's any existing migration documentation, and also look at the specific configuration differences between the old and new modules.
Let me check the specific constructor implementations and configuration options of both the old and new modules to understand the exact differences in their initialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of StorageRedisMemcachedModule
rg -l "StorageRedisMemcachedModule"
# Search for Redis server string configurations that need to be converted to DSN
rg -A 2 "new StorageRedisMemcachedModule\("
Length of output: 571
Script:
#!/bin/bash
# Check for the new modules mentioned in the review
rg -l "StorageRedisDsnModule|StorageMemcachedEtagModule"
# Look for any README or documentation files
fd -e md
# Check the implementation of new modules to understand their configuration
rg -A 5 "class StorageRedisDsnModule|class StorageMemcachedEtagModule"
# Look for any examples or tests using the new modules
rg -A 3 "new StorageRedisDsnModule|new StorageMemcachedEtagModule"
Length of output: 3010
Script:
#!/bin/bash
# Check README and spec content
cat README.md
cat docs/spec.md
# Look at the constructor signatures of both old and new modules
ast-grep --pattern 'class StorageRedisDsnModule {
$$$
public function __construct($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'class StorageMemcachedEtagModule {
$$$
public function __construct($$$) {
$$$
}
$$$
}'
Length of output: 2982
Script:
#!/bin/bash
# Check the implementation of StorageRedisDsnModule
cat src/StorageRedisDsnModule.php
# Check the implementation of StorageMemcachedEtagModule
cat src/StorageMemcachedEtagModule.php
# Look for any usage examples in tests
cat tests-pecl-ext/StorageRedisDsnModuleTest.php
cat tests-pecl-ext/StorageMemcachedEtagModuleTest.php
Length of output: 6676
tests-deprecated/StorageRedisModuleTest.php (3)
18-20: Add a deprecation message with migration instructions.
The @deprecated annotation should include a message explaining why it's deprecated and how to migrate to the new StorageRedisDsnModule.
/**
- * @deprecated
+ * @deprecated Use StorageRedisDsnModule instead. See Symfony/cache DSN format for configuration.
*/Line range hint 52-57: Enhance test coverage beyond type assertions.
The test only verifies the instance type but doesn't test the actual caching functionality. Consider adding tests for:
- Cache operations (get, set, delete)
- Error handling
- Connection failures
Line range hint 59-71: Remove hardcoded test values.
The Memcached server configuration uses hardcoded values (mem1.domain.com:11211:33,mem2.domain.com:11211:67). Consider:
- Moving these to test configuration
- Using environment variables
- Using more realistic test values
tests/HttpCacheTest.php (2)
18-19: Fix typo in method name testisNotModifiedFale
The implementation change to use getResourceStorageInstance() looks good. However, there's a typo in the method name: "Fale" should be "False".
- public function testisNotModifiedFale(): CliHttpCache
+ public function testisNotModifiedFalse(): CliHttpCache53-53: Consider renaming the test method for clarity
The implementation change to use getResourceStorageInstance() looks good. However, consider renaming the method to better describe what it's testing, e.g., testIsNotModifiedWithCliHeaders or testEtagHeaderParsingInCli.
- public function testHeaderSetInCli(): void
+ public function testIsNotModifiedWithCliHeaders(): voidsrc/StorageRedisDsnModule.php (2)
17-36: Complete the class documentation.
The documentation contains a [...] placeholder and some incomplete binding descriptions. Consider adding:
- Complete list of provided bindings
- Configuration examples with DSN format
- Link to Symfony Redis adapter documentation
37-51: Consider adding options validation.
The constructor implementation is solid with proper type hints and immutable properties. Consider adding validation for the options array to ensure only supported Redis options are provided.
Example validation:
public function __construct(
private readonly string $dsn,
private readonly array $options = [],
AbstractModule|null $module = null,
) {
+ $supportedOptions = ['retry_interval', 'timeout', 'read_timeout', 'tcp_keepalive', 'persistent', 'lazy'];
+ $unknownOptions = array_diff(array_keys($options), $supportedOptions);
+ if ($unknownOptions) {
+ throw new \InvalidArgumentException('Unknown Redis options: ' . implode(', ', $unknownOptions));
+ }
parent::__construct($module);
}tests-pecl-ext/StorageRedisDsnModuleTest.php (1)
7-18: Consider organizing imports by category.
For better readability, consider organizing imports into logical groups:
- PSR interfaces
- Framework classes
- Symfony components
- Annotations
-use BEAR\RepositoryModule\Annotation\EtagPool;
-use BEAR\RepositoryModule\Annotation\ResourceObjectPool;
-use PHPUnit\Framework\TestCase;
-use Psr\Cache\CacheItemPoolInterface;
-use Ray\Di\Injector;
-use Ray\PsrCacheModule\Annotation\Shared;
-use Ray\PsrCacheModule\MemcachedAdapter;
-use Ray\PsrCacheModule\RedisAdapter;
-use Symfony\Component\Cache\Adapter\RedisTagAwareAdapter;
-use Symfony\Component\Cache\Adapter\TagAwareAdapter;
-use Symfony\Component\Cache\Adapter\TagAwareAdapterInterface;
-use Symfony\Component\Process\Process;
+// PSR interfaces
+use Psr\Cache\CacheItemPoolInterface;
+
+// Framework
+use PHPUnit\Framework\TestCase;
+use Ray\Di\Injector;
+
+// Symfony components
+use Symfony\Component\Cache\Adapter\RedisTagAwareAdapter;
+use Symfony\Component\Cache\Adapter\TagAwareAdapter;
+use Symfony\Component\Cache\Adapter\TagAwareAdapterInterface;
+use Symfony\Component\Process\Process;
+
+// Cache adapters
+use Ray\PsrCacheModule\MemcachedAdapter;
+use Ray\PsrCacheModule\RedisAdapter;
+
+// Annotations
+use BEAR\RepositoryModule\Annotation\EtagPool;
+use BEAR\RepositoryModule\Annotation\ResourceObjectPool;
+use Ray\PsrCacheModule\Annotation\Shared;tests-deprecated/ResourceStorageCacheableTrait.php (3)
20-23: Add migration instructions to deprecation notice.
While the trait is properly marked as deprecated, it would be helpful to provide migration instructions or alternatives in the deprecation notice to guide users on how to migrate away from this trait.
/**
- * @deprecated
+ * @deprecated Use Symfony's TagAwareAdapter directly instead.
*/31-33: Remove redundant @Inject annotation.
The @Inject annotation is redundant as the #[Inject] attribute is already present. Consider removing the docblock annotation to maintain a single source of truth.
- /** @Inject */
#[Inject]
final public function setInjector(InjectorInterface $injector): void82-83: Refine psalm suppression scope.
The @psalm-suppress all is too broad and might hide potential type-related issues. Consider specifying exactly which psalm rules need to be suppressed.
- /** @psalm-suppress all */
+ /** @psalm-suppress MixedAssignment, PossiblyNullReference */tests/ResourceRepositoryTest.php (2)
Line range hint 91-118: Consider removing unused doctrineCache implementation.
The doctrineCache class appears to be unused after the refactoring to use TagAwareAdapter. Consider removing it if it's no longer needed for the test.
Line range hint 89-144: Consider breaking down the test method.
The testCreateFromDoctrineAnnotation method has grown quite large. Consider:
- Breaking it down into smaller, focused test methods
- Using test data providers
- Moving the setup code to helper methods
Example refactoring:
private function createTagAwareAdapter(): TagAwareAdapter
{
return new TagAwareAdapter(new NullAdapter());
}
private function createRepository(TagAwareAdapter $adapter): Repository
{
$provider = new TagAwareAdapterTestProvider($adapter);
return new Repository(
new RepositoryLogger(),
new HeaderSetter(new EtagSetter(new CacheDependency(new UriTag()))),
new ResourceStorage(
new RepositoryLogger(),
new NullPurger(),
new UriTag(),
new ResourceStorageSaver(),
$provider,
$provider
),
new AnnotationReader(),
new Expiry(0, 0, 0)
);
}
public function testCreateFromDoctrineAnnotation(): void
{
$adapter = $this->createTagAwareAdapter();
$repository = $this->createRepository($adapter);
$this->assertInstanceOf(Repository::class, $repository);
}tests/QueryRepositoryTest.php (1)
165-165: Consider adding tag operation test coverage.
The binding to TagAwareAdapterInterface is correct, but since we're transitioning to tag-aware caching, consider adding specific test cases for tag-based operations (e.g., invalidation by tags).
Would you like me to help create test cases for tag-based cache operations?
src/StorageMemcachedEtagModule.php (2)
31-31: Avoid variable shadowing in closure parameters
In the closure on line 31, the parameter $memcacheServer shadows the outer variable of the same name. This can lead to confusion and potential bugs. Consider renaming the closure parameter to $server for clarity.
Apply this diff to rename the closure parameter:
- $this->memcacheServer = array_map(static fn ($memcacheServer) => explode(':', $memcacheServer), explode(',', $memcacheServer));
+ $this->memcacheServer = array_map(static fn ($server) => explode(':', $server), explode(',', $memcacheServer));28-31: Consider renaming variables for clarity
The variable $memcacheServer represents multiple servers after processing. To enhance readability, consider renaming it to $memcacheServers or $memcacheServerList.
Apply this diff to rename the variable:
public function __construct(
- string $memcacheServer,
+ string $memcacheServers,
AbstractModule|null $module = null,
) {
- $this->memcacheServer = array_map(static fn ($server) => explode(':', $server), explode(',', $memcacheServer));
+ $this->memcacheServer = array_map(static fn ($server) => explode(':', $server), explode(',', $memcacheServers));📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (31)
- .github/dependabot.yml (1 hunks)
- composer.json (1 hunks)
- psalm.xml (1 hunks)
- src-annotation/EtagPool.php (1 hunks)
- src-annotation/RedisDsn.php (1 hunks)
- src-annotation/RedisDsnOptions.php (1 hunks)
- src-annotation/ResourceObjectPool.php (1 hunks)
- src-annotation/TagsPool.php (1 hunks)
- src-deprecated/BcModule.php (1 hunks)
- src-deprecated/NamespacedCacheProvider.php (1 hunks)
- src-deprecated/ResourceStorageCacheableTrait.php (1 hunks)
- src-deprecated/StorageRedisMemcachedModule.php (1 hunks)
- src-deprecated/StorageRedisModule.php (1 hunks)
- src/ProdQueryRepositoryModule.php (2 hunks)
- src/QueryRepositoryModule.php (3 hunks)
- src/RedisDsnProvider.php (1 hunks)
- src/ResourceStorage.php (5 hunks)
- src/StorageMemcachedEtagModule.php (1 hunks)
- src/StorageMemcachedModule.php (2 hunks)
- src/StorageRedisDsnModule.php (1 hunks)
- tests-deprecated/ResourceStorageCacheableTrait.php (1 hunks)
- tests-deprecated/StorageApcModuleTest.php (1 hunks)
- tests-deprecated/StorageRedisModuleTest.php (1 hunks)
- tests-pecl-ext/StorageMemcachedEtagModuleTest.php (1 hunks)
- tests-pecl-ext/StorageMemcachedModuleTest.php (1 hunks)
- tests-pecl-ext/StorageRedisDsnModuleTest.php (1 hunks)
- tests/HttpCacheTest.php (3 hunks)
- tests/ModuleFactory.php (2 hunks)
- tests/QueryRepositoryTest.php (4 hunks)
- tests/ResourceRepositoryTest.php (3 hunks)
- tests/ResourceStorageTest.php (1 hunks)
🔇 Additional comments (56)
src-annotation/RedisDsn.php (1)
1-14: Implementation looks good!
The attribute class is well-structured and follows PHP 8 attribute best practices. The use of final prevents inheritance which is appropriate for an attribute class, and the TARGET_PARAMETER restriction correctly limits its usage to parameters only.
src-annotation/TagsPool.php (2)
1-5: LGTM! Follows PHP best practices.
The file structure adheres to PSR standards with proper strict typing and namespace declaration.
7-8: LGTM! Appropriate imports for attribute-based DI qualifier.
The necessary dependencies are imported for creating a custom qualifier attribute.
src-annotation/RedisDsnOptions.php (1)
1-14: LGTM! Well-structured attribute class following best practices.
The class is well-organized and follows PHP best practices:
- Proper use of strict types
- Correct namespace following PSR-4
- Appropriate use of PHP 8 attributes
- Intentionally empty class serving as a marker interface for DI qualification
src-annotation/ResourceObjectPool.php (2)
1-5: LGTM! Well-structured file header.
The file follows PSR-12 coding standards with proper PHP opening tag, strict types declaration, and namespace definition.
7-8: LGTM! Appropriate use statements.
The imports are correctly specified and necessary for the implementation.
tests-deprecated/StorageApcModuleTest.php (1)
13-15: Verify consistent deprecation strategy across codebase.
Let's ensure all deprecated components follow the same deprecation pattern.
✅ Verification successful
Deprecation strategy is consistently implemented across the codebase
The codebase follows a clear and consistent deprecation approach:
- All deprecated code is properly marked with
@deprecatedPHPDoc tags - Files are organized in dedicated
-deprecateddirectories (src-deprecated,tests-deprecated,src-annotation-deprecated) - Where applicable, replacement suggestions are provided (e.g., "Use StorageRedisDnsModule instead", "Use \Ray\PsrCacheModule\Psr6ApcuModule")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check deprecation notices across the codebase
# Expected: Consistent deprecation message format across all deprecated files
# Check all PHP files for @deprecated tags and their formats
rg -t php "@deprecated" -A 2
# Check if other files have been moved to deprecated directories
fd -t f . | rg "deprecated"
Length of output: 3726
psalm.xml (1)
17-23: LGTM! Well-scoped error suppression.
The addition of this error handler is appropriate as it:
- Targets only undefined class errors
- Limits the suppression to a single file (RedisDsnProvider.php)
- Aligns with the PR's objective of supporting multiple Redis connection types that may only be available at runtime
tests-pecl-ext/StorageMemcachedEtagModuleTest.php (1)
20-21: LGTM! The core test logic is correct.
The test correctly validates that:
- The DI container is properly configured
- The correct adapter type is instantiated
src/ProdQueryRepositoryModule.php (3)
8-8: LGTM: New imports support the caching infrastructure changes.
The added imports align with the PR's objective of enhancing the caching mechanism:
ResourceObjectPoolannotation for cache identificationLocalCacheProviderfor local caching implementation
Also applies to: 10-10
24-24: LGTM: Retained performance optimization for symfony/cache.
The existing performance optimization for symfony/cache is maintained with the KnownTagTtl setting.
22-23: Verify the caching hierarchy with the new Redis implementation.
The binding configuration introduces local caching for ResourceObjectPool, but we should verify how this interacts with the new StorageRedisDsnModule mentioned in the PR objectives.
Let's check the caching hierarchy implementation:
✅ Verification successful
Let me gather more specific information about the caching implementation and hierarchy.
Let me check one more aspect to understand the complete caching hierarchy.
The caching hierarchy is correctly implemented with Redis and local cache layers
The verification shows a proper implementation of a two-tier caching system:
StorageRedisDsnModuleconfigures Redis as the main storage usingRedisTagAwareAdapterProdQueryRepositoryModuleadds a local cache layer (APCu or File) throughLocalCacheProvider- The binding annotations ensure proper separation and interaction between the cache layers
The implementation aligns with the PR objectives, providing both distributed caching (Redis) and local caching capabilities for optimal performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the caching hierarchy and dependencies
# Expected: Find related cache adapter configurations and their interaction
# Search for cache adapter configurations
echo "=== Cache Adapter Configurations ==="
rg -l "StorageRedisDsnModule|StorageRedisModule|LocalCacheProvider|ResourceObjectPool" --type php
# Search for specific cache binding patterns
echo -e "\n=== Cache Binding Patterns ==="
ast-grep --pattern 'bind($$$)->annotatedWith(ResourceObjectPool::class)'
Length of output: 785
Script:
#!/bin/bash
# Check the implementation details of StorageRedisDsnModule and LocalCacheProvider
echo "=== Redis DSN Module Implementation ==="
rg -A 10 "class StorageRedisDsnModule" src/StorageRedisDsnModule.php
echo -e "\n=== Cache Provider Implementation ==="
rg -A 10 "class LocalCacheProvider" src/
echo -e "\n=== Resource Object Pool Usage ==="
rg -A 5 "ResourceObjectPool" src/ResourceStorage.php
echo -e "\n=== Query Repository Module Bindings ==="
rg -A 10 "class ProdQueryRepositoryModule" src/ProdQueryRepositoryModule.php
Length of output: 1967
Script:
#!/bin/bash
# Check how LocalCacheProvider and Redis are configured in the modules
echo "=== Module Configuration Details ==="
rg -A 15 "protected function configure" src/StorageRedisDsnModule.php
echo -e "\n=== Resource Storage Implementation ==="
rg -A 10 "class ResourceStorage" src/ResourceStorage.php
echo -e "\n=== Query Repository Implementation ==="
rg -B 5 -A 15 "LocalCacheProvider" src/ --type php
Length of output: 2998
tests/ModuleFactory.php (2)
8-8: LGTM: Import changes align with the new caching strategy.
The new imports reflect the architectural shift from PSR-6 CacheItemPool to Symfony's Cache Adapter, supporting the PR's objective of enhanced tag management.
Also applies to: 11-11
22-23: Verify test coverage for new cache adapter bindings.
The bindings look correct, using ArrayAdapter for both ResourceObjectPool and EtagPool in the test environment. However, we should ensure these test bindings properly exercise the new caching behavior.
Let's verify the test coverage:
✅ Verification successful
Let me gather more information about the test coverage by checking the actual test cases.
Based on the test coverage analysis, I can now generate the final response:
Test coverage for cache adapter bindings is sufficient
The codebase has comprehensive test coverage for the cache adapter bindings:
- Tests verify cache behavior with both ResourceObjectPool and EtagPool
- Multiple test cases cover cache operations, etag handling, and cache control headers
- Tests include error cases (FakeErrorCache) and various cache scenarios
- Both query and command interceptors are tested with the cache adapters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are corresponding test cases for the new adapter bindings
# Test 1: Look for test cases using ResourceObjectPool
rg -l "ResourceObjectPool" "tests/"
# Test 2: Look for test assertions related to cache adapter behavior
rg -A 5 "ArrayAdapter|AdapterInterface" "tests/"
Length of output: 3130
Script:
#!/bin/bash
# Look for test methods and assertions related to caching behavior
rg -A 10 "function test.*Cache|function test.*Etag" tests/
Length of output: 15737
src/RedisDsnProvider.php (4)
1-6: LGTM!
The file structure follows PSR standards with proper strict type declaration.
17-19: LGTM!
The class is well-defined with proper type safety through generics and final keyword.
1-36: Verify module binding configuration.
Let's ensure this provider is properly bound in the new StorageRedisDsnModule.
✅ Verification successful
Let me try a different search strategy to locate the module binding configuration.
Let me check the complete StorageRedisDsnModule implementation to verify the binding configuration.
Module binding configuration is properly implemented
The RedisDsnProvider is correctly bound in StorageRedisDsnModule with:
- Proper binding of DSN and options via annotations
- Provider binding for Redis connection
- Integration with
RedisTagAwareAdapterfor resource object pool
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the provider binding in StorageRedisDsnModule
# Expected: Find the binding configuration using RedisDsnProvider
echo "Searching for StorageRedisDsnModule binding configuration..."
ast-grep --pattern 'class StorageRedisDsnModule
{
$$$
bind($$$)->to(RedisDsnProvider::class)
$$$
}'
Length of output: 238
Script:
#!/bin/bash
# Search for any file containing StorageRedisDsnModule
echo "Searching for StorageRedisDsnModule file..."
fd StorageRedisDsnModule
# Search for Redis-related bindings in PHP files
echo -e "\nSearching for Redis bindings..."
rg -t php "bind.*Redis.*Provider" -A 3
Length of output: 818
Script:
#!/bin/bash
# Get the complete StorageRedisDsnModule implementation
echo "Content of StorageRedisDsnModule.php:"
cat src/StorageRedisDsnModule.php
Length of output: 2264
20-30: Verify custom attribute definitions.
The constructor uses custom attributes #[RedisDsn] and #[RedisDsnOptions]. Let's ensure these are properly defined.
✅ Verification successful
Let me try a different search approach to locate these attribute definitions.
Let's examine the content of these attribute definition files to verify their proper implementation.
Custom attributes are properly defined and implemented
Both RedisDsn and RedisDsnOptions attributes are correctly defined in their respective files:
- They are properly decorated with
#[Attribute]targeting parameters - They are marked as qualifiers with
#[Qualifier] - They are correctly namespaced under
BEAR\RepositoryModule\Annotation - Both are implemented as final classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the attribute class definitions exist
# Expected: Find the attribute class definitions
# Search for RedisDsn attribute definition
echo "Searching for RedisDsn attribute definition..."
ast-grep --pattern 'class RedisDsn
{
$$$
}'
# Search for RedisDsnOptions attribute definition
echo "Searching for RedisDsnOptions attribute definition..."
ast-grep --pattern 'class RedisDsnOptions
{
$$$
}'
Length of output: 322
Script:
#!/bin/bash
# Search for attribute definitions using ripgrep for more flexibility
echo "Searching for RedisDsn attribute..."
rg -l "class RedisDsn" --type php
echo -e "\nSearching for RedisDsnOptions attribute..."
rg -l "class RedisDsnOptions" --type php
echo -e "\nSearching for attribute declarations with context..."
rg "#\[Attribute\].*class Redis(Dsn|DsnOptions)" -A 5 --type php
Length of output: 571
Script:
#!/bin/bash
echo "Content of src-annotation/RedisDsn.php:"
cat src-annotation/RedisDsn.php
echo -e "\nContent of src-annotation/RedisDsnOptions.php:"
cat src-annotation/RedisDsnOptions.php
Length of output: 677
src/StorageMemcachedModule.php (3)
7-8: LGTM: Clean import additions for tag-aware caching.
The new imports properly support the transition to tag-aware caching functionality.
Also applies to: 13-15
32-32: LGTM: Clean binding configuration.
The AdapterInterface binding is properly configured with the ResourceObjectPool annotation.
34-41: LGTM with a verification suggestion.
The TagAwareAdapter configuration is well-structured with proper dependency injection setup.
Please verify that the CacheNamespace parameter is properly configured in your application. Run this to check the namespace configuration:
✅ Verification successful
CacheNamespace configuration is properly implemented
The CacheNamespace parameter is correctly configured through the CacheVersionModule which installs CacheNamespaceModule with a version parameter. This is verified by:
- CacheNamespace is properly imported from Ray\PsrCacheModule\Annotation\CacheNamespace
- CacheVersionModule installs CacheNamespaceModule with version configuration
- The configuration is consistently used across all cache storage modules (Redis, Memcached)
- Unit tests verify the namespace configuration (CacheVersionModuleTest)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CacheNamespace configuration
rg -l "bind.*CacheNamespace::class" --type php
Length of output: 48
Script:
#!/bin/bash
# Let's try a broader search for CacheNamespace class and its usage
rg "CacheNamespace" --type php -A 3
# Also search for any cache namespace related configuration
rg "cache.*namespace" -i --type php
Length of output: 6201
tests-pecl-ext/StorageMemcachedModuleTest.php (2)
7-8: LGTM: New imports align with architectural changes.
The added imports support the transition to tag-aware caching and resource pooling, which aligns with the PR objectives.
Also applies to: 14-15
23-33: LGTM: Comprehensive test coverage of the new architecture.
The test effectively validates:
- Resource storage initialization
- Tag-aware adapter configuration
- Resource and Etag pool setup with Memcached
tests/ResourceStorageTest.php (3)
10-12: LGTM: Required imports added correctly.
The new imports support the shift to provider-based architecture with tag-aware caching.
Line range hint 43-49: LGTM: Clean test setup implementation.
The setup method properly initializes both the storage and test resource object.
33-41: Consider using different providers for testing isolation.
Using the same provider instance for both resource object and ETag pools might mask potential issues. Consider creating separate providers for more thorough testing.
tests-deprecated/StorageRedisModuleTest.php (1)
Line range hint 27-33: Consider improving the test infrastructure.
The current setup has several potential issues:
- Starting a real Redis server in tests can be unreliable and may fail in CI environments
- The hard-coded sleep could lead to flaky tests
- No cleanup of Redis data between tests
Consider using a Redis mock or test container for more reliable testing.
Let's check if other test files use similar patterns:
✅ Verification successful
Review comment verified - issues confirmed and class is already deprecated
The review comment's concerns are valid, and importantly, this test class is already marked as @deprecated. The verification shows:
- This is the only Redis test file using this pattern (
Process(['redis-server'])) - It does use a hard-coded sleep (
usleep(1000000)) as mentioned - While there is a
tearDownthat stops the Redis server, there's no explicit data cleanup between tests
Since this class is already deprecated, the architectural improvements suggested would be better applied to the new test implementation that replaces this deprecated one.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar Redis server process usage in tests
rg -l "Process\(\['redis-server'\]\)" "tests*/"
# Search for similar sleep patterns in tests
rg "usleep|sleep" "tests*/"
Length of output: 181
Script:
#!/bin/bash
# Search for similar Redis server process usage in tests with correct path
rg -l "Process\(\['redis-server'\]\)" tests-deprecated/
# Search for similar sleep patterns in tests with correct path
rg "usleep|sleep" tests-deprecated/
# Let's also check for other Redis-related test files
fd "Redis.*Test" tests-deprecated/
# And check the content of the specific test file to see cleanup patterns
cat tests-deprecated/StorageRedisModuleTest.php
Length of output: 2474
tests/HttpCacheTest.php (1)
31-31: LGTM!
The change to use getResourceStorageInstance() is consistent with the refactoring pattern and properly integrates with the ETag caching test scenario.
src/StorageRedisDsnModule.php (2)
1-16: LGTM! Well-structured file header with appropriate imports.
The file follows PSR-12 standards with proper namespace declaration and well-organized use statements.
1-68: Verify the module integration.
Let's verify the integration with related components and usage patterns.
✅ Verification successful
Module integration is properly implemented and tested
The verification shows a well-integrated system with:
- RedisDsnProvider correctly implements the provider interface and handles Redis connection creation
- Proper test coverage with StorageRedisDsnModuleTest that verifies:
- Redis server connection
- Dependency injection
- Expected adapter instances
- Complete annotation definitions for Redis DSN and options
- Correct dependency chain between StorageRedisDsnModule, RedisDsnProvider, and Redis annotations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify module integration and dependencies
# Check for RedisDsnProvider implementation
echo "Checking RedisDsnProvider implementation..."
rg -l "class RedisDsnProvider"
# Check for module usage in application
echo "Checking module usage..."
rg -l "StorageRedisDsnModule"
# Verify Redis annotations
echo "Checking Redis annotations..."
rg -l "RedisDsn|RedisDsnOptions"
# Check for test coverage
echo "Checking test coverage..."
fd -g "*Test.php" | rg "StorageRedisDsn"
Length of output: 742
Script:
#!/bin/bash
# Let's examine the RedisDsnProvider implementation and test coverage
# Check RedisDsnProvider implementation
echo "=== RedisDsnProvider Implementation ==="
cat src/RedisDsnProvider.php
# Check test implementation
echo -e "\n=== Test Implementation ==="
cat tests-pecl-ext/StorageRedisDsnModuleTest.php
# Check Redis annotation definitions
echo -e "\n=== Redis Annotations ==="
cat src-annotation/RedisDsn.php
cat src-annotation/RedisDsnOptions.php
Length of output: 3971
src-deprecated/ResourceStorageCacheableTrait.php (1)
Line range hint 24-99: Verify impact on dependent code and serialization compatibility.
Since this trait handles critical serialization logic, please ensure:
- All implementations using this trait have clear migration paths
- The new implementation maintains serialization format compatibility
- Existing cached data remains accessible during migration
Let's verify the usage and potential impact:
✅ Verification successful
Let me gather more information about the new implementation to understand the migration path and compatibility:
Based on the analysis of both implementations, I can now provide the final response:
The deprecation of ResourceStorageCacheableTrait appears safe to proceed
The verification shows:
- No active usage of the trait in the codebase (no matches for
use.*ResourceStorageCacheableTrait) - The new
ResourceStorageclass insrc/ResourceStorage.phpprovides a complete replacement with:- Compatible serialization format (similar properties being serialized)
- Improved type safety and null handling
- Direct dependency injection instead of trait-based injection
The trait is already properly marked as deprecated (in src-deprecated/), and there are no dependent implementations that need migration paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all classes using this trait
echo "Classes using ResourceStorageCacheableTrait:"
rg -l "use.*ResourceStorageCacheableTrait"
# Find potential serialized data format dependencies
echo -e "\nSerialization format dependencies:"
rg -A 2 "__serialize|__unserialize"
Length of output: 1577
Script:
#!/bin/bash
# Check the new ResourceStorage implementation
echo "New ResourceStorage implementation:"
cat src/ResourceStorage.php
# Look for any migration guides or documentation
echo -e "\nMigration documentation:"
fd -e md -e txt | xargs rg -l "migrat|deprecat"
Length of output: 9298
composer.json (1)
20-21: Verify compatibility with updated dependencies.
The dependency updates to ray/aop (^2.16) and ray/di (^2.17.2) align with the PR objectives for introducing new storage modules. While these are minor version bumps that should maintain backward compatibility, the significant version jumps warrant verification.
Let's check the changelogs for any breaking changes:
✅ Verification successful
Let me check the content of these README files to look for any breaking changes or compatibility notes.
Based on the README contents, I can now generate the final response since I have enough information about the compatibility aspects of these dependencies.
Dependencies update is safe to proceed
The version updates for ray/aop (^2.16) and ray/di (^2.17.2) are safe as:
- Both packages maintain backward compatibility within the 2.x series
- Ray.Aop's README shows no breaking changes in the core functionality
- The main features (method interception, matchers, interceptors) remain consistent
- Ray.Di maintains the same Guice-inspired architecture throughout 2.x
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in the dependency updates
# Test: Look for UPGRADE.md, CHANGELOG.md, or README.md files in vendor directories
echo "Checking for upgrade notes and changelogs..."
fd -t f "UPGRADE.md|CHANGELOG.md|README.md" -d 2 vendor/ray/aop vendor/ray/di
Length of output: 227
Script:
#!/bin/bash
# Check README contents for version compatibility information
echo "=== ray/aop README.md ==="
cat vendor/ray/aop/README.md
echo -e "\n=== ray/di README.md ==="
cat vendor/ray/di/README.md
# Also let's check if there are any Git tags that might indicate version changes
echo -e "\n=== Git tags for ray/aop ==="
cd vendor/ray/aop && git tag -l "2.*" || echo "No git info available"
echo -e "\n=== Git tags for ray/di ==="
cd ../di && git tag -l "2.*" || echo "No git info available"
Length of output: 11205
tests-deprecated/ResourceStorageCacheableTrait.php (2)
58-61: Reconsider code coverage ignore markers.
The error handling path is marked as ignored for code coverage. Consider adding tests for this error scenario to ensure proper exception handling.
38-51: Verify serialization of complex objects.
The serialization includes complex objects like loggers and injectors which might contain circular references or unserializable resources. Consider implementing a test that verifies the serialization of these objects works as expected.
✅ Verification successful
Serialization implementation is safe and verified by existing tests
The codebase already has serialization tests in place that verify the serialization of similar components:
QueryRepositoryTest.phptests multiple serialization/unserialization cyclesDonutCommandRedisCacheTest.phpverifies serialization of cache components- All dependency classes (
UriTag,ResourceStorageSaver,RepositoryLogger,NullPurger,FastlyCachePurger) are simple implementations without complex object graphs
The original concern about circular references is mitigated by the existing test coverage and the straightforward nature of the dependency implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing serialization tests
rg -l "serialize.*ResourceStorageCacheableTrait" tests/
Length of output: 57
Script:
#!/bin/bash
# Search for any serialization tests in the codebase
rg -l "serialize.*" tests/
# Search for test files related to ResourceStorageCacheableTrait
fd "ResourceStorageCacheableTrait.*Test" tests/
# Check if the trait's dependencies implement Serializable or have __serialize methods
ast-grep --pattern 'class $_ implements $$$Serializable$$$'
ast-grep --pattern 'function __serialize'
Length of output: 251
Script:
#!/bin/bash
# Let's check the actual implementations of the dependencies
rg "class.*(?:RepositoryLoggerInterface|PurgerInterface|UriTagInterface|ResourceStorageSaver|InjectorInterface)" -A 5
# Check the content of existing serialization tests to understand patterns
rg "serialize" tests/QueryRepositoryTest.php -B 2 -A 2
rg "serialize" tests/DonutCommandRedisCacheTest.php -B 2 -A 2
# Look for any unserialize implementations
rg "__unserialize.*ResourceStorageCacheableTrait" -A 5
Length of output: 2896
tests/ResourceRepositoryTest.php (2)
13-16: LGTM: Required imports added for new functionality.
The new imports are necessary for implementing the tag-aware caching functionality using Symfony's Cache components.
30-30: Verify TMP_DIR environment variable availability.
The code assumes $_ENV['TMP_DIR'] is always available. Consider:
- Adding a fallback value
- Documenting the requirement in the test setup instructions
✅ Verification successful
Let me check if TMP_DIR is defined elsewhere in the codebase, particularly in test configuration files or setup.
Let me check the bootstrap.php file since it appeared in the search results and is typically where test environment setup occurs.
TMP_DIR is properly configured in test bootstrap
The environment variable TMP_DIR is automatically set in tests/bootstrap.php to __DIR__ . '/tmp', which creates a temporary directory within the tests folder. The code also includes cleanup functionality to ensure the directory is empty before tests run. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if TMP_DIR is documented in test setup
rg -l "TMP_DIR" "README*" "CONTRIBUTING*" "docs/" || echo "Warning: TMP_DIR not documented"
Length of output: 222
Script:
#!/bin/bash
# Check for TMP_DIR in PHP files, focusing on test setup and configuration
rg -l "TMP_DIR" --type php
# Check for .env files that might contain TMP_DIR
fd -e env -x cat {}
# Look for PHPUnit configuration files
fd -e xml -x cat {}
Length of output: 8129
Script:
#!/bin/bash
# Check the content of bootstrap.php for TMP_DIR setup
cat tests/bootstrap.php
Length of output: 586
tests/QueryRepositoryTest.php (2)
9-9: LGTM! New imports align with tag-aware caching strategy.
The added imports support the transition to tag-aware caching, which is consistent with the PR's objective of implementing native tag management.
Also applies to: 26-27
160-160: LGTM! Proper error handling control.
The explicit return value ensures that PHP's default error handler is suppressed, which is essential for testing cache component error scenarios.
src/StorageMemcachedEtagModule.php (2)
25-25: Confirm PHP version compatibility for 'readonly' properties
The use of the readonly property modifier on line 25 requires PHP 8.1 or higher. Please ensure that the project's minimum PHP version supports this feature to avoid compatibility issues.
41-44: Verify 'clientProvider' configuration in binding
In the binding configuration on lines 41-44, the 'clientProvider' parameter is set to the string 'memcached'. Please verify that this value correctly references the intended provider and that the dependency injection container will resolve it as expected.
src/QueryRepositoryModule.php (8)
8-9: LGTM!
The added use statements for ResourceObjectPool and TagsPool correctly import the necessary annotations.
14-14: LGTM!
The addition of the AdapterInterface use statement is appropriate.
16-17: LGTM!
The added use statements for TagAwareAdapter and TagAwareAdapterInterface correctly import the necessary classes.
46-47: Confirm usage of NullAdapter for ResourceObjectPool
Binding AdapterInterface annotated with ResourceObjectPool to NullAdapter effectively disables caching for resource objects. Please confirm that this is the intended behavior.
49-49: Ensure correct binding of TagsPool
Binding AdapterInterface annotated with TagsPool to null will cause the TagAwareAdapter to use the same adapter as ResourceObjectPool. Confirm that this is the desired behavior.
51-57: Validate TagAwareAdapterInterface constructor binding
The binding of TagAwareAdapterInterface annotated with ResourceObjectPool to the constructor of TagAwareAdapter with parameters itemsPool and tagsPool appears correct. Ensure that ResourceObjectPool and TagsPool are properly bound and will resolve to the expected instances.
58-59: Confirm binding of TagAwareAdapterInterface with EtagPool
Binding TagAwareAdapterInterface annotated with EtagPool to null means it will use the same adapter as ResourceObjectPool. Please confirm if this is intended.
69-69: Verify binding of ResourceStorageSaver
The binding for ResourceStorageSaver does not specify an implementation or scope. If ResourceStorageSaver is a concrete class without dependencies, this is acceptable. Otherwise, consider specifying the implementation or required constructor parameters.
src/ResourceStorage.php (8)
27-36: Adding Props type definition enhances static analysis
The introduction of the @psalm-type Props annotation provides a clear definition of the array structure used in serialization. This improves code readability and aids static analysis tools in type-checking.
49-55: Proper declaration of new private properties with type annotations
The new properties $roPoolProvider, $etagPoolProvider, $roPool, and $etagPool are correctly declared with appropriate type hints. This ensures type safety and clarity about the expected types of these properties.
57-60: Accurate parameter type annotations in constructor docblock
The updated docblock for the constructor parameters $roPoolProvider and $etagPoolProvider specifies their types as ProviderInterface<TagAwareAdapterInterface>. This enhances documentation and supports better static analysis.
65-71: Enhanced constructor with dependency injection attributes
The constructor now accepts ResourceStorageSaver, $roPoolProvider, and $etagPoolProvider, utilizing the #[Set] attribute for dependency injection. Assigning these parameters and initializing the pools improves the modularity and testability of the class.
74-84: Modular initialization of pools using initializePools method
The new initializePools method encapsulates the logic for setting up the resource object and ETag pools. This separation of concerns enhances code maintainability and aligns with best practices.
84-84: Review usage of static analysis suppression
The line // @phpstan-ignore-line suppresses potential static analysis warnings. It's advisable to verify whether this suppression is necessary. If possible, address the root cause to maintain code quality.
Consider checking the specific warning being suppressed and evaluating if code adjustments can eliminate the need for suppression.
184-185: Assertion added to verify successful save operation
Adding assert($result, 'Donut save failed.'); ensures that the save operation for donuts is successful. This proactive check improves reliability by catching failures early in the execution flow.
263-287: Implementation of __serialize and __unserialize methods for custom serialization
The __serialize and __unserialize methods have been implemented to control the serialization process of the ResourceStorage object. By serializing the essential properties, including the providers, and reinitializing the pools upon unserialization, the object's integrity is maintained.
Ensure that re-initializing the pools using the providers after unserialization correctly restores the object's state and that no vital state is lost in the process. Testing the serialization and deserialization flow can confirm that the object behaves as expected.
A new test case `testInvalidServerString` was added to ensure that passing an invalid server string to `StorageMemcachedEtagModule` triggers a `CacheException`. This change helps validate proper error handling for incorrect configurations.
Corrects the typo in the deprecation notice of `StorageRedisModule`. The new message now accurately references `StorageRedisDsnModule` instead of the incorrect `StorageRedisDnsModule`.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai Approved? |
|
@koriym: Yes, I have reviewed the changes, and they look good. The PR is approved. ✅ Actions performedComments resolved and changes approved. |
StorageRedisModuleに変わってStorageRedisDsnModuleを提供StorageRedisDsnModuleではタグ管理をアダプターがネイティブで行うRedisTagAwareAdapterを使用。StorageMemcachedEtagModuleを提供StorageRedisModuleはdeprecatedにStorageRedisDsnModule
Redisの接続設定をsymfony/cacheのDSNを使って行います。
上記は下記と同じ
Redis, RedisArray, RedisCluster, Relay or Predis と多様な接続と設定に対応することが可能です。
詳細:
https://symfony.com/doc/current/components/cache/adapters/redis_adapter.html
StorageMemcachedEtagModule
EtagのキャッシュエンジンをMemcachedにするモジュールです。
DSN詳細
symfony/cacheのマニュアルを抽出してマニュアルに加える予定です。
https://github.com/bearsunday/bearsunday.github.io/blob/redis-dsn/manuals/1.0/ja/186.redis-dsn.md