Set initialWaitTtl = 0; add $waitTtl param to withTtl() #6
Conversation
WalkthroughThe changes update the version constraint for the "vimeo/psalm" dependency in Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/RoadRunnerStoreTest.php (2)
29-42: Consider updating tests to include wait TTL parameters.The existing
testSaveSuccessandtestSaveReadSuccesstests should be updated to verify that the wait TTL parameter is correctly passed to the lock methods, similar to how it's tested intestWaitAndSaveSuccess.Consider updating these tests to use the full method signature:
$this->rrLock->expects(self::once()) ->method('lock') - ->with('resource-name', 'random-id') + ->with('resource-name', 'random-id', 300.0, 0) ->willReturn('lock-id');$this->rrLock->expects(self::once()) ->method('lockRead') - ->with('resource-name', 'random-id') + ->with('resource-name', 'random-id', 300.0, 0) ->willReturn('lock-id');Also applies to: 44-54
95-107: Consider updating failure tests to include wait TTL parameters.Similar to the success tests, the failure test cases should also verify the complete method signature including the wait TTL parameter.
Update the test expectations to include all parameters:
$this->rrLock->expects(self::once()) ->method('lock') - ->with('resource-name', 'random-id') + ->with('resource-name', 'random-id', 300.0, 0) ->willReturn(false);$this->rrLock->expects(self::once()) ->method('lockRead') - ->with('resource-name', 'random-id'); + ->with('resource-name', 'random-id', 300.0, 0) + ->willReturn(false);Also applies to: 109-121
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
composer.json(1 hunks)src/RoadRunnerStore.php(3 hunks)tests/RoadRunnerStoreTest.php(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: static analysis
src/RoadRunnerStore.php
[error] 39-39: MissingOverrideAttribute: Method Spiral\RoadRunner\Symfony\Lock\RoadRunnerStore::save should have the "Override" attribute
[error] 61-61: MissingOverrideAttribute: Method Spiral\RoadRunner\Symfony\Lock\RoadRunnerStore::saveread should have the "Override" attribute
[error] 77-77: MissingOverrideAttribute: Method Spiral\RoadRunner\Symfony\Lock\RoadRunnerStore::exists should have the "Override" attribute
[error] 89-89: MissingOverrideAttribute: Method Spiral\RoadRunner\Symfony\Lock\RoadRunnerStore::putoffexpiration should have the "Override" attribute
[error] 104-104: MissingOverrideAttribute: Method Spiral\RoadRunner\Symfony\Lock\RoadRunnerStore::delete should have the "Override" attribute
[error] 114-114: MissingOverrideAttribute: Method Spiral\RoadRunner\Symfony\Lock\RoadRunnerStore::waitandsave should have the "Override" attribute
🔇 Additional comments (8)
composer.json (1)
37-37:❓ Verification inconclusive
Verify Psalm 6.0 compatibility and potential breaking changes.
The expansion to include Psalm 6.0 should be verified to ensure there are no breaking changes in static analysis behavior that could affect the codebase.
🌐 Web query:
What are the breaking changes between Psalm 5.9 and Psalm 6.0?💡 Result:
Psalm 6.0 introduces several breaking changes compared to Psalm 5.9. Key changes include:
PHP Version Support: Psalm 6.0 requires PHP 8.1 or higher. (github.com)
Amp Library Update: The
amphp/amplibrary has been updated to version 3.0. (github.com)PHP-Parser Upgrade: Psalm now uses
nikic/php-parserversion 5.0. (github.com)Dictionaries Overhaul: The dictionaries have been completely overhauled, providing more accurate and up-to-date signatures for PHP and extension functions and methods. (newreleases.io)
Default Configuration Changes:
- The
ignoreInternalFunctionFalseReturnandignoreInternalFunctionNullReturnsettings are nowfalseby default. This means Psalm will warn when a native function's false or null return type is not explicitly checked. (newreleases.io)Codebase Modernization: The codebase has been modernized for higher performance. (newreleases.io)
For a comprehensive list of breaking changes, refer to the Psalm 6.0.0 release notes.
Citations:
- 1: vimeo/psalm#10701
- 2: vimeo/psalm#10701
- 3: vimeo/psalm#10701
- 4: https://newreleases.io/project/github/vimeo/psalm/release/6.0.0?utm_source=openai
- 5: https://newreleases.io/project/github/vimeo/psalm/release/6.0.0?utm_source=openai
- 6: https://newreleases.io/project/github/vimeo/psalm/release/6.0.0?utm_source=openai
Verify project compatibility with Psalm 6.0 breaking changes
Before merging the expanded constraint (
"vimeo/psalm": "^5.9 || ^6.0"), please confirm the codebase and CI environment are ready for Psalm 6.0’s known breaking changes:
- PHP version: Psalm 6.0 requires PHP 8.1 or higher. Ensure your
composer.jsonand deployment targets are updated accordingly.- amphp/amp 3.0 upgrade: Review any async code that depends on Amp’s API for compatibility with v3.
- nikic/php-parser 5.0: Verify that any custom plugins or tooling consuming php-parser still work as expected.
- Default config changes: Psalm now warns on unchecked
falseornullreturns by default (ignoreInternalFunctionFalseReturnandignoreInternalFunctionNullReturnarefalse). Update yourpsalm.xmlif you need to maintain previous behavior.- Dictionaries overhaul & performance tweaks: Run a full Psalm 6.0 analysis and address any new issues or altered type checks.
src/RoadRunnerStore.php (5)
28-28: Significant behavior change - verify this aligns with intended usage.Changing the default
initialWaitTtlfrom 60 seconds to 0 seconds fundamentally changes the lock acquisition behavior. With 0, lock acquisition will fail immediately if the lock is not available, rather than waiting up to 60 seconds.Ensure this aligns with the intended usage patterns and that consumers of this library are aware of this breaking change.
34-36: Good enhancement to make wait TTL configurable.The updated
withTtlmethod signature now allows consumers to specify both the lock TTL and wait TTL, providing more flexibility. The implementation correctly uses the providedwaitTtlparameter instead of the instance'sinitialWaitTtl.
49-49: Consistent implementation of wait TTL parameter.The lock method call now correctly includes the
initialWaitTtlas the fourth parameter, matching the expected interface signature.
68-68: Consistent implementation of wait TTL parameter for read locks.The lockRead method call now correctly includes the
initialWaitTtlas the fourth parameter, maintaining consistency with the lock method.
39-39:❓ Verification inconclusive
Address missing Override attributes flagged by static analysis.
The pipeline failures indicate that several interface methods are missing
#[Override]attributes. While not part of the current changes, these should be addressed to comply with static analysis requirements.Also applies to: 61-61, 77-77, 89-89, 104-104, 114-114
🏁 Script executed:
#!/bin/bash # Check which interfaces these methods are implementing to confirm Override attribute requirements ast-grep --pattern $'interface $_ { $$$ function save($_): $_; $$$ }' ast-grep --pattern $'interface $_ { $$$ function saveRead($_): $_; $$$ }' ast-grep --pattern $'interface $_ { $$$ function exists($_): $_; $$$ }'Length of output: 236
Add missing
#[Override]attributes for interface implementationsOur static analysis is failing because several methods in
src/RoadRunnerStore.phpimplement interface contracts but lack the required#[Override]attribute. Please:• In
src/RoadRunnerStore.php, add#[Override]above each of the following method signatures:
– Line 39:public function save(Key $key): void
– Line 61:public function saveRead(Key $key, …): …
– Line 77:public function exists(Key $key): bool
– Line 89, Line 104, Line 114: (other interface-implementing methods)• Confirm which interfaces each method implements (they may be defined in external packages) and ensure the attribute is applied consistently across all overridden methods.
This will resolve the pipeline failures related to missing
#[Override]annotations.🧰 Tools
🪛 GitHub Actions: static analysis
[error] 39-39: MissingOverrideAttribute: Method Spiral\RoadRunner\Symfony\Lock\RoadRunnerStore::save should have the "Override" attribute
tests/RoadRunnerStoreTest.php (2)
169-169: Test expectation correctly updated to match new default.The test expectation has been properly updated from 60 to 0 to reflect the new default
initialWaitTtlvalue.
187-187: Test expectation correctly updated to match new default.Consistent with the other test case, this expectation has been properly updated to reflect the new default wait TTL value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/RoadRunnerStoreTest.php (1)
194-233: Good test coverage for the newwithTtlmethod signature.The new parameterized test provides solid coverage for the updated
withTtlmethod that now accepts an optionalwaitTtlparameter. The data provider covers key scenarios including null, fractional, and zero values.Consider adding test cases for edge scenarios such as:
- Negative TTL/waitTtl values (if they should be handled)
- Very large values
- Invalid input types (if applicable)
To enhance test coverage, consider adding these test cases to the data provider:
+ yield 'large values' => [ + 3600, + 1800, + 3600, + 1800, + ]; + + yield 'negative ttl' => [ + -1, + null, + -1, + 0, + ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/RoadRunnerStore.php(7 hunks)tests/RoadRunnerStoreTest.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/RoadRunnerStore.php
🔇 Additional comments (1)
tests/RoadRunnerStoreTest.php (1)
169-169: LGTM! Test expectations correctly updated to reflect new default value.The test expectations have been properly updated to use
0instead of60as the fourth argument (initialWaitTtl), which correctly aligns with the PR objective to change the defaultinitialWaitTtlfrom 60 to 0 seconds.Also applies to: 187-187
roxblnfk
changed the title
3 participants
Also use
initialWaitTtlwhile lock creationSummary by CodeRabbit