chore: lazy init sms adapter + late static binding#10961
Conversation
📝 WalkthroughWalkthroughThis pull request updates several return type declarations from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Event/Event.php (1)
95-230: Consider applying late static binding consistently across all fluent setters.The change from
selftostaticimproves inheritance support by using late static binding. However, this pattern has been applied to only 7 of the 13 fluent setter methods in the class.The following methods still return
self:
setPaused()(line 76)setContext()(line 258)setClass()(line 282)setParam()(line 306)reset()(line 406)from()(line 614)For consistency and to fully support fluent chaining in subclasses, consider updating the remaining setter methods to also return
static. If certain methods should intentionally returnself, documenting the rationale would be helpful for future maintainers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Appwrite/Event/Event.php(7 hunks)src/Appwrite/Platform/Workers/Messaging.php(1 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). (4)
- GitHub Check: Agent
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (1)
src/Appwrite/Platform/Workers/Messaging.php (1)
390-392: LGTM: Lazy initialization pattern correctly implemented.The lazy initialization of the SMS adapter is appropriate and defers resource creation until actually needed. The existing null-check fallback at line 394 ensures graceful degradation if adapter creation fails.
There was a problem hiding this comment.
Pull request overview
This PR implements two performance and type safety improvements for the Appwrite messaging system. The changes prevent unnecessary SMS adapter initialization in worker processes and improve type inference for method chaining in Event classes.
Key Changes:
- Lazy initialization of SMS adapter in Messaging worker to avoid creating adapter instances in workers that don't process SMS messages
- Late static binding for Event class setter methods to return correct subclass types instead of parent Event type
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Appwrite/Platform/Workers/Messaging.php | Moved SMS adapter initialization from constructor to lazy initialization in sendInternalSMSMessage method, preventing unnecessary adapter creation when worker handles non-SMS messages |
| src/Appwrite/Event/Event.php | Changed return types from self to static for setter methods (setQueue, setEvent, setProject, setPlatform, setUser, setUserId, setPayload) to enable proper type inference in subclasses through late static binding |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c912b51 to
c7fd312
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Event/Event.php (1)
76-406: Consider unifying return types for all fluent interface methods.Several other setter methods (
setPaused,setContext,setClass,setParam,reset) still returnselfrather thanstatic. While the current changes fix the immediate type errors, adoptingstaticconsistently across all fluent methods would improve type safety and prevent similar issues in the future.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Appwrite/Event/Database.php(1 hunks)src/Appwrite/Event/Event.php(7 hunks)src/Appwrite/Event/Messaging.php(0 hunks)src/Appwrite/Platform/Workers/Messaging.php(1 hunks)
💤 Files with no reviewable changes (1)
- src/Appwrite/Event/Messaging.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Platform/Workers/Messaging.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Appwrite/Event/Database.php (1)
src/Appwrite/Event/Event.php (1)
setProject(142-146)
src/Appwrite/Event/Event.php (1)
src/Appwrite/Event/Database.php (1)
setProject(164-178)
⏰ 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). (3)
- GitHub Check: Benchmark
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Event/Database.php (1)
164-178: LGTM! Correct use of late static binding.The change from
selftostaticenables proper type resolution in method chaining whenDatabaseextendsEvent. This allows subclasses to return their own type rather than the parent type, fixing type-related errors.src/Appwrite/Event/Event.php (1)
95-230: LGTM! Late static binding correctly implemented.The changes from
selftostaticreturn types enable proper type inference for subclasses, addressing the type errors mentioned in the PR objectives. These modifications maintain backward compatibility while improving type safety in inheritance chains.
✨ Benchmark results
⚡ Benchmark Comparison
|
Uh oh!
There was an error while loading. Please reload this page.