newfold-labs/wp-module-tasks

A task system for processing asynchronous tasks with wordpress, the tasks could be one-off long running tasks or periodic tasks.

wp-module-tasks's Issues

Implement tests

We should implement some PHPUnit tests for key functionality.

Consider making priority optional

It seems like there would be times when just ordering tasks by priority alone could be limiting. It seems like we might want to have some tasks processed based on date/time, with priority being optional or being an additional sort if priority is the same.

I can see not using priority at all for handling things like processing admin page view events and instead handling those as either a stack (LIFO) or a queue (FIFO).

There could be other times when we would want a true queue or stack instead of a solely priority-based system.

I'd love to see a system where you can instantiate a queue, and configure it to run one or more jobs on a subset of tasks (e.g. by task name) at a specific cron interval. Likewise, you could instantiate a stack or priority queue at different intervals for different subsets of tasks.

I think this package would provide some inspiration in this direction: https://github.com/deliciousbrains/wp-queue

Ensure use of `$wpdb->prepare()`

There are some places where $wpdb->prepare() should be used. It appears this was done in a few places, but is missing in others.

For example: https://github.com/newfold-labs/wp-module-tasks/blob/main/includes/Models/Data/Task.php#L290

Make cron schedule configurable

Having a 20-second cron schedule could overwhelm the site’s resources with overlapping cron tasks on a high-traffic site.

While I realize this 20-second cron is important when setting up a site to ensure plugins are properly installed during onboarding, I don't think we should retain this cron schedule past onboarding.

wp-module-tasks/includes/Scheduler.php

Lines 36 to 59 in a7d309c

	/**
	* Add a 20 seconds interval
	*
	* @param array $schedules The existing interval schedules
	*/
	public function add_interval_schedule( $schedules ) {
	// Adds the schedule for the given intervals in seconds
	if ( ! array_key_exists( 'twenty_seconds', $schedules ) || 20 !== $schedules[ 'twenty_seconds' ]['interval'] ) {
	$schedules[ 'twenty_seconds' ] = array(
	'interval' => 20,
	'display' => __( 'Cron to run once every twenty seconds' ),
	);
	}
	// Adds the schedule for the given intervals in seconds
	if ( ! array_key_exists( 'ten_minutes', $schedules ) || 600 !== $schedules[ 'ten_minutes' ]['interval'] ) {
	$schedules[ 'ten_minutes' ] = array(
	'interval' => 600,
	'display' => __( 'Cron to run once every ten minutes' ),
	);
	}
	return $schedules;
	}

The existing cleanup task should happen on the pre-existing 15-minute cron instead of adding a new 10-minute cron.

We should remove the 20-second cron and make it possible to configure custom cron schedules to handle different groups/types of tasks at different intervals.

Reconsider method of Task instantiation

Having 9 parameters to instantiate a Task seems a bit much. Do we really need them all in the constructor?

wp-module-tasks/includes/Models/Data/Task.php

Lines 135 to 143 in a7d309c

	$id = null,
	$task_name = null,
	$task_executor_path = null,
	$task_execute = null,
	$args = null,
	$num_retries = 0,
	$task_interval = null,
	$task_priority = null,
	$enabled = true

For example, why not default to a low or mid-range priority and then allow calling $task->priority( 80 ) to change it?

By introducing chainable methods, we could do something like $task->priority( 80 )->retries(3). This allows us to instantiate in a more reader-friendly way and without having to pass null for multiple parameters.

Remove unused code

In looking at the code, it seems that there are some things we aren't using at all. To avoid technical debt, it would be best to remove these things.

For example:

• https://github.com/newfold-labs/wp-module-tasks/search?q=task_executor_path
• https://github.com/newfold-labs/wp-module-tasks/search?q=task_execute

Drop `Models.php` file since it isn't in use

https://github.com/newfold-labs/wp-module-tasks/search?q=models&type=code

Drop https://github.com/newfold-labs/wp-module-tasks/blob/main/includes/Models/Models.php

Update Satis reference in readme file

Update readme Satis reference:

composer config repositories.newfold composer https://newfold.github.io/satis

Should be:

composer config repositories.newfold composer https://newfold-labs.github.io/satis

Avoid use of `phpcs:ignore`

We should avoid using // phpcs:ignore since this ignores ALL rules. I think it is probably irrelevant in most cases since you've addressed the issues. However, if there is a specific rule we are trying to intentionally ignore, we should include the rule name in the comment so that other rules aren't ignored.

Instances:

• wp-module-tasks/includes/Models/Data/TaskResult.php

  Line 88 in a048966

  // phpcs:ignore

• wp-module-tasks/includes/Models/Data/TaskResult.php

  Line 129 in a048966

  // phpcs:ignore

• wp-module-tasks/includes/Models/Data/Task.php

  Line 181 in a048966

  // phpcs:ignore

• wp-module-tasks/includes/Models/Data/Task.php

  Line 270 in a048966

  // phpcs:ignore

• wp-module-tasks/bootstrap.php

  Line 70 in a048966

  // phpcs:ignore

• wp-module-tasks/bootstrap.php

  Line 72 in a048966

  // phpcs:ignore

• wp-module-tasks/includes/Scheduler.php

  Line 101 in a048966

  // phpcs:ignore

What are the use cases for disabling a task?

Is there a reason we would disable a task as opposed to just deleting it?

If not, we should remove the enabled column from the database as well as any class properties, etc.

Use `set_time_limit()` instead of `ini_set( 'max_execution_time' )`

As a best practice, we should use the set_time_limit() function instead of the ini_set() function and setting max_execution_time.

The primary reason is that the set_time_limit() function sets the time limit for the current script only. Setting max_execution_time impacts the entire PHP environment and could cause performance issues.

Restrict when tables checks are done and add code to allow deleting tables as well

Currently, table setup checks are being run on every page load. We should change that up so that the plugin can call a function in the module when the plugin is activated (or deactivated) to set up or remove those tables. Alternatively, our plugins have upgrade routines that could handle the table setup.

wp-module-tasks/includes/Models/Data/Task.php

Lines 91 to 117 in a7d309c

	public static function setup() {
	global $wpdb;
	$table_name = MODULE_TASKS_TASK_TABLE_NAME;
	// Maybe create the table
	if ( ! function_exists( 'maybe_create_table' ) ) {
	require_once ABSPATH . 'wp-admin/includes/upgrade.php';
	}
	$charset_collate = $wpdb->get_charset_collate();
	$sql = "CREATE TABLE `{$wpdb->prefix}{$table_name}` (
	task_id bigint(20) NOT NULL AUTO_INCREMENT,
	task_name varchar(63) NOT NULL,
	task_executor_path varchar(125),
	task_execute varchar(125),
	args longtext,
	num_retries int(2) UNSIGNED,
	task_interval int(2) UNSIGNED,
	task_priority int(2) UNSIGNED,
	task_status varchar(12),
	updated TIMESTAMP NOT NULL ON UPDATE CURRENT_TIMESTAMP,
	enabled tinyint(1),
	PRIMARY KEY (task_id)
	) $charset_collate;";
	maybe_create_table( $wpdb->prefix . $table_name, $sql );
	}

wp-module-tasks/includes/Models/Data/TaskResult.php

Lines 12 to 33 in db66d27

	public static function setup() {
	global $wpdb;
	$table_name = MODULE_TASKS_TASK_RESULTS_TABLE_NAME;
	// Maybe create the table
	if ( ! function_exists( 'maybe_create_table' ) ) {
	require_once ABSPATH . 'wp-admin/includes/upgrade.php';
	}
	$charset_collate = $wpdb->get_charset_collate();
	$sql = "CREATE TABLE `{$wpdb->prefix}{$table_name}` (
	task_result_id bigint(20) NOT NULL AUTO_INCREMENT,
	task_id bigint(20) NOT NULL,
	task_name varchar(63) NOT NULL,
	stacktrace longtext,
	success tinyint(1),
	updated TIMESTAMP NOT NULL ON UPDATE CURRENT_TIMESTAMP DEFAULT CURRENT_TIMESTAMP,
	PRIMARY KEY (task_result_id)
	) $charset_collate;";
	maybe_create_table( $wpdb->prefix . $table_name, $sql );
	}

Call the `setup_task_*` functions via `register_activation_hook`

Currently, the setup_task_* functions are still called on every page load. We should move these into a register_activation_hook call (as you did with the register_deactivation_hook).

https://github.com/newfold-labs/wp-module-tasks/blob/main/bootstrap.php#L100-L101

Should we be using any additional indexes on our custom tables?

Have we looked into adding any additional indexes to our custom tables? Is it worth indexing by task name or priority?

https://spinupwp.com/wordpress-database-optimization-indexing/