fix property access when no hooks #157
Conversation
Signed-off-by: Robert Landers <landers.robert@gmail.com>
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ 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). (2)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #157 +/- ##
============================================
+ Coverage 46.85% 47.21% +0.35%
- Complexity 1313 1317 +4
============================================
Files 103 103
Lines 3677 3679 +2
============================================
+ Hits 1723 1737 +14
+ Misses 1954 1942 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Performance Metrics
Performance Metrics
|
![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 (1)
tests/Unit/EntityHistoryTest.php (1)
245-263: Enhance the access control test coverage.The security test correctly validates that restricted properties throw SecurityException, but could be improved:
- Import the attribute class instead of using fully qualified name:
+use Bottledcode\DurablePhp\State\Attributes\DenyAnyOperation; - #[Bottledcode\DurablePhp\State\Attributes\DenyAnyOperation(fromType: 'test')] + #[DenyAnyOperation(fromType: 'test')]
- Test both get and set operations to ensure comprehensive access control:
// Try to access the restricted property $restrictedEvent = RaiseEvent::forOperation('$restrictedProperty::get', []); + // Test both get and set operations are restricted expect(fn() => processEvent($restrictedEvent, $history->applyRaiseEvent(...)))->toThrow(SecurityException::class); + + $restrictedSetEvent = RaiseEvent::forOperation('$restrictedProperty::set', ['new value']); + expect(fn() => processEvent($restrictedSetEvent, $history->applyRaiseEvent(...)))->toThrow(SecurityException::class);
- Consider testing the public property to demonstrate the access control is selective.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Unit/EntityHistoryTest.php(2 hunks)
⏰ 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). (2)
- GitHub Check: Unit Tests - PHP
- GitHub Check: build-docker
🔇 Additional comments (3)
tests/Unit/EntityHistoryTest.php (3)
27-27: LGTM! Necessary imports for new test functionality.The SecurityException and TaskCompleted imports are appropriately added to support the new property signal tests.
Also applies to: 30-30
221-243: Comprehensive property set signal test.The test thoroughly validates property setting functionality by checking both the state change (
$history->getState()->testProperty) and the event result. The test structure and assertions are well-designed.
199-219: NestedgetInnerEvent()calls correctly unwrap the event layers.Verified that:
AwaitResultimplementsgetInnerEvent()and returns the inner event passed in.- Wrapper classes (e.g.
WithFrom,WithEntity, etc.) also implementgetInnerEvent().TaskCompleteddoes not implementgetInnerEvent(), so a second call lands on its result.No changes needed—the current assertions on lines 217–218 of
tests/Unit/EntityHistoryTest.phpaccurately verify the unwrappedTaskCompletedevent and itsresult.
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Summary by CodeRabbit