Allow for peaceful requeueing

Can we get a test for this, we probably already have something that throws the other exception

But love it

+++ b/core/modules/system/src/Tests/System/CronQueueTest.php
@@ -61,5 +61,12 @@ public function testExceptions() {
+    $this->assertEqual(\Drupal::state()->get('cron_queue_test_requeue_exception'), 1);

Can you also assert that the queue size remains at 1?

Ta

Also, I think we need some docs updates - and possibly a change notice.

unless bot disagrees I think this is ready

I'm not really keen on using exceptions for control flow but the idea works.

1. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -168,6 +169,10 @@ protected function processQueues() {
   +          catch (RequeueException $e) {
   +            // This item needs more time to be processed.
   +            $queue->releaseItem($item);
   

   Lets at least change the wording around the exception. The comments about "needs more time" imply things about why the exception was thrown when it could be anything. A resource isn't available yet, it wants to hand off to a different worker, the third moon of Jupiter was out of alignment, the task should only be run on Mondays...

   Lets just say "the worker requested the task be immediately requeued" or something along those lines.

   Similar changes to the change log.

2. +++ b/core/lib/Drupal/Core/Queue/RequeueException.php
   @@ -0,0 +1,14 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Queue\RequeueException.
   + */
   +
   +
   +namespace Drupal\Core\Queue;
   

   Extra line before namespace.

@neclimdul isn't that the reason why PHP has Exception types, so that you can handle them conditionally if they occur? This patch looks good to me. If this was the pre-Drupal 8 era, I would suggest that a change notice be written for it as it sounds like a nifty idea that developers would want to know more about.

Edit: my bad, you've already got the change noticed handled.

@neclimdul isn't that the reason why PHP has Exception types, so that you can handle them conditionally if they occur?

Well it does, but they should really be used for errors (which might have different types), whereas this is not an error, it's just "Not right now thanks".

I opened #2670866: Add return values to QueueWorkerInterface::processItem() instead of using exceptions for code flow to tackle that in 9.x, but I really don't see a way to do it in a bc-compatible way, so exceptions seems like the only choice here.

Committed/pushed to 8.1.x, thanks!

Right, thanks catch. What I try to emphasis when using exceptions is that exceptions should be exceptional. Typed exceptions exist so you can handle different exceptional cases. That means there can be different flows based on the exception but they should not be used for flow.

The way I think of it is this, if you are developing a feature you expect the users to use, you should not be using exceptions. If you are recovering from something —an error or failure in state— then exceptions is appropriate.

I'm not convinced passes that litmus and so should not be handled by exceptions. The API around the return type is exactly why I hesitated though, thanks catch for opening the follow up. This seems like the right approach so we can get the feature in and being used but can develop the right solution.

Thanks for taking my concerns to heart!

So thinking this though, the BC seems to cover the item runner, but I wonder about BC with worker threads. The API goes both ways of course. So if a similar implementation existed in Drush and you run your queue there, if it doesn't handle the return it would normally delete the item since an exception wasn't thrown. Then the item is lost rather then being completed correct?

So if a similar implementation existed in Drush and you run your queue there, if it doesn't handle the return it would normally delete the item since an exception wasn't thrown. Then the item is lost rather then being completed correct?

This is why I just committed the patch with the 9.x follow-up, couldn't think of a way to handle bc in both directions.

We can add __drupal_queue_api_version to $data passed to processItem and then the worker can decide whether to suspend the queue or to immediately requeue.

That could be an option though yes. It will be extra code everywhere, but allow us to add the new return types then remove the bc stuff later which is all you can ask for.

Theoretical new return types we might add other than this one in the future that aren't errors. For example we might want to distinguish between 'keep my place at the front of the queue' vs. 'send me to the back of the queue'.

Right we can't guarantee that a backend supports ordering but I think "don't delete" vs. "recreate" might be fine despite that.

This is already in, so we should open a new issue for any follow-up.