[META] Problems with RecipientHandler

Change interface to be RecipientHandlerInterface::buildRecipientList($handler_settings) returning array of email addresses.

There may be non-trivial performance issues here. An array of 100k email addresses would consume a fair amount of RAM?

I suspect you'd need to use a queue task with pagination, which is a lot of complexity when 99.9% of use cases can be made to work with queries.

A simpler solution to allow other modules to get recipients from sources other than queries would be to devolve more responsibility from the spool storage service to the recipient handler. We could add an addToSpoolFromEntity($newsletter) method to the recipient handler with code like that currently in the spool storage service. With some fancy footwork, this could be 100% BC.

Usability: handler and settings are specified on the simplenews_issue field, but it feels like it should be on the newsletter. 1) Conceptually, the newsletter defines the set of people to send to - currently by subscriber records, but in future it could be by something else such as user entities. 2) Roles: the newsletter editor (maybe not a techie) picks which newsletter to send to; the site admin configures the settings of how this maps to a set of email addresses. True, possibly some of the settings could be exposed to the newsletter admin. Basically it seems like a view that could have exposed filters

Remove ->handler and ->handler_settings from simplenews_issue and add them to Newsletter entity.

tldr; this looks like a mistake, newsletters make sense as they are currently, pots of stored subscriber emails. Recipients is a different concept to Newsletters that could use Newsletters but also work with completely different sources of emails.

THOUGHT ONE:

If we were doing this from scratch we might use a plugin deriver:
* A derivative plugin recipient handler could be generated for each Newsletter entity.
* The total set of available recipient handler plugins would then be these derivatives, plus whatever other custom/contrib ones a project has.
* The sitebuilder could select on the settings for the simplenews_issue field which plugins are available for that instance of the field.
* If multiple plugins are made available by the sitebuilder, then the content editor can select the one they want.

THOUGHT TWO:

But the current field is an entity reference field and the BC issues are thorny. I think your proposal though is maybe closer to being perfectly BC than you realise:
- Allow a handler to be set on the newsletter entity (no UI even needed at first, it's an advanced use case)
- Make simplenews_all the default handler if none is explicitly specified on a newsletter
- in the spool storage service, get the handler set on the newsletter, unless one is specified in the field in which case get that
- stop specifying simplenews_all as the default handler for the field

In my use case I would create a new newsletter called "Custom", set a "myproject_custom" handler on it, and make it the only available newsletter on the field. In my handler, I would build the recipient query using criteria derived from other fields on the node.

THOUGHT THREE:

I think you're going in a fundamentally wrong direction here. This isn't right:

Conceptually, the newsletter defines the set of people to send to - currently by subscriber records, but in future it could be by something else such as user entities

I'd suggest the Newsletter does not define a set of people to send to.
Rather it defines a pot of email addresses, a pot that people have likely asked to have their email added to.
It's linked intrinsically to subscriptions.

As you rightly say, it would be great to allow sending to sets of people other than subscribers. But there's still a need in most cases for these pots of subscribers to exist, and managing them has its own set of complex issues. The newsletter config entities handle this well.

I'd suggest the need is to reduce the entanglement between nodes and Newsletters so that it becomes easier to use simplenews with recipients that don't use Newsletters at all.

Employee newsletters could be a good example here: they're all users, and (un)subscribing is not a relevant concept. Moving the handler to the Newsletter doesn't help implement this at all, it only entangles things.

Bug: simplenews_count_subscriptions ignores RecipientHandlerInterface::count()

If I'm right above, that's not a bug. The so called "Newsletters" are really what in other systems are called "Email lists". There are plenty of valid reasons to want to count the number of current subscribers to a list.

What's a bug is that we call simplenews_count_subscriptions from the NodeTabForm and the SpoolStorage service. When we have a node, we should get the handler and invoke RecipientHandlerInterface::count(), instead of assuming that the node is sending to a single Newsletter and getting the subscriber count of that.

Restriction: forced to be based around subscribers as the spool has a mandatory snid (simplenews_mail_spool snid is "not NULL"). Cannot implement "all users with role XX".

The default value of snid is zero. I wonder what harm it does if (when using non-subscriber recipients like users) the handler leaves the snid as zero for every inserted row.

Restriction: forced to be based on SQL query, which is static (cannot depend on handler settings).

I don't see what you mean. RecipientHandlerInterface::buildRecipientQuery() is not a static method, though I'm sure that's not literally what you meant.

We already provide the handler settings to the recipient handler plugin in its configuration array (see SpoolStorage::addFromEntity() and simplenews_get_recipient_handler()), so buildRecipientQuery() could use them to alter the query based on the handler settings?

Bug: unsubscribe links likely won't work or make sense

Yeah, that's a tricky one.

The spool already has a data field, so we can add arbitrary data to each row in the spool from the recipient handler. Once it's there, if we can get it into the MailEntity then we can get it into the render array, where templates etc. can react to however they want to. This allows people to provide whatever explanations and unsubscribe links or instructions they need to for their use case.

If we also added a method like "buildRecipientFooter()" to a new interface on the recipient handler plugin, then we could encapsulate the unsubscribe footer creation in the same place as the rest of the recipient logic, which would be neat. There are some warnings in the current template about caching getting complicated with personalised content, so there might be limits on what one can do with this just from the recipient handler without other fixes.

I'd suggest the need is to reduce the entanglement between nodes and Newsletters so that it becomes easier to use simplenews with recipients that don't use Newsletters at all.

This seems to be by far the biggest place where our viewpoints differ ...
I think you are missing the fact that Newsletter also other jobs. ...
A newsletter is a thread of continuity that links configuration and a set of mails. The job of storing subscriptions could be seen as a secondary function of newsletters.

Very interesting answer Adam, you know this module much better than me. Now I follow your clues I can see how vital having a newsletter is. That has a big impact on how I think about #2556441: Sending an item to multiple newsletters.

While in my mind it might be ideal for the job of storing subscriptions to belong to some other "email list" entity, it doesn't seem particularly problematic to give the Newsletter entity this double functionality.

If you want to move the handlers to the newsletter, my thought two above describes a BC approach to that. But I don't see what we really gain by doing this. Who really needs it?

we need to consider it also from the perspective of SpoolStorage for #2556441: Sending an item to multiple newsletters combining the results of multiple queries and eliminating duplicates.

Yes, duplicate management is an issue on my mind too. Maybe that's better discussed on the other issue.

There may be non-trivial performance issues here. An array of 100k email addresses would consume a fair amount of RAM?

Well maybe not? Estimate 30 bytes per email address = 3MB plus some storage overheads but hopefully not more than double?

Anyway if you think queries are OK, then it's fine by me.

Your byte maths seems good to me, and I prefer the flexibility of raw emails. However, I don't see a significant upside to raw emails that's worth the effort especially given the BC issues.
Queries may help with doing deduplication in a performant way, though I'm not strong on them either.

Bug: unsubscribe links likely won't work or make sense

I don't think it's especially tricky. Your ideas here seem very complicated ... the templates already have access to newsletter_id and from that can work out whether to skip unsubscribe links and perhaps add some explanatory text instead

You're assuming that knowing the newsletter id is enough information to figure out what link/message to show. But if someone has a very dynamic recipient handler that is selecting people for all kinds of odd reasons, simply knowing the newsletter id may not be enough to figure out what to show. Hence the possible need to pass information using the spool's data field, about why the recipient is being sent the mail. I'll open another issue for it.

*****************

I've noticed that SpoolList::nextMail tries to load a subscriber by mail address, because MailEntity expects a subscriber. So recipient handlers that work naively with users should fail miserably.

We can work round this easily by creating a subscriber for every user, but that's a bit of a kludge.

Adam asked me to note here that as I posted in #2556441: Sending an item to multiple newsletters in my project I plan to work round what's possibly the hardest challenge here ("Restriction: assumption that mail spool entries are subscriber entity, whereas they could be user entity or something else") by using the decoupled_auth module (which allows users without login credentials) and then using hook_entity_insert to create a user for every subscriber and a subscriber for every user.