Test/queue runner refacto #7905
Draft
+571
−235
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Extract shared functionality from ActionScheduler_Abstract_QueueRunner - Add abstract methods for queue group, cron hook, and schedule - Use wpm_apply_filters_typed for type-safe filter handling - Provide foundation for feature-specific queue runners Refs: #7894
- Remove duplicate code by extending AbstractQueueRunner instead of ActionScheduler_Abstract_QueueRunner - Maintain 60-second processing intervals for backward compatibility - Keep 'rocket-rucss' queue group for proper isolation - Preserve all existing RUCSS functionality unchanged Refs: #7894
- Extend AbstractQueueRunner for shared functionality - Process Rocket Insights jobs every 30 seconds instead of 60 - Use 'rocket-insights' queue group for complete isolation from RUCSS - Follow singleton pattern consistent with existing architecture Refs: #7894
- Add 'ri_queue_runner' service registration to container - Enable dependency injection for the queue runner - Support clean initialization through service provider pattern Refs: #7894
- Add 'initialize_ri_queue_runner' hook to properly initialize queue runner - Remove unused queue_runner property (uses singleton pattern instead) - Ensure queue runner is initialized when Rocket Insights is active Refs: #7894
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the queue runner implementation by extracting common functionality from RUCSSQueueRunner into a new AbstractQueueRunner base class. This enables code reuse across multiple queue runners (RUCSS and Rocket Insights).
- Introduces
AbstractQueueRunnerabstract class with common queue processing logic - Refactors
RUCSSQueueRunnerto extend the new abstract class, removing ~200 lines of duplicate code - Adds new
QueueRunnerfor Rocket Insights with a 30-second schedule interval - Implements initialization for Rocket Insights queue runner in the subscriber
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| inc/Engine/Common/Queue/AbstractQueueRunner.php | New abstract base class providing common queue runner functionality including initialization, batch processing, cache management, and abstract methods for child classes to define their specific configuration |
| inc/Engine/Common/Queue/RUCSSQueueRunner.php | Refactored to extend AbstractQueueRunner, removing duplicate code and implementing only the configuration methods (cron schedule, hook name, and queue group) |
| inc/Engine/Admin/RocketInsights/Queue/QueueRunner.php | New queue runner implementation for Rocket Insights extending AbstractQueueRunner with a 30-second schedule interval |
| inc/Engine/Admin/RocketInsights/Subscriber.php | Added initialization method for Rocket Insights queue runner hooked to admin_init |
| inc/Engine/Admin/RocketInsights/ServiceProvider.php | Minor whitespace/formatting adjustment to comment placement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
- Create abstract base class extending ActionScheduler_Abstract_QueueRunner - Provides common functionality for RUCSS and Rocket Insights queue runners - Implements shared init(), run(), and do_batch() methods - Abstract methods for queue-specific configuration (group, schedule, cron hook) - Enables creation of separate queue runners with different intervals Part of queue runner refactoring for issue #7894
- Extend AbstractQueueRunner instead of ActionScheduler_Abstract_QueueRunner - Maintains 60-second processing interval (every_minute schedule) - Uses 'rocket-rucss' queue group for RUCSS jobs only - Preserves existing RUCSS functionality and behavior - Singleton pattern with instance() method Part of queue runner refactoring for issue #7894
- Create new QueueRunner extending AbstractQueueRunner for Rocket Insights - Processes jobs every 30 seconds (every_thirty_seconds schedule) vs 60s for RUCSS - Uses 'rocket-insights' queue group for complete separation from RUCSS - Singleton pattern with instance() method for consistency - Dedicated cron hook 'action_scheduler_run_queue_rocket_insights' Addresses issue #7894 requirement for faster RI processing
- Add dynamic queue group assignment based on optimization_type - Add factory methods: for_rucss() and for_rocket_insights() - Fix critical bug where RI jobs were incorrectly assigned to 'rocket-rucss' group - Enable proper queue separation for different optimization types
- Modify process_pending_jobs() to use Queue factory methods - Ensure jobs are processed by correct queue runners based on optimization_type - Fix routing of jobs to appropriate queue groups (rucss vs rocket-insights) - Complete the queue separation implementation
- Remove queue parameter from JobProcessor constructor and registration - Remove unused queue property from JobProcessor class - PHPStan compliance fix for unused written property
- Implement full do_batch logic since parent abstract class has no such method - Fix method parameter contravariance in run() method signature - Remove duplicate PHPDoc comments causing parsing issues - Complete ActionScheduler integration with proper claim handling - PHPStan compliance achieved
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add exception handling with try/catch/finally in AbstractQueueRunner::do_batch() - Add clear_caches() call after releasing claim to prevent memory issues - Remove unnecessary do_batch overrides from RUCSSQueueRunner and RocketInsightsQueueRunner - Fix spelling typo: 'identifer' -> 'identifier' in docblocks - Clean up excessive whitespace in ServiceProvider comment
- Add missing @throws tag in do_batch() docblock - Fix inline comment punctuation (must end with periods)
- Add 'ri_queue_runner' to provides array for container registration - Register RIQueueRunner as shared service in container - Enable dependency injection for the queue runner service Addresses GitHub feedback about missing service registration
- Clean up queue runner cron when Rocket Insights is disabled - Add cleanup_queue_runner_cron() method for deactivation cleanup - Follow same pattern as other scheduling methods in codebase - Ensure cron events are properly removed when feature is disabled Addresses GitHub feedback about missing cleanup when disabled
- Add 'action_scheduler_run_queue_rocket_insights' to events cleanup list - Ensure queue runner cron is properly removed on plugin uninstall - Maintain consistency with RUCSS queue runner cleanup pattern Addresses GitHub feedback about missing uninstall cleanup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #(issue number)
Explain how this code impacts users.
Type of change
Detailed scenario
What was tested
Describe the scenarios that you tested, and specify if it is automated or manual. For manual scenarios, provide a screenshot of the results.
How to test
Describe how the PR can be tested so that the validator can be autonomous: environment, dependencies, specific setup, steps to perform, API requests, etc.
Technical description
Documentation
Explain how this code works. Diagrams & drawings are welcome.
New dependencies
List any new dependencies that are required for this change.
Risks
List possible performance & security issues or risks, and explain how they have been mitigated.
Mandatory Checklist
Code validation
Code style
Unticked items justification
If some mandatory items are not relevant, explain why in this section.
Additional Checks