Conditionally load job classes

[mikkamp]

Changes proposed in this Pull Request:

This PR changes the behaviour for loading Job classes. This is part of a series of performance improvements, each change doesn't make a major difference but every little helps reduce the load.

The class JobInitializer already had a is_needed which should prevent the functionality from running on the frontend. However the way the code was structured all the job classes were loaded even if they are never used. So this PR went through several steps to ensure they are only loaded when actually used.

The first step was to prevent interface JobInterface from extending interface Service. There is no need for each individual job to be a service as the JobRepository can handle the management of these classes. All services are loaded when we call maybe_register_services, so by removing this, we prevent the container from loading the job classes both when fetching Service and later for JobInterface, only the latter is needed.

The second issue was that the JobRepository was being passed a list of all jobs in the constructor. This was changed to provide the container, so the JobRepository can load the jobs when needed, only when the is_needed function returns true (so we don't have to load them on frontend requests).

Also in SyncerHooks we should load jobs only once the hook is triggered (not in the constructor). This prevents them from loading too early, where they might not even be needed in the request.

We also ensure that the JobRepository is used consistently throughout the code, which will make it easier for any future optimizations.

The timing isn't exact as there are a lot of things which can be cached, but if we compare some of the profiling for front end requests before and after the changes we can see some significant changes in how many classes need to be resolved.

Before:

After:

Resolves part of #1250

Detailed test instructions:

The functionality of the code wasn't changed, so the core thing to test is that scheduled jobs continue to run in the same way as they used to.

Test syncing single product

1. Connect a site to a Merchant Center account to enable product syncing
2. Edit a product (change some details) and save it
3. Go to WooCommerce > Status > Scheduled Actions, and confirm that the action gla/jobs/update_products/process_item is completed without issues
4. Check the Merchant Center to confirm those changes have been synced (might need to wait a while for the product to change)

Test syncing all products

1. Go to the connection test page: /wp-admin/admin.php?page=connection-test-admin-page
2. Sync all products (with Async enabled)
3. Go to WooCommerce > Status > Scheduled Actions, and confirm that there are several completed actions for gla/jobs/update_all_products/create_batch and gla/jobs/update_all_products/process_item (depends on how many products there are)

Test restoring recurring actions

1. Go to WooCommerce > Status > Scheduled Actions, and cancel the pending actions wc_gla_cron_daily_notes and gla/jobs/resubmit_expiring_products/start
2. The second job should cancel but be scheduled again immediately
3. Deactivate and reactive the extension to get the first action back into the pending list

Changelog entry

• Tweak - Conditionally load job classes.

[codecov]

Codecov Report

Attention: Patch coverage is 83.07692% with 11 lines in your changes missing coverage. Please review.

Project coverage is 67.3%. Comparing base (733b466) to head (04102b6).
Report is 12 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/ConnectionTest.php	0.0%	5 Missing ⚠️
...c/API/Site/Controllers/GTINMigrationController.php	50.0%	3 Missing ⚠️
src/Coupon/SyncerHooks.php	84.6%	2 Missing ⚠️
...ternal/DependencyManagement/JobServiceProvider.php	0.0%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             develop   #2758     +/-   ##
===========================================
+ Coverage       67.1%   67.3%   +0.2%     
+ Complexity      4675    4673      -2     
===========================================
  Files            479     479             
  Lines          19548   19539      -9     
===========================================
+ Hits           13116   13158     +42     
+ Misses          6432    6381     -51     

Flag	Coverage Δ
php-unit-tests	67.3% <83.1%> (+0.2%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/Infrastructure/GoogleListingsAndAdsPlugin.php	0.0% <ø> (ø)
...ernal/DependencyManagement/RESTServiceProvider.php	100.0% <100.0%> (ø)
src/Jobs/JobException.php	70.8% <100.0%> (+37.5%)	⬆️
src/Jobs/JobInterface.php	0.0% <ø> (ø)
src/Jobs/JobRepository.php	100.0% <100.0%> (+66.7%)	⬆️
src/MerchantCenter/AccountService.php	94.7% <100.0%> (ø)
src/MerchantCenter/MerchantStatuses.php	76.2% <100.0%> (+0.1%)	⬆️
src/Product/SyncerHooks.php	90.6% <100.0%> (-0.1%)	⬇️
src/Settings/SyncerHooks.php	100.0% <100.0%> (ø)
src/Shipping/SyncerHooks.php	100.0% <100.0%> (ø)
... and 4 more

... and 3 files with indirect coverage changes

[puntope]

LGTM ✅ Thanks for the refactoring:

• Tested Sync a single product
• Tested sync all products async
• Tested Migrate GTIN Job
• Tested cancelling gla/jobs/resubmit_expiring_products/start

Note: Just curious why we don't need to test the rest of Jobs like Notifications if we also refactor them.

[mikkamp]

Note: Just curious why we don't need to test the rest of Jobs like Notifications if we also refactor them.

In my testing I wanted to confirm that at least one of each specific job type was tested. Since a lot of jobs share base classes I didn't add test instructions for every single job as we aren't changing the functionality of the jobs, just how / when they are initialized.

For notes specifically it was my intention to refactor this at a later stage (if there are optimizations to be done) to ensure they are only loaded when needed.