Background process runner#498
Background process runner#498ankitrox wants to merge 11 commits intofeature/regenerate-existing-imagesfrom
Conversation
BG process placeholder class
Checks if the job is still running and prevents not re-run
felixarntz
left a comment
There was a problem hiding this comment.
@ankitrox I think this PR is a bit too much all at once. Let's work in smaller units, to be able to have focused reviews and not block multiple items because of having them in one PR.
Your PR currently tackles #482, #494, and also #503. Can you please reduce this (or open a new PR with the code instead) which only focuses on #503? We should always try to have one PR for one issue. Of course, don't completely delete the other code, but let's split that into multiple PRs. I believe the first one should be the PR for #503 since that is the most foundational part of the three.
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Left some quick feedback.
| */ | ||
| public function __construct() { | ||
| add_action( 'wp_ajax_' . Perflab_Background_Process::BG_PROCESS_ACTION, array( $this, 'handle_request' ) ); | ||
| add_action( 'wp_ajax_nopriv' . Perflab_Background_Process::BG_PROCESS_ACTION, array( $this, 'handle_request' ) ); |
There was a problem hiding this comment.
| add_action( 'wp_ajax_nopriv' . Perflab_Background_Process::BG_PROCESS_ACTION, array( $this, 'handle_request' ) ); | |
| add_action( 'wp_ajax_nopriv_' . Perflab_Background_Process::BG_PROCESS_ACTION, array( $this, 'handle_request' ) ); |
| /** | ||
| * Class Perflab_Background_Job. | ||
| * | ||
| * Runs the heavy lifting tasks in background in separate process. |
| /** | ||
| * Meta key for storing job name/action. | ||
| * | ||
| * @const string |
There was a problem hiding this comment.
Add @since n.e.x.t for all constant and variables https://developer.wordpress.org/coding-standards/inline-documentation-standards/php/#2-1-class-members
| /** | ||
| * Perflab_Background_Job constructor. | ||
| * | ||
| * @param int $job_id job ID. |
There was a problem hiding this comment.
Add @since n.e.x.t for all the functions.
| * @param string $name Job identifier or name. | ||
| * @param array $data Job data. | ||
| * | ||
| * @since n.e.x.t |
There was a problem hiding this comment.
As per document standard https://developer.wordpress.org/coding-standards/inline-documentation-standards/php/#4-hooks-actions-and-filters you should add @since before the parameter document.
| add_action( 'wp_ajax_' . Perflab_Background_Process::BG_PROCESS_ACTION, array( $this, 'handle_request' ) ); | ||
| add_action( 'wp_ajax_nopriv' . Perflab_Background_Process::BG_PROCESS_ACTION, array( $this, 'handle_request' ) ); |
There was a problem hiding this comment.
For the same file class file, we should use self::. It should be updated in other places as well.
| add_action( 'wp_ajax_' . Perflab_Background_Process::BG_PROCESS_ACTION, array( $this, 'handle_request' ) ); | |
| add_action( 'wp_ajax_nopriv' . Perflab_Background_Process::BG_PROCESS_ACTION, array( $this, 'handle_request' ) ); | |
| add_action( 'wp_ajax_' . self::BG_PROCESS_ACTION, array( $this, 'handle_request' ) ); | |
| add_action( 'wp_ajax_nopriv' . self::BG_PROCESS_ACTION, array( $this, 'handle_request' ) ); |
|
Thanks for the review @mukeshpanchal27 . As per discussion with @felixarntz , I have created a separate (and smaller) PR background job class: #507 So, this PR will be closed and further work will be continued in those smaller chunks. Please consider to review those new PRs. |
|
Closing to avoid confusion with another open PR |
Background process runner class.
Mainly responsible for handling and validating the heavy lifting tasks in background without hampering the user experience.
Summary
Fixes #482 #494
Relevant technical choices
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.