Use Batch API to send mails

skybow, great timing. Batch API is on my list to investigate. It is not in simplenews because is was never built. Please give it a try using 6.x-2.x-dev.

As far as i understand the code, currently something like a queueu is used. Instead Perhaps it would be also posof implementing your own queue system you could also use http://drupal.org/project/drupal_queue

I was wondering today about this. If you need help testing, I'm up to it!

dereine, thanks for the tip. But my priority is with the 7.x upgrade. So I will use it there first.

I'm also for using the batch API in foreground sends.

In addition we've checked the queue system for the send process.
The API design of the queue seems to be not well designed for the simplenews case.
In all implementation cases we tried to think of, the simplenews code became more complex, cluttered, less efficient.

Subscribing to this issue, while changing the target version, as no new features will be implemented in 6.x-1.x.

You may not need Simplenews at all to send a bulk email. You just add Send email action to a view of users via VBO, and you select the users you want to send mail, and press Send :)

A good idea for 7.x-1.x?

It's that easy.

The only thing that is necessary to do is calling simplenews_mail_spool N times (N = ceil($spool_count / $throttle_amount)) followed by a clear spool and update status and then batch api is taking care of the rest. It goes through the mail spool so if anything goes wrong, the mails will be sent on cron just like before.

Tests would be nice for this, some documentation updates and probably also calling it on the newsletter overview page. The problem there is that the function could possibly be called multiple times ( if you send ultiple newsletters at once), so it would need to be redesigned a bit. e.g. not doing anything in add_to_spool() and then add a function that set's up $batch for everything that is currently in the spool.

Re-rolled, can't reproduce the test fail locally, let's try again.

Ok, this patch does it a bit differently so that it is also possible to send multiple newsletter without breaking the API.

Basically, I moved the batch stuff in a separate function and added an additional argument to simplenews_add_node_to_spool() to override the default send behaviour (which is based on the variable). This allows to either always send or never send, even if Simplenews is configured otherwise. The form callbacks now set it to FALSE and then check the variable themself and call the batch function manually. The advantage of having a separate API function for this is that it would be very easy to add something like "There are currently x mails in the mail spool. [Send now]", either in this module or another one.

This isn't a very clean API, especially due to the direct variable_get() call, but it's possible to implement this without changing the API. Alternatively, we could remove the send stuff completely from the API and provide an additional function, something like simplenews_mail_attempt_immediate_send($use_batch = FALSE) that would send immediatly if configured to do so.

What do you think?

Additionally, I also refactored the count_spool() function to allow counting the whole spool and properly use the query builder API properly.

I think there's no need to try to have a stable API. Ppl usually don't use these functions anyway and we're still alpha..

The rest of the things sound pretty fine. But i didn't fully understand the first decision / reasons.

One general thing i'd want to understand is a concurrency situation.
- What happens if two independent ppl create send SEND a newsletter (on the same category)?
- What happens if two independent ppl create send SEND a newsletter (on different categories)?
- What happens with old items already in spool? (i don't think user Y should continue the batch of user X...)

Yeah, I guess so, since I've renamed that function anyway already.

The current mail spool API does not provide a way to only send spool entries of a single node, category or multiple (but specific) nodes/categories.

That means that it will attempt to send the complete mail spool every time the batch send function is called. This is not something that is introduced by this patch nor should it imho be fixed here (We could only argue that we should fix that first).

Also, if a second person starts a send process while another one is already running, the function will calculate the number of necessary simplenews_mail_spool() calls incorrectly (e.g. 50% of the currently being sent newsletter *and* the new newsletter). This isn't very problematic, because what happens is this:

1. Because there are two parallel invocations of the batch API, the spool will be sent faster than expected, so one or both of the batches could call simplenews_mail_spool() more often than necessary.
2. The function simply attempts to get a number of mail spool rows to send, will not find any and will stop the processing. So it's similar to a hook_cron() call if there is no newsletter to send.
3. Thanks to the locking, there will be no duplicated newsletter or similar issues.

The only thing that I'm not 100% sure is if the two final calls to delete old mail spool rows and update the newsletter status could result in a race condition, but this is not any different to the current code.

To fix this, we'd need to add support for a $conditions array to both simplenews_mail_spool() and simplenews_get_spool().

Something that would need to be implemented is the current Simplenews Scheduler patch, which *does* use this API :)

#1056388: Port to Drupal 7

Ok, here is the patch *with* API changes, as discussed today.

- Basically, instead of different kinds of $status and/or $nid (or none) arguments, all relevant functions (..._mail_spool(), ..._get_spool(), ..._count_spool()) now have that unified into a single $conditions array. This allows to send the spool of a single node, a number of nodes, of a specific category, ...

- Cleaned up some more db_select() code in get_spool(), dropped custom query building with a db_or().

- Renamed the send_batch() function to simplenews_mail_attempt_immediate_send(), which has a $use_batch = TRUE argument.

There are currently three test fails, which happens because the send on publish doesn't immediatly send anymore right now.

We have two options there:

1. Try to send immediately without using batch (Downside is that things could explode if you try to send a newsletter to too many recipients => current situation)
2. Do not send immediately (Might be an undesired behavior but the most save one. Would also require a one-line change in the tests).

(3. Attempt to send using batch. Not an option because the publishing might happen on cron or in another way that is not triggered by a UI. That's the whole point of this feature...)

Uploading the patch would be useful...

And now against 7.x-1.x, the above patch was against an older commit.

Great stuff.

Since send on publish is a typical background triggered task, relying on cron to actually send out is generally fine.
There might be a delay of one cron step, if cron slot 1 publishes...
Possibly you should make sure that hook_cron of simplenews runs after your publishing cron.

If however the publishing condition is triggered by a foreground task, it's like switching to "send" when cron mode is enabled.
I see nothing negative with that.
Immediate foreground send is a special case that was introduced for direct/active UI only unconditional "send now" tasks.

Ready to commit then after test change.

Ok, fixed the tests and commited.