Skip to content

Conversation

obenland
Copy link
Member

@obenland obenland commented Feb 19, 2025

Proposed changes:

  • Replace hardcoded allowlist in Scheduler with extensible registration system
  • Add Scheduler::register_async_batch_callback() method for dynamic callback registration
  • Remove hardcoded Migration callbacks from Scheduler class
  • Update Migration class to register its callbacks through new registration system
  • Add safety check to ensure callbacks are registered at proper time
  • Maintain full backward compatibility - async_batch works exactly the same way

Other information:

  • Have you written new tests for your changes, if applicable?

Testing instructions:

  • Run the existing test suite to verify backward compatibility:
    npm run env-test -- --filter Test_Scheduler
  • All 12 tests should pass, confirming async batch functionality works as before
  • Test that Migration callbacks are properly registered and functional:
    npm run env-test -- --filter test_update_comment_counts_with_existing_valid_lock
    npm run env-test -- --filter test_async_upgrade

Usage for other plugins/classes:

Any class can now register async batch jobs without modifying core files:

class My_Custom_Jobs {
    public static function init() {
        // Must be called before or during 'init' action
        Scheduler::register_async_batch_callback( 'my_custom_hook', array( self::class, 'my_batch_method' ) );
    }
    
    public static function my_batch_method( $batch_size = 50, $offset = 0 ) {
        // Your batch processing logic
        // Return array for next batch or null when done
    }
}

Changelog entry

  • Automatically create a changelog entry from the details below.
Changelog Entry Details

Significance

  • Patch
  • Minor
  • Major

Type

  • Added - for new features
  • Changed - for changes in existing functionality
  • Deprecated - for soon-to-be removed features
  • Removed - for now removed features
  • Fixed - for any bug fixes
  • Security - in case of vulnerabilities

Message

Add extensible async batch callback registration system to replace hardcoded allowlist

@obenland obenland requested a review from a team February 19, 2025 17:42
@obenland obenland self-assigned this Feb 19, 2025
@pfefferle
Copy link
Member

@obenland what about this PR?

@obenland
Copy link
Member Author

Depends on #1521.

@obenland obenland marked this pull request as ready for review May 13, 2025 18:30
@obenland obenland added Skip Changelog Disables the "Changelog Updated" action for PRs where changelog entries are not necessary. and removed [Status] In Progress labels May 13, 2025
@obenland
Copy link
Member Author

@pfefferle This should be ready for review

@obenland
Copy link
Member Author

obenland commented Jun 2, 2025

After team call: Let's make it possible to fill the Scheduler from class-migrations.php.

Meaning, how can we expand the batch callback allow list from the migration class.
Also move action hooks back into the migration class.
Could also be done in Migration::async_migration.

@Copilot Copilot AI review requested due to automatic review settings August 29, 2025 19:15
Copy link

@Copilot Copilot AI left a 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 refactors migration jobs to reuse the existing Scheduler::async_batch infrastructure instead of having their own dedicated scheduling system. The changes consolidate job scheduling through a unified batch processing system.

Key changes:

  • Removes custom migration scheduling methods and replaces them with async_batch infrastructure
  • Updates test coverage to reflect the new scheduling approach
  • Simplifies the migration job architecture by eliminating duplicate scheduling logic

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
includes/class-migration.php Removes async_migration and async_upgrade methods, updates migration scheduling to use async_batch
includes/class-scheduler.php Adds migration callbacks to batch_callbacks array and registers corresponding action hooks
tests/includes/class-test-migration.php Updates tests to use new Scheduler::async_batch approach and removes old migration-specific tests
tests/includes/class-test-scheduler.php Adds new tests for migration functionality using the async_batch infrastructure
Comments suppressed due to low confidence (1)

tests/includes/class-test-migration.php:1

  • Inconsistent factory usage. Line 381 uses self::factory()->post->create() while line 481 uses $this->factory->post->create(). Should use the same pattern throughout the file.
<?php

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@obenland obenland marked this pull request as draft September 3, 2025 14:54
…system

* Add Scheduler::register_async_batch_callback() method for dynamic callback registration
* Remove hardcoded Migration callbacks from Scheduler class
* Update Migration class to register its callbacks through new system
* Add safety check to ensure callbacks are registered at proper time
* Maintain backward compatibility - async_batch works exactly the same way
* Enable any class to add async batch jobs without modifying core files
Moves async batch callback registration for activity sending and retrying from Scheduler's static property to Dispatcher initialization. Updates _doing_it_wrong message version to 'unreleased' for clarity.
@obenland obenland force-pushed the update/migration-async-jobs branch from 9c44a63 to 681aa0c Compare September 11, 2025 19:40
# Conflicts:
#	tests/includes/class-test-migration.php
#	tests/includes/class-test-scheduler.php
@obenland obenland marked this pull request as ready for review September 11, 2025 19:45
@obenland obenland requested a review from pfefferle September 11, 2025 19:45
Co-authored-by: Matthias Pfefferle <[email protected]>
@obenland obenland requested a review from pfefferle September 19, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Disables the "Changelog Updated" action for PRs where changelog entries are not necessary.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants