-
-
Notifications
You must be signed in to change notification settings - Fork 27
fix(sentry): remove duplicate code and improve singleton instance reset in SingletonAspect #1048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…et in SingletonAspect - Remove duplicate variable assignments in SingletonAspect::process() - Add proper singleton instance reset logic using Closure::bind - Support both 'instance' and 'instances' property patterns - Ensure clean singleton behavior when context already exists
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough引入 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 分钟
Possibly related PRs
Suggested reviewers
兔子的诗
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (4)**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/**/src/**/*.php📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/*/src/**/*.php📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.php📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (5)📚 Learning: 2025-12-12T05:33:43.947ZApplied to files:
📚 Learning: 2025-12-12T05:33:43.947ZApplied to files:
📚 Learning: 2025-12-12T05:33:43.947ZApplied to files:
📚 Learning: 2025-12-12T05:33:43.947ZApplied to files:
📚 Learning: 2025-12-12T05:33:43.947ZApplied to files:
🧬 Code graph analysis (1)src/sentry/src/Metrics/Aspect/HistogramAspect.php (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). (19)
🔇 Additional comments (13)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.33)At least one path must be specified to analyse. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sentry/src/Aspect/SingletonAspect.php(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: All PHP files must includedeclare(strict_types=1)at the top
Use PSR-12 coding standards for PHP code formatting
Use 4-space indentation with short array syntax in PHP code
Use.php-cs-fixer.phpconfiguration for PHP code formattingAll PHP files must include a strict_types declaration at the top
**/*.php: Follow project PHP coding standard enforced by php-cs-fixer with PSR-12 style, 4-space indentation, and short array syntax
Maintain type coverage by updating or adding tests when public APIs change; ensurecomposer test:typesstays green before pushing
Files:
src/sentry/src/Aspect/SingletonAspect.php
src/**/src/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use namespace pattern
FriendsOfHyperf\{ComponentName}for all component classes
Files:
src/sentry/src/Aspect/SingletonAspect.php
src/*/src/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
src/*/src/**/*.php: Namespace convention for components must follow FriendsOfHyperf{ComponentName}
All code must be coroutine-safe and avoid global state without proper context management, blocking I/O operations, and non-coroutine-safe third-party libraries without wrappers
Use Hyperf's Context API for request-scoped data instead of global state
Follows PSR-12 coding standards and use PHP-CS-Fixer for automatic formatting
Use PHPStan at maximum level for static analysis
Ensure component namespace doesn't conflict with existing Hyperf components or other packages in the ecosystem
Integrate deeply with Hyperf's Dependency Injection container for service registration
Leverage Hyperf's AOP (Aspect-Oriented Programming) for cross-cutting concerns via aspects defined in ConfigProvider
Use Hyperf's Event System to register listeners for framework events in components
All code must support coroutine-based concurrency using Swoole/Swow compatibility
Files:
src/sentry/src/Aspect/SingletonAspect.php
src/**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
Each component lives in its own subdirectory with PSR-4 namespaces matching
FriendsOfHyperf\*
Files:
src/sentry/src/Aspect/SingletonAspect.php
🧠 Learnings (1)
📚 Learning: 2025-12-12T05:33:43.947Z
Learnt from: CR
Repo: friendsofhyperf/components PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-12T05:33:43.947Z
Learning: Applies to src/*/src/**/*.php : Leverage Hyperf's AOP (Aspect-Oriented Programming) for cross-cutting concerns via aspects defined in ConfigProvider
Applied to files:
src/sentry/src/Aspect/SingletonAspect.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.3
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.3
- GitHub Check: Agent
- GitHub Check: Seer Code Review
🔇 Additional comments (1)
src/sentry/src/Aspect/SingletonAspect.php (1)
14-14: LGTM!正确添加了
Closure导入以支持后续的Closure::bind使用。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR attempts to improve the SingletonAspect by removing duplicate code and adding singleton instance reset logic to ensure proper coroutine-safe singleton behavior. However, the implementation contains several critical bugs that prevent it from working as intended.
Key Changes
- Adds Closure import and singleton reset logic using Closure::bind
- Attempts to remove duplicate variable assignments (but introduces new duplicates instead)
- Adds support for resetting both 'instance' and 'instances' property patterns
| Closure::bind(function () use ($className, $arguments) { | ||
| match (true) { | ||
| property_exists($className, 'instance') => $className::$instance = null, | ||
| property_exists($className, 'instances') => $className::$instances[$arguments[0]] = null, |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line attempts to set $className::$instances[$arguments[0]] to null, but it doesn't verify that $arguments[0] exists. If the getInstance() method is called without arguments but the class has an 'instances' property, this will cause an "Undefined array key 0" error. Add a check to ensure $arguments[0] exists before accessing it.
| property_exists($className, 'instances') => $className::$instances[$arguments[0]] = null, | |
| property_exists($className, 'instances') && array_key_exists(0, $arguments) => $className::$instances[$arguments[0]] = null, |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…et in SingletonAspect (#1048) * fix(sentry): remove duplicate code and improve singleton instance reset in SingletonAspect - Remove duplicate variable assignments in SingletonAspect::process() - Add proper singleton instance reset logic using Closure::bind - Support both 'instance' and 'instances' property patterns - Ensure clean singleton behavior when context already exists * 更新 SingletonAspect.php Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(sentry): refine singleton instance reset logic in SingletonAspect * fix(metrics): remove unused spawnDefaultMetrics function in MetricSetter trait * fix(metrics): 移除冗余的defer调用,直接在事件处理后刷新指标 * fix(metrics): 移除defer调用,直接在事件处理后刷新指标 * fix(coroutine): 替换defer调用为Coroutine::defer以统一协程处理方式 --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
Test plan
composer test:unit --group=sentrySummary by CodeRabbit
Bug 修复
优化
清理
✏️ Tip: You can customize this high-level summary in your review settings.