QueueWorker processing class for sync & expire

I've made a start on this.

I'm off work for a few days, so I'm posting this patch in case anyone wants to move it forwards in my absence. Notes on what needs doing are in comments.

Working on this today.

Here's the patch.

The test was written with #2879301: Time handling and periodic field/form element type (?) already applied locally, so there's some commented-out code that relies on that and will have to be restored when that lands.

I've had what I think is a better idea for handling this, specifically, to deal with the problem of different processes wanting to renew or expire a license.

I've created a new contrib project which we'll use to handle scheduling expiry and renewals: https://www.drupal.org/project/scheduled_executable

I saw in a comment on another issue that kazanir said we might need to use Advanced Queue for this sort of thing. I had a look at that, but it didn't answer the problem of what to do when we have one cron task that wants to expire a license, and another (or a human) that wants to renew it. I figure for this we need something that schedules tasks, rather than merely queues them, and the module I've written has a system for resolving multiple tasks that have been scheduled to happen at the same time. So in our case, we'd resolve a task for expiry and a task for renewal of the same license at the same time to say that only the renewal should in fact be executed.

I've not got time right now to work on updating my patch to work for this, but I'll come back to it later.

This needs to be rebased to use AdvancedQueue, now that the module has been ported.

From the duplicate issue:

In D7 we used the core queue for expiration and AdvancedQueue for remote sync, in D8 it will be easier to simply depend on AdvancedQueue, but we can do that once AQ has a beta out.

Does AQ handle the problem of repeatedly queuing licenses if they are not being taken out of the queue quickly enough?

Eg:

- time T: Cron runs. hook_cron() puts Licenses 1-100 into the queue because their expiry is due.
- T + 30m: Cron runs. Queue worker processes only Licenses 1-10 because it runs out of time. hook_cron() puts Licenses 11-100 into the queue again because they are still unprocessed, and look like they need to be put in again.
- T + 60m: Cron runs. Queue worker processes only Licenses 11-20 because it runs out of time. hook_cron() puts Licenses 21-100 into the queue again because they are still unprocessed, and look like they need to be put in again.
- etc

@joachim
There is a feature request that would handle this: #2918866: Add support for unique jobs.

Core also already has a pattern for this that can be followed:

/**
 * Implements hook_cron().
 *
 * Queues news feeds for updates once their refresh interval has elapsed.
 */
function aggregator_cron() {
  $queue = \Drupal::queue('aggregator_feeds');

  $ids = \Drupal::entityManager()->getStorage('aggregator_feed')->getFeedIdsToRefresh();
  foreach (Feed::loadMultiple($ids) as $feed) {
    if ($queue->createItem($feed)) {
      // Add timestamp to avoid queueing item more than once.
      $feed->setQueuedTime(REQUEST_TIME);
      $feed->save();
    }
  }

  // Delete queued timestamp after 6 hours assuming the update has failed.
  $ids = \Drupal::entityQuery('aggregator_feed')
    ->condition('queued', REQUEST_TIME - (3600 * 6), '<')
    ->execute();

  if ($ids) {
    $feeds = Feed::loadMultiple($ids);
    foreach ($feeds as $feed) {
      $feed->setQueuedTime(0);
      $feed->save();
    }
  }
}

We never guarded against this in D7 cause we assumed a site would setup their cron properly to run different hooks at different intervals, but since that requires additional contrib, it's fine to consider it up front.

I have combined the work that @bojanz mentioned with the patch @joachim supplied.
Adding a new field to the License entity called "queued" with a getter and setter, using this similar to how it is being used in the Feed example above.

I have also updated the patch to work with recent changes on the dev branch.

Apologies, that was a horrible patch, I have triple checked this one and it applies much nicer. (and should hopefully pass all the tests)

> This needs to be rebased to use AdvancedQueue, now that the module has been ported.

AQ is crashing for me on install, so I'm going to commit something now that uses core queue, and that can be improved that to AQ in due course.

Committing this patch which has the following changes from #14:

- removed the 'queues' field that I'd put in ages ago
- removed the deletion of the 'queued' time after 6 hours -- that may have to be restored when remote licenses are implemented, but it'll probably be done with AQ in ways I don't yet understand
- various tweaks to docs

Urgh, this rotted again.

I am going to upload this rebase, and if it passes tests, commit it. Improvements to use AQ can come later; I don't want the work that's already here to keep getting lost.

But why introduce a queued field that we're going to remove before we even tag a release?

On Slack we discussed that we could probably get away with simply checking the license in the worker, and skipping it if it was already modified. Then, for sites which have a large number of licenses (that wouldn't be processed in a single cron run), we advise a solution that can assign different periods to different crons.

> On Slack we discussed that we could probably get away with simply checking the license in the worker, and skipping it if it was already modified.

Oops, I'd forgotten about that...

One thing to consider, which we should document in the README, is that sites using fixed periods will *really quickly* hit performance issues. Consider a license that is sold with a fixed yearly cycle: you only need 100 or so license owners in total before you have 100 licenses getting processed for expiry on the day the fixed year ends.

Here's a patch. It uses AdvancedQueue to queue up each expired license, then sets the state to expired and saves. Tests for the method that gets expired licenses in LicenseStorage, checking the jobs, and processing the job to expire a license.

Thanks for working on this!

1. +++ b/src/LicenseStorage.php
   @@ -58,4 +58,22 @@ class LicenseStorage extends CommerceContentEntityStorage implements LicenseStor
   +    if (!$time) {
   +      $time = time();
   +    }
   

   I'm happy to say that a $time parameter should always be passed in.

2. +++ b/tests/modules/commerce_license_set_expiry_test/config/install/advancedqueue.advancedqueue_queue.commerce_license.yml
   @@ -0,0 +1,7 @@
   +id: commerce_license
   +label: 'Commerce License'
   +backend: database
   +backend_configuration: {}
   +processor: cron
   +processing_time: 180
   +locked: true
   

   Do we need this? Isn't the same thing in the main module's config/install?

There's also a double blank line between two methods somewhere, but I forgot to mark it in Dreditor...

I'll find that pesky double blank line.

The main commerce module doesn't have an advanced queue config. commerce_recurring has its own. In fact, it's the only commerce module that does. We don't have a dependency on commerce_recurring, so it looked like the proper way was for us to have our own. Unless you meant the main advancedqueue module, then, sure, it has its own queue. But I followed the pattern already set in commerce_recurring.

Patch and interdiff attached.

Happy to help!

> The main commerce module doesn't have an advanced queue config. commerce_recurring has its own. In fact, it's the only commerce module that does. We don't have a dependency on commerce_recurring, so it looked like the proper way was for us to have our own.

What I meant was that we have this:

> +++ b/config/install/advancedqueue.advancedqueue_queue.commerce_license.yml

so do we also need it in a test module?

+use Drupal\Core\CronInterface;
+use Drupal\Core\Entity\EntityTypeManagerInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+
+/**
+ * Default cron implementation.
+ */
+class Cron implements CronInterface {

I find it weirder for Cron to implement the core interface, than our own, cause the core cron, the recurring cron, and the license cron all do different things.

+    $this->entityTypeManager = $entity_type_manager;
+    $this->licenseStorage = $entity_type_manager->getStorage('commerce_license');

This doesn't make sense. Either you keep $this->entityTypeManager, or you take each storage that you need, not both.
The more common pattern (esp when needing multiple storages) is to keep $this->entityTypeManager and fetch the storages as needed.

To me it doesn't feel like getLicenseIdsToExpire() is worth having, it's only needed by Cron, so it's simpler to inline it, like Recurring does. I'll leave that call to joachim though.

+class ExpireExpiredLicenses extends JobTypeBase implements ContainerFactoryPluginInterface {

May I suggest LicenseExpire? ExpireExpired sounds like stutter, to borrow a different community's guideline.

Also, you're injecting eventDispatcher but not using it.

+    try {
+      // Set the license to expired. The plugin will take care of revoking it.
+      $license->state = 'expired';
+      $license->save();
+    }
+    catch (DeclineException $exception) {
+      return $result = JobResult::failure($exception->getMessage());
+    }

Can a license save result in a DeclineException, or is this copy/paste from Recurring?

@joachim Ah, okay, I see what you're saying. I put it in the test module because installing the config for the main module includes all of the Views and other things we aren't testing for. I didn't want to have to turn that stuff on.

@bojanz As for naming, I'm cool with whatever we want.

I didn't test for the DeclineException. I think I took it from Recurring. I'll add a negative test to test for the error and make sure we can do it.

I'm not entirely sure what you mean with the cron interface, bojanz.

I'll fix up those other points and post up an update tomorrow. Thanks for looking at this, guys!

I don't understand the comment about cron either....

For the method in the storage, since we have it in the patch already, let's keep it that way. It's a useful separation for testing.

Added `advancedqueue` package dependency.

My point was that we've defined a Cron service, but it doesn't implement Drupal\commerce_license\CronInterface like the one in recurring does, it implements \Drupal\Core\CronInterface;

Sorry. The patch got included in the last patch.

Fixes #25.1

When I run cron the site breaks with error.

> Call to a member function enqueueJob() on null in Drupal\commerce_license\Cron->run() (line 76

I take #38 back. After reinstalling the module it worked alright. Thanks.

I agree with Bojan on #29 - the first improvement. We can just use load the `entityTypeManager` and further load oher storages later.

Some nitpicks.

@bojanz I added a cron interface. I see what you meant now.

@subhojit777 Included your changes.

Removed the unnecessary call to eventDispatch.

Changed that storage variable.

Renamed the job class.

I moved getLicensesToExpire() out of the storage class and into the Cron class but kept it as a separate method so we can unit test the method rather than run a whole cron to test for the results.

Thanks everyone for moving this forward!

1. +++ b/src/Cron.php
   @@ -0,0 +1,94 @@
   +   * @return array|int
   

   Shouldn't that be int[]? AFAIK array|int means 'an array or an integer', and that's not the case here. It's an array of integers.

2. +++ b/src/Cron.php
   @@ -0,0 +1,94 @@
   +    // Ones we've already revoked will be marked expired.
   

   I'm not sure what this comment means. Is it part on an older version of this patch maybe?

3. +++ b/tests/src/Kernel/LicenseCronExpiryTest.php
   @@ -0,0 +1,352 @@
   +   * Clear the entity handlers cached in the entity type manager, so that the
   +   * license storage handler gets re-instantiated with the mocked service
   +   * injected.
   

   We're not using a mocked service are we...?

   I mean, we *could* do that, rather than change the timestamps on the licenses. That would be cleaner testing. But I don't think that's necessary for this patch.

Branch tests are failing at the moment -- #2942109: failing Recurring integration test. Although hopefully that was a glitch, as locally the Recurring integration test passes for me.

Locally, I get test failures for this patch:

There were 2 errors:

1) Drupal\Tests\commerce_license\Kernel\System\LicenseCronExpiryTest::testGetLicenseIdsToExpireTomorrow
Error: Call to undefined method Drupal\commerce_license\LicenseStorage::getLicenseIdsToExpire()

/Users/joachim/Sites/_sandbox/commerce_license/tests/src/Kernel/LicenseCronExpiryTest.php:162

2) Drupal\Tests\commerce_license\Kernel\System\LicenseCronExpiryTest::testGetLicenseIdsToExpireYesterday
Error: Call to undefined method Drupal\commerce_license\LicenseStorage::getLicenseIdsToExpire()

/Users/joachim/Sites/_sandbox/commerce_license/tests/src/Kernel/LicenseCronExpiryTest.php:197

--

There was 1 failure:

1) Drupal\Tests\commerce_license\Kernel\System\LicenseCronExpiryTest::testLicenseCronExpiryExpired
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'queued' => 1
 )

/Users/joachim/Sites/drupal-8-commerce/core/tests/Drupal/KernelTests/KernelTestBase.php:1114
/Users/joachim/Sites/_sandbox/commerce_license/tests/src/Kernel/LicenseCronExpiryTest.php:274

I'm not getting any failures:

vendor/bin/phpunit -c core modules/contrib/commerce_license/tests/src/Kernel/LicenseCronExpiryTest.php
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\commerce_license\Kernel\System\LicenseCronExpiryTest
.....

Time: 31.35 seconds, Memory: 5.75MB

OK (5 tests, 40 assertions)

And those line numbers don't reference the current version of the code. 197 is blank line and 162 is:
$cron = \Drupal::service('commerce_license.cron');

And I'm not using a mocked service. There's a point when mocking all of the services becomes an exercise in how well you wrote the mocks, and we're really testing the results of our queries. It's a kernel test, so I don't think there's a problem with running a real query. I'm open to changing them, that's just my opinion! Edit: I see you were talking about the comment. Yep, I'll change it!

The "Ones we've already revoked will be marked expired." comment is explaining the ->condition('state', 'active') line, reaffirming that we are only getting licenses whose expiration dates have passed but haven't already been expired.

I think PHPStorm generated the array|int @return comment. We're returning the results of entityQuery->execute(), which is an int|array in its doc.

> We're returning the results of entityQuery->execute(), which is an int|array in its doc.

That's because that can be a count query, which returns an int. We're definitely returning an array of ints, so it should be int[].

> And those line numbers don't reference the current version of the code. 197 is blank line and 162 is:

Weird!

> It's a kernel test, so I don't think there's a problem with running a real query.

The thing I meant that we could mock is the time service, so we change the site's present time to a future timestamp, instead of hacking the time on the license entities. Same net result.

New patch with those couple of things addressed. Joachim, I think you might have tested against the wrong patch or maybe you have an old file hanging out. There are no more occurrences of "Drupal\commerce_license\LicenseStorage::getLicenseIdsToExpire()" in the patch anywhere.

Let's try this one!

One of these days I'll remember to set the status and post the file in the same response...

Very quick and incomplete review:

1. +++ b/src/Cron.php
   @@ -0,0 +1,93 @@
   +    $query = \Drupal::entityQuery('commerce_license')
   

   Get the query from the entity type manager.

2. +++ b/tests/src/Kernel/LicenseCronExpiryTest.php
   @@ -0,0 +1,351 @@
   +   * Returns a fresh copy of commerce_license storage.
   +   *
   +   * Clear the entity handlers cached in the entity type manager, so that the
   +   * license storage handler gets current data.
   +   *
   +   * (This clears all the entity type definitions too, which we don't need;
   +   * see https://www.drupal.org/node/2902487.)
   +   *
   +   * @return \Drupal\Core\Entity\EntityStorageInterface
   +   *   A fresh copy of commerce_license storage.
   +   */
   +  protected function freshLicenseStorage() {
   

   I don't think we need this. Just use reloadEntity() -- it's in the test base class in core.

Fixed and fixed!

I thought this was my failure - the failure is happening in LicenseSetExpiryTest.php, not in LicenseCronExpiryTest.php, which is the one I did.

Weird. LicenseSetExpiryTest is passing locally for me.

+++ b/src/Cron.php
@@ -0,0 +1,94 @@
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static(
+      $container->get('entity_type.manager'),
+      $container->get('datetime.time')
+    );
+  }

Services don't have a static create() -- that's for plugins & forms.

I'll reroll this later today.

I patterned that cron class off of the one in commerce_recurring. It has the same static create() method. Should I do a patch for both of them?

Here's a new patch.

Kernel tests all pass locally.

Changes from last patch:

- don't need create method
- fail job if license no longer active.
- rename the queue to be more specific
- removed unused constant in test

> I patterned that cron class off of the one in commerce_recurring. It has the same static create() method. Should I do a patch for both of them?

Yes, do a patch for that one too if you have the time :)

Sure, I can do that.

> PHP Fatal error: Class 'Drupal\recurring_period\Plugin\RecurringPeriod\RecurringPeriodBase' not found in /var/www/html/modules/contrib/commerce_license/tests/modules/commerce_license_set_expiry_test/src/Plugin/RecurringPeriod/CommerceLicenseSetExpiryTest.php on line 14

Are we missing a dependency? But then how come the test passes locally?

Test module is missing the dependency on the plugin type provider module... could that be it?

According to https://www.drupal.org/docs/8/phpunit/running-phpunit-tests ("Tests pass locally but fail when run by drupal.org's testbot")

commerce_license.info.yml needs

test_dependencies:
 - drupal:recurring_period

added

I got slammed at work and didn't have time to come back to this. At the latest, I can make this a goal for sprinting at Midcamp on Sunday. I have that patch for commerce_recurring to do, too.

Here we go!
Fixed the advancedqueue storage name vs job name, put in the test dependency.

It didn't apply. Trying again.

Changed info.yml diff

Revising patch for current dev branch.

advanced_queue storage was renamed but not everywhere. Renaming everywhere.

Renamed queue really everywhere.

I finally got my local environment to apply the patch and get the testing error that CI is giving us. Fixed!

These info files are killing me!

Wow!

I still don't understand what made the tests finally start passing -- the diff between patches 66 and 85 is this:

diff --git a/commerce_license.info.yml b/commerce_license.info.yml
index cfaf8dc..616bfdc 100644
--- a/commerce_license.info.yml
+++ b/commerce_license.info.yml
@@ -11,7 +11,6 @@ dependencies:
   - commerce:commerce_product
   - state_machine
   - recurring_period
-  - advancedqueue
 test_dependencies:
   - advancedqueue
   - recurring_period
diff --git a/config/install/advancedqueue.advancedqueue_queue.commerce_license.yml b/config/install/advancedqueue.advancedqueue_queue.commerce_license.yml
new file mode 100644
index 0000000..9517cf3
--- /dev/null
+++ b/config/install/advancedqueue.advancedqueue_queue.commerce_license.yml
@@ -0,0 +1,7 @@
+id: commerce_license
+label: 'Commerce License'
+backend: database
+backend_configuration: {}
+processor: cron
+processing_time: 180
+locked: true
diff --git a/config/install/advancedqueue.advancedqueue_queue.commerce_license_expire.yml b/config/install/advancedqueue.advancedqueue_queue.commerce_license_expire.yml
deleted file mode 100644
index cb35907..0000000
--- a/config/install/advancedqueue.advancedqueue_queue.commerce_license_expire.yml
+++ /dev/null
@@ -1,7 +0,0 @@
-id: commerce_license_expire
-label: 'Commerce License expiry'
-backend: database
-backend_configuration: {}
-processor: cron
-processing_time: 180
-locked: true
diff --git a/src/Cron.php b/src/Cron.php
index aae5350..c8b2e9d 100644
--- a/src/Cron.php
+++ b/src/Cron.php
@@ -49,7 +49,7 @@ class Cron implements CronInterface {
     if ($license_ids) {
       $queue_storage = $this->entityTypeManager->getStorage('advancedqueue_queue');
       /** @var \Drupal\advancedqueue\Entity\QueueInterface $queue */
-      $queue = $queue_storage->load('commerce_license_expire');
+      $queue = $queue_storage->load('commerce_license');
       foreach ($license_ids as $license_id) {
         // Create a job and queue each one up.
         $expire_remove_roles_job = Job::create('commerce_license_expire', [
diff --git a/tests/src/Functional/RoleGrantedLockingTest.php b/tests/src/Functional/RoleGrantedLockingTest.php
index a775e56..1ac3184 100644
--- a/tests/src/Functional/RoleGrantedLockingTest.php
+++ b/tests/src/Functional/RoleGrantedLockingTest.php
@@ -19,6 +19,7 @@ class RoleGrantedLockingTest extends BrowserTestBase {
   public static $modules = [
     'system',
     'user',
+    'advancedqueue',
     'recurring_period',
     'commerce_license',
   ];

Though I see from that in my haste to commit this I didn't spot that the patch is removing the dependency on AQ, which should be in. I'll do a quick clean-up for that.

Thanks everyone for all your work on this, and for not giving up! :)