Problem/Motivation

When records with dud emails like john@gmail or john@localhost are in the queue these records keeps on clogging it.
It would be great to set an option where the user can assign a retry amount.

Proposed resolution

Instead of just using an exception, which causes drupal to try again; We can Add $message[fail_count]
Add logic if fail count matches we skip that from creating a record at record _queue_mail_get_queue()->createItem($message);

Remaining tasks

Review patch

User interface changes

Could add a field to set retry limit

API changes

none

Data model changes

none

CommentFileSizeAuthor
#14 3000104-14.patch4.5 KBlarowlan
#14 3000104-14.interdiff.txt1.55 KBlarowlan
#12 3000104-retry-limit-pacth-11.patch4.42 KBpasan.gamage
#10 3000104-retry-limit-pacth-10.patch4.98 KBpasan.gamage
#7 3000104-retry-limit-pacth-7.patch4.98 KBpasan.gamage
#4 3000104-retry-limit-pacth-3.patch5.01 KBpasan.gamage
#2 3000104-retry-limit-pacth-1.patch4.83 KBpasan.gamage

Comments

pasan.gamage created an issue. See original summary.

pasan.gamage’s picture

pasan.gamage commented
Issue summary: View changes
StatusFileSize
new 3000104-retry-limit-pacth-1.patch4.83 KB
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs work 
  1. +++ b/config/schema/queue_mail.schema.yml
@@ -8,3 +8,9 @@ queue_mail.settings:
+    treshold:

    typo: threshold 

  2. +++ b/src/Form/QueueMailSettingsForm.php
@@ -89,6 +89,20 @@ class QueueMailSettingsForm extends ConfigFormBase {
+    $form['threshold'] = array(
...
+    $form['requeue_interval'] = array(

    should use [] short array syntax 

  3. +++ b/src/Plugin/QueueWorker/SendMailQueueWorker.php
@@ -19,6 +20,14 @@ class SendMailQueueWorker extends QueueWorkerBase {
+    // In case there is only one item in queue, lets make sure it's been
+    // carried on to next cron run.

    this comment is kind of off.

    Suggest:

    Prevent retrying until specified interval has elapsed 

  4. +++ b/src/Plugin/QueueWorker/SendMailQueueWorker.php
@@ -78,14 +87,37 @@ class SendMailQueueWorker extends QueueWorkerBase {
-          ->error('Error sending email (from %from to %to with reply-to %reply).', array(
+          ->error(
+            'Error sending email (from %from to %to with reply-to %reply).',
+            array(

    out of scope change? 

  5. +++ b/src/Plugin/QueueWorker/SendMailQueueWorker.php
@@ -78,14 +87,37 @@ class SendMailQueueWorker extends QueueWorkerBase {
+          \Drupal::queue('queue_mail', TRUE)->createItem($message);

    Could use a comment here.

    Something like:

    Add back to the queue with an updated fail count. 

  6. +++ b/src/Plugin/QueueWorker/SendMailQueueWorker.php
@@ -78,14 +87,37 @@ class SendMailQueueWorker extends QueueWorkerBase {
+              'Attempt sending email (from %from to %to with reply-to %reply) ¶
+              exceeded retry threshold and was deleted.',

    no need for a break here, it will throw out the log 

  7. +++ b/src/Plugin/QueueWorker/SendMailQueueWorker.php
@@ -78,14 +87,37 @@ class SendMailQueueWorker extends QueueWorkerBase {
+              array(

    use short array syntax here (I realise the module does not, but we shouldn't introduce new issues)

pasan.gamage’s picture

pasan.gamage commented
StatusFileSize
new 3000104-retry-limit-pacth-3.patch5.01 KB

Included a patch with
- Adds post update for existing users.
- Code review fixes

pasan.gamage’s picture

pasan.gamage commented
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Needs work 
  1. +++ b/queue_mail.post_update.php
@@ -0,0 +1,20 @@
+ * Adds post update hooks with the new config fields for existing users.

    This should just be 'Contains post update hooks for the module' 

  2. +++ b/queue_mail.post_update.php
@@ -0,0 +1,20 @@
+function queue_mailer_post_update_config_fields() {

    The module is named queue_mail, so this won't work - remove the `er` from the function name

pasan.gamage’s picture

pasan.gamage commented
StatusFileSize
new 3000104-retry-limit-pacth-7.patch4.98 KB
pasan.gamage’s picture

pasan.gamage commented
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

This looks good to me, thanks @pasan.gamage

pasan.gamage’s picture

pasan.gamage commented
StatusFileSize
new 3000104-retry-limit-pacth-10.patch4.98 KB

Thanks @larowlan for pointing out the operator bug :)
Attached is the fix.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/src/Plugin/QueueWorker/SendMailQueueWorker.php
@@ -19,6 +20,13 @@ class SendMailQueueWorker extends QueueWorkerBase {
+      throw new RequeueException();

requeue will keep it at the top of the queue.

We need


\Drupal::queue('queue_mail', TRUE)->createItem($message);
      return;

instead

pasan.gamage’s picture

pasan.gamage commented
StatusFileSize
new 3000104-retry-limit-pacth-11.patch4.42 KB
pasan.gamage’s picture

pasan.gamage commented
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
StatusFileSize
new 3000104-14.interdiff.txt1.55 KB
new 3000104-14.patch4.5 KB

  • sinn committed 8deb210 on 8.x-1.x 
    Issue #3000104: Support a retry limit
  • sinn committed b6314c3 on 8.x-1.x authored by larowlan
    Issue #3000104 by pasan.gamage, larowlan: Support a retry limit
sinn’s picture

sinn commented
Status: Needs review » Fixed
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Is there a chance of a new release containing this?