Problem with recipient handlers

Yep, doesn't need to be static IMHO.

Having the settings in the class constructor means that sending a node with different settings needs a different constructor.

So my suggestion would be to move $settings from the constructor to the method.

public function __construct($newsletter, $handler);
public function addToSpool($node, $settings)
// And then later.
public function addToSpool($entity_type, $entity, $settings)

Or we could even move $newsletter too?

Having the $settings as a separate argument means that the implementations of that interface do not need to know how it is stored, even if it would be part of $node.

Hm, good point.

Well, the problem is that $handler and $settings does come from the node.

So it's a bit weird parts of that are passed to the constructor and parts if to the method.

There are very few cases where we need multiple handlers for different nodes (sending of multiple newsletters at once), so what about:

$handler = simplenews_get_recipient_handler($node);
$handler->addtoSpool();

The helper method would extract the necessary values from the node and pass it all to the constructor of the handler.
Creating an object and passing some values in isn't that costly and if we end up doing it many times, we can still add a per-node static cache.

I think this is covered in a more up-to-date way in #3054695: Problem with recipient handler interface and spooling. In particular note that in D8, the current implementation of the add to spool logic bypasses the entity query interface so we need a more radical rethink.

@AdamPS explained his thoughts about this in #3054695: Problem with recipient handler interface and spooling #16:

We can optimise by removing the mail field from the spool and just relying on snid. That means that it will often be possible to use an entity query to return the snid values, without the need to load all the nodes to find the value of the mail field. As a bonus this solves a current bug that subscriber lookup fails because a user changed their mail.

It does mean we would lose #3054693: Allow mail spool entries that are not subscriber entity. However it's easy to create a subscriber for every user, which likely covers 90% of cases.

There's a lot going on this patch beyond simply adding a new method to the Handler, and it seems to break BC in multiple ways.

I'd suggest that removing the mail field is a separate issue? (I also don't yet understand the need.)

To maintain BC while keeping DRY here we could:

1. Create a new SpoolingRecipientHandlerInterface with method addToSpool($query).
2. In RecipientHandlerBase put a public static method spoolFromQuery($query) and place current spool insertion code on it
3. In spoolstorage::addFromEntity() call addToSpool() if the handler implements SpoolingRecipientHandlerInterface, else fall back to calling RecipientHandlerBase::spoolFromQuery for BC.
4. Have RecipientHandlerBase implement SpoolingRecipientHandlerInterface
5. Have RecipientHandlerBase::addToSpool() call statically RecipientHandlerBase::spoolFromQuery

+++ b/src/Plugin/simplenews/RecipientHandler/RecipientHandlerBase.php
@@ -36,36 +34,51 @@ class RecipientHandlerBase extends PluginBase implements RecipientHandlerInterfa
-    $select->condition('t.subscriptions_target_id', $this->newsletter->id());

We seem to have lost the code that filters the query to only subscribers of the current newsletter

@jonathanshaw Thanks for the comment. Yes this is a rough hack proof of concept!

#11 You are right

#10
NB I don't remove the mail field I just stop using it. The reason was in a comment in the other issue #3054695: Problem with recipient handler interface and spooling #16.

We can optimise by removing the mail field from the spool and just relying on snid. That means that it will often be possible to use an entity query to return the snid values, without the need to load all the nodes to find the value of the mail field. As a bonus this solves a current bug that subscriber lookup fails because a user changed their mail.

Yes this isn't BC. The module is contrib and beta - it's not core stable. The existing interface is very limited and keeping BC will add complexity/confusion to the code and take more time (your suggestions seem broadly correct but there are further problems you didn't mention). Simplenews fixes have grown hugely beyond my plans and I don't have spare time. My guess is that hardly anyone implements RecipientHandlerInterface. For those that do, it's a fairly easy tidy up and they can copy the changes in RecipientHandlerBase.

@Berdir - any objection to non-BC? Would you prefer I start a new branch? It feels overkill to me but I could.

1. +++ b/src/Spool/SpoolList.php
   @@ -65,13 +65,7 @@ class SpoolList implements SpoolListInterface {
   -    else {
   -      $subscriber = simplenews_subscriber_load_by_mail($spool_data->mail);
   -    }
   -
   +    $subscriber = simplenews_subscriber_load($spool_data->snid);
   

   How about loading from snid if it's not empty, falling back to loading from mail if snid is empty. That solves performance and change of address concerns, but may keep a little flexibility for some use cases.

2. +++ b/src/Spool/SpoolStorage.php
   @@ -269,14 +250,12 @@ class SpoolStorage implements SpoolStorageInterface {
   -        'mail' => $spool['mail'],
   ...
   -        'data' => serialize($spool['data']),
   

   I'm not sure what the gain is in touching these

Thanks @jonathanshaw. I will answer those points, but please after that I think it's best if you don't review any more until I produce a better patch.

1.

How about loading from snid if it's not empty, falling back to loading from mail if snid is empty. That solves performance and change of address concerns, but may keep a little flexibility for some use cases.

If someone has filled in mail but not snid then it has not solved change of address concerns.

keep a little flexibility for some use cases.

I am in favour of flexibility when it comes to "don't restrict options" but not when it means "allow people to do the same thing in multiple ways some of which are buggy". This seems like the second case. If someone has a subscriber then why can't they put the snid and avoid the bug. If they don't then filling in mail won't help - they need to fill in data.

2. I now plan to keep data. I still prefer to remove mail for the reason above. It serves no purposes, encourages people to use the interface wrongly and will occasionally contain an inaccurate value.

OK now ready for review please.

RecipientHandlerNewUsers is there as an example in case you want to try it. I won't check it in like this, but instead move that class to the demo or tests. However I hope you will agree it's pretty neat that you only have to write 3 lines of code.

Nice! As you say, the simplicity of the entity query implementation is lovely. If we're doing this, I suggest considering in a follow-up the possibility of automatically create a subscriber for each user.

We're hard breaking BC in 4 different ways here:
1) Removing the simplenews_get_recipient_handler() method
2) Altering RecipientHandlerInterface
3) Making RecipientHandlerBase no longer implement RecipientHandlerInterface, breaking anyone extending it
4) Altering SpoolStorageInterface

You've addressed RecipientHandlerInterface in #10/12 above. I'll discuss the others below, in case you think them worth preventing:

1. +++ b/simplenews.install
   @@ -70,17 +64,22 @@ function simplenews_schema() {
   +        'not null' => FALSE,
   

   This seems like an unrelated change, and puzzling as you're moving away from allowing non-subscriber recipients.

2. +++ b/src/Plugin/simplenews/RecipientHandler/RecipientHandlerAll.php
   @@ -0,0 +1,36 @@
   diff --git a/src/Plugin/simplenews/RecipientHandler/RecipientHandlerBase.php b/src/Plugin/simplenews/RecipientHandler/RecipientHandlerBase.php
   

   I have the impression that traits are thought of as a better solution for code reuse, and a hierarchy of base classes might be a code smell. Maybe this should be a trait.

   Another question is BC. The fact that the current version is called RecipientHandlerBase would have suggested to other people that they should extend it. But if they have done so, their code will now break as it will no longer implement RecipientHandlerInterface.

   We could both preserve BC for extending handlers, and avoid the layered base classes, if we:
   a) created a RecipientHandlerTrait with the code you proposed to put in RecipientHandlerBase
   b) put in RecipientHandlerBase the code you proposed to put in RecipientHandlerSelectBase
   c) Don't have a RecipientHandlerSelectBase

3. +++ b/simplenews.module
   @@ -735,24 +735,6 @@ function simplenews_mail($key, &$message, $params) {
   -function simplenews_get_recipient_handler($newsletter, $handler, $configuration = array()) {
   

   You could deprecate this instead of removing it if you cared about making it BC. But definitely needs to die sometime! Unlikely to be used by anyone outside I would have thought.

4. +++ b/src/Spool/SpoolStorage.php
   @@ -264,23 +250,29 @@ class SpoolStorage implements SpoolStorageInterface {
   +  public function getRecipientHandler(ContentEntityInterface $issue) {
   

   I'm not sure why this needs to be public.

5. +++ b/src/Spool/SpoolStorageInterface.php
   @@ -128,6 +128,19 @@ interface SpoolStorageInterface {
   +  function getRecipientHandler(ContentEntityInterface $issue);
   

   Adding this to the interface breaks any alternative implementations in theory, but it's likely that anyone making an alternative will extend the provided implementation so it's likely harmless. Whether it's necessary, I'm not sure.

Thanks @jonathanshaw. I feel you are still aiming for a higher level of BC than is required given it's contrib and a beta. For sure we shouldn't break sites that have no customisations/hooks. However in a few cases, the scale of necessary changes are large and it's too restrictive to avoid any change to interfaces.

1. Unavoidable unfortunately as the interface is wrong. The $issue parameter is missing and so we can't create a recipient handler.
2. True, but we need to make large changes and to try to be BC would make it contorted and confusing for the future (I think with a significant risk that we fail anyway). I expect that the number of people who have implemented RecipientHandler to be small, and the disruption caused to be small. I'd rather be clear that we are changing the interface and ensure that people take a proper look at what code changes they need.
3. See 2).
4. For stable+core that would be true but this is beta+contrib. The idea of providing a base class is that it allows us to add methods without breaking derived implementations. If someone has their own implementation that does not extend the base then they are on their own! I have already added other methods to this interface in other issues.

I am willing to consider creating a new branch, but it seems like more work/disruption without really helping anyone. I have no interest in supporting two branches so I would then only apply fixes to the new branch. So people would still need to change their code.

1. I feel I recently grasped how the current code allows "non-subscriber" recipients. There must be a subscriber object, but it can be a temporary one that is not in the database but instead serialised in the data parameter. In particular I deduced that from this line:

      $subscriber = unserialize($spool_data->data);

I haven't tested it yet, but assuming it works it seems like a good idea. I'll report back.

2. The case for traits is when you want to reuse code without any common class hierarchy. However a hierarchy is preferred where possible, and it is extremely common in core, e.g. class Node extends EditorialContentEntityBase extends ContentEntityBase extends EntityBase.

RecipientHandlerBase is documented as a base for all recipient handlers, so it should not include any code specific to the particular implementation of select - especially as I see that implementation as a special case, breaking core interfaces.

I've made it as easy for people as I can: existing recipient handlers likely need to change to extend either RecipientHandlerSelectBase or RecipientHandlerAll.

3. See BC 1)

4. It is public for improved BC. I am removing simplenews_get_recipient_handler() so there should be a drop-in replacement.

5. See BC 4). This is following the global direction of D8 to use services rather than functions. Functions prevent customisation to alter the implementation, use dependency injection etc.

New patch fixes SimplenewsMonitoringTest. I think the other fail might be a random.

Please let me know if there are any further review comments.

Once we have completed code review I would like to commit the patch without tests as a first step. Second step will be to fix some of the related issues. Then at the end when it's all working I will write a new test file for recipient handler.

Had a very quick look, a lot going on here, will need some time to understand this and think about BC. An updated issue title and summary would be helpful to understand this :)

I'm happy with whatever you think best about BC.

1. +++ b/src/Plugin/simplenews/RecipientHandler/RecipientHandlerAll.php
   @@ -0,0 +1,36 @@
   +    $select->condition('s.status', SubscriberInterface::ACTIVE);
   
   +++ b/src/Plugin/simplenews/RecipientHandler/RecipientHandlerEntityBase.php
   @@ -0,0 +1,54 @@
   +  abstract protected function buildEntityQuery();
   

   I wonder about making this not abstract, but instead providing here an EntityQuery based implementation that already does some of the work. This might both provide an example to guide others, and help with some of the details that are a little obscure like
   + $select->condition('t.subscriptions_status', SIMPLENEWS_SUBSCRIPTION_STATUS_SUBSCRIBED);
   + $select->condition('s.status', SubscriberInterface::ACTIVE);

   Alternatively, perhaps the docblock could include a code example that achieved the same thing as the simplenews_all handler, but using EntityQuery instead.

2. +++ b/src/Plugin/simplenews/RecipientHandler/RecipientHandlerSelectBase.php
   @@ -0,0 +1,40 @@
   +   *   Query with the columns 'snid' and 'newsletter_id' for each recipient.
   

   Would it be better to handle newsletter_id as an expression instead of a query field? I suspect not, the way it is (which you've kept) allows the newsletter to vary per recipient which could be desired in some cases.

3. +++ b/src/Spool/SpoolList.php
   @@ -65,11 +65,11 @@ class SpoolList implements SpoolListInterface {
   -    if ($spool_data->data) {
   -      $subscriber = $spool_data->data;
   +    if (!empty($spool_data->data)) {
   +      $subscriber = unserialize($spool_data->data);
   

   +++ b/src/Spool/SpoolStorage.php
   @@ -109,12 +118,6 @@ class SpoolStorage implements SpoolStorageInterface {
   -        if (mb_strlen($message->data)) {
   -          $message->data = unserialize($message->data);
   -        }
   -        else {
   -          $message->data = simplenews_subscriber_load_by_mail($message->mail);
   -        }
   

   +++ b/src/Spool/SpoolStorage.php
   @@ -264,23 +250,32 @@ class SpoolStorage implements SpoolStorageInterface {
   +    if (isset($spool['data'])) {
   +      $spool['data'] = serialize($spool['data']);
   +    }
   

   These seem unrelated to the rest of this issue. I've no idea if they're safe or effective changes.

Responses to #25

1. I'm confused! The lines of code that you referenced seem to be from RecipientHandlerSelectBase but your text refers to RecipientHandlerEntityBase. RecipientHandlerEntityBase already is an implementation that does most of the work - the entire addToSpool function.

Alternatively, perhaps the docblock could include a code example that achieved the same thing as the simplenews_all handler, but using EntityQuery instead.

I would expect that if someone wants to tweak RecipientHandlerAll then they would override RecipientHandlerAll and add an extra condition.

My RecipientHandlerNewUsers seems to me like a more likely example of how people will user RecipientHandlerEntityBase. Rather than docblock I will put it into the simplenews_demo sub-module - that allows IDEs, auto-documentation etc to discover it in the class hierarchy.

2. I thought about that but decided not to because it would be a backwards step relating to #2556441: Sending an item to multiple newsletters.

3. For this issue we are changing to look up on 'msid' instead of 'mail'. The old code was muddled with two separate places in the code that call simplenews_subscriber_load_by_mail() whereas only one is needed. So I removed one call, and changed the other to simplenews_subscriber_load().

Response to #24: IS done. I raised a separate issue to discuss BC as it affects multiple issues in development: #3056958: Decide strategy for BC / branches.

Slightly update patch that prepares for multiple newsletters. I'm not necessarily planning to solve that issue soon, but the idea is to ensure that we can solve it without having to change the interface again.

I have also added a list of remaining tasks to the IS.

On the BC issue, I now favour creating a new branch, and would like to do it ASAP. Please can you comment on that issue to confirm/reject?

I changed my mind because I have drawn up a list of issues that aren't fully BC. I found that there are many parts that would be complex and disruptive to try and release in a "semi-BC" way. Thanks @jonathanshaw for continuing to press me to think about it!

I wonder about making this not abstract, but instead providing here an EntityQuery based implementation that already does some of the work. This might both provide an example to guide others, and help with some of the details that are a little obscure like
+ $select->condition('t.subscriptions_status', SIMPLENEWS_SUBSCRIPTION_STATUS_SUBSCRIBED);
+ $select->condition('s.status', SubscriberInterface::ACTIVE);

I would expect that if someone wants to tweak RecipientHandlerAll then they would override RecipientHandlerAll and add an extra condition.

My thought is that sometimes people might want to filter by something like newsletter subscribers who have a certain user role.

This is much easier using your nice new entity query (rather than just adding conditions to RecipientHandlerAll's select query). But doing it using an entity query involves understanding how one should properly treat these 2 (non obvious) status fields, so I was wondering if it would be nice to provide a base implementation or doc for that. Certainly not important.

OK here is a new patch that I would like to commit to a new 2.x branch. Here are the changes - basically fixed some more bugs and added better examples.

• Partially fixed recipient handler settings form (it's still broken, but not worth fixing further due to #3055850: Wrong location of recipient handler UI.
• Fixed sending using the data field and clarify meaning of that field.
• Created RecipientHandlerSubscribersByRole as requested in #30.
• Moved example class RecipientHandlerNewUsers into simplenews_demo sub-module.
• Written another example class to spool using the 'data' field.
• Optimise subscriber count by reusing known information.
• Created new class RecipientHandlerIdsBase because sometimes EntityQuery is not flexible enough - it can only examine data referenced directly from the subscriber entities.

I will write automatic tests later because we need some other fixes first. However it has been tested fairly well by hand.

Looking great Adam, some really good things emerging here. The following comments offered simply in the interest of thoroughness, to make sure you're sure:

1. +++ b/modules/simplenews_demo/src/Plugin/simplenews/RecipientHandler/RecipientHandlerSiteMail.php
   @@ -0,0 +1,39 @@
   +  public function addToSpool() {
   +    $subscriber = ['mail' => \Drupal::config('system.site')->get('mail')];
   +    $spool = [
   +      'entity_type' => $this->issue->getEntityTypeId(),
   +      'entity_id' => $this->issue->id(),
   +      'newsletter_id' => $this->getNewsletterId(),
   +      'data' => $subscriber,
   +    ];
   
   +++ b/src/Spool/SpoolStorage.php
   @@ -109,12 +118,6 @@ class SpoolStorage implements SpoolStorageInterface {
   -          $message->data = simplenews_subscriber_load_by_mail($message->mail);
   

   Amazing how easy it is to make a temporary subscriber in the recipient handler here.

   But it does make me wonder: if we're supporting this possibility, why not leave the mail column undeprecated, and automatically create a subscriber downstream if we have a mail but no data or snid?

   Insisting on snid and deprecating mail I could understand, but if snid remains optional and passing mail via data is allowed, we still have the problems of mail being a somewhat unreliable way of looking up a subscriber.

   Maybe the reason is simply that mail is an unnecessary convenience being removed for maintainability, as you can do with data anything you can do with mail.

2. +++ b/modules/simplenews_demo/src/Plugin/simplenews/RecipientHandler/RecipientHandlerSubscribersByRole.php
   @@ -0,0 +1,47 @@
   +    return \Drupal::entityQuery('simplenews_subscriber')
   +      ->condition('status', SubscriberInterface::ACTIVE)
   +      ->condition('subscriptions', $this->getNewsletterId())
   +      ->condition('subscriptions.status', SIMPLENEWS_SUBSCRIPTION_STATUS_SUBSCRIBED)
   

   There's an argument for this being on buildEntityQuery() in RecipientHandlerEntityBase as 99% of people extending RecipientHandlerEntityBase will want this code.

3. +++ b/modules/simplenews_demo/src/Plugin/simplenews/RecipientHandler/RecipientHandlerSubscribersByRole.php
   @@ -0,0 +1,47 @@
   +  static function settingsForm(array $element, $settings) {
   

   Great example to have

4. +++ b/simplenews.install
   @@ -70,17 +64,22 @@ function simplenews_schema() {
          'data' => array(
   +        'description' => 'A serialized array of name value pairs that define a temporary subscriber.',
   

   If we're doing a 2.x branch, maybe it's time to rename this 'subscriber' as that's what this really is. My drupal expectation was that 'data' was an arbitrary array where a value could store any extra values they liked.

   Alternatively, 'subscriber' could be a array key within 'data'.

5. +++ b/src/Plugin/simplenews/RecipientHandler/RecipientHandlerBase.php
   @@ -2,70 +2,70 @@
   +  protected function getNewsletterId() {
   

   I don't really see the point of this method, but it's certainly harmless.

6. +++ b/src/Plugin/simplenews/RecipientHandler/RecipientHandlerIdsBase.php
   @@ -0,0 +1,50 @@
   +abstract class RecipientHandlerIdsBase extends RecipientHandlerBase {
   

   Who would use RecipientHandlerIdsBase instead of RecipientHandlerEntityBase or RecipientHandlerSelectBase? I couldn't think of a use case.

7. +++ b/tests/src/Kernel/SimplenewsMonitoringTest.php
   @@ -42,12 +42,10 @@ class SimplenewsMonitoringTest extends KernelTestBase {
   -      'mail' => 'mail@example.com',
   

   Conceivably you could use the value of mail here to create a temporary subscriber object. But given that virtually nobody will use this method, that's probably only worth doing if you use it in tests.

8. +++ b/tests/src/Kernel/SimplenewsMonitoringTest.php
   @@ -42,12 +42,10 @@ class SimplenewsMonitoringTest extends KernelTestBase {
   -      'data' => array('data' => 'data'),
   

   Not sure why we're removing this

Thank you for checking. I believe I have everything covered however please follow up if you have any doubts.

1. Interesting questions.

why not leave the mail column undeprecated, and automatically create a subscriber downstream if we have a mail but no data or snid?

'data' allows passing not only a mail but also a langcode and any additional custom fields such as perhaps a name.

problems of mail being a somewhat unreliable way of looking up a subscriber.
No 'data' does not lookup a subscriber in the database - it initialises a temporary subscriber. That's another way that it differs from the previous 'mail' parameter.

2.

99% of people extending RecipientHandlerEntityBase will want this code

I don't agree: in all my use cases the recipient handlers are based on user data and there are no actual subscribers. But even if a high percentage want the code, unless it's 100%, then force adding them is bad because it's messy for code to try and take them off again.

4. It's not a subscriber though - it is data properties to set on a temporary subscriber. The data is unserialised then passed to EntityInterface::create for parameter "array $values" which has comment "array of values to set, keyed by property name".

5. This is for future compatibility. In general, RecipientHandlerBase has a $newsletter_ids array field because one day there might be multiple or none. The wrapper getNewsletterId() is a shortcut to check there is precisely one and return it.

6. I covered the reason briefly in #31:

Created new class RecipientHandlerIdsBase because sometimes EntityQuery is not flexible enough - it can only examine data referenced directly from the subscriber entities

I have a recipient handler that starts with an EntityQuery on 'commerce_order_item', loads the user from the uid field then looks up the corresponding snid. This needs RecipientHandlerIdsBase because RecipientHandlerEntityBase requires EntityQuery on 'simplenews_subscriber'. I could remove RecipientHandlerEntityBase and insist that everyone use RecipientHandlerIdsBase , but I thought the extra help in RecipientHandlerEntityBase was generally useful.

7. This seems similar to 1. If they want a temporary subscriber, they should pass the 'data' parameter which accepts all the necessary parameters.

8. Because that's a misuse of the 'data' parameter, so a potentially confusing example. The only reason it didn't break the tests before was that the spool was never sent.

I tried thinking about ways to refactor the RecipientHandlerBase classes to help use cases such as wanting to pass an array of temporary subscribers etc. But I can't see a generic way of doing it. The base classes in the patch and demos do a good job of showing some of the range of things that is possible.

The real step forward is moving addtoSpool() into the RecipientHandler, that unlocks things very nicely.

Feels like this is ready.

OK I have figured out how to eliminate RecipientHandlerIdsBase - thanks for asking good questions to prompt me to think further. This feels much better - notice how RecipientHandlerSiteMail has become simpler.

If you are happy, please set RTBC.

I crossposted with you Adam! I don't have time to review you last right now, here's what I was going to say ...

Some thoughts:

1. REQUEST_TIME is deprecated, per https://www.drupal.org/node/2785211
2. We've got know performance concerns with these handlers, so pagination is an interesting possibility.
3. As I said in my last comment , I'm curious about whether we can provide more flexible base methods.

With regard to 2 & 3, I dreamed up the approach below overnight. Feel free to ignore, it's only convenience stuff.

public function addToSpool() {
   $recipients = $this->getRecipients();
   if (empty($recipients)) {
     return;
   }

  foreach (array_chunk($recipients, 1000) as $recipientsChunk) {
   $insert = $this->connection->insert('simplenews_mail_spool')
     ->fields(['entity_id', 'entity_type', 'newsletter_id', 'snid', 'status', 'timestamp']));

   foreach ($recipientsChunk as $recipient) {
     $defaults = [
       'data' => NULL,
       'entity_type' => $this->issue->getEntityTypeId(),
       'entity_id' => $this->issue->id(),
       'newsletter_id' => $this->getNewsletterId(),
       'snid' => NULL,
       'status' => SpoolStorageInterface::STATUS_PENDING,
       'timestamp' => \Drupal::time()->getRequestTime(),
     ];
     $preparedRecipient = $this->prepareRecipient($recipient) + $defaults;
     // Discard extraneous keys.
     $preparedRecipient = array_intersect_key($preparedRecipient, $defaults);
     ksort($preparedRecipient);
     $insert->values($preparedRecipient);
   }  

   $insert->execute();
  }

  return count($recipients)
 }

protected function prepareRecipient($recipient) {
  switch gettype($recipient) {
    case 'integer':
      # Assume a subscriber id
      return ['snid' => $recipient];
    case 'string':
      # Assume an email
      return ['data' => ['mail' => $recipient]];
    case 'array':
     return $this->adjustData($recipient);
}

protected function adjustData($recipient) {
      $spoolFields =  ['data', 'entity_type', 'entity_id', 'newsletter_id', 'snid', 'status', 'timestamp'];
      // Extract any array keys that are not spool fields, and add them to the data subarray.
      $extraData = array_diff_key($recipient, array_flip($spoolFields));
      if (isset($recipient['data'] && is_array($recipient['data']) {
        $recipient['data] = $recipient['data] + $extraData;
      else {
        $recipient['data] = $extraData;
      }
      return $recipient;
}
     

I don't have time to review you last right now

In that case, I would like to go ahead and commit later in the week. If you later spot a problem then we can tackle it whilst I'm writing the tests.

1. True, but there are 23 matches across the whole module so let's tackle that separately.

2. Actually we have speculative performance concerns. I am a firm believer that performance fixes should only be made in response to actual problems and with clear evidence of an improvement. Your code would introduce a new window condition whereby an issue was partly spooled then timed out, but there isn't any code to track progress and continue from the last success.

3. I think that we had roughly the same idea here.

Yes, we're thinking similarly.

+++ b/src/Plugin/simplenews/RecipientHandler/RecipientHandlerBase.php
@@ -2,70 +2,113 @@
+    return count($values);

+++ b/src/Plugin/simplenews/RecipientHandler/RecipientHandlerEntityBase.php
@@ -0,0 +1,33 @@
+    $ids = $this->buildEntityQuery()->execute();
+    return $this->addArrayToSpool('snid', $ids);
+  }

I'm not sure it makes sense for addArrayToSpool to return the count. Instead you could do:

public function addToSpool() {
   $ids = $this->buildEntityQuery()->execute();
   $this->addArrayToSpool('snid', $ids);
   return count($ids);
  }

To a degree I see your point - the caller knows the count right. But on the other hand why shouldn't it return the count, I will spend another half hour

Set to 2.x branch. Delete spool 'mail' column rather than deprecate. Fix minor coding standard error.

I will commit this one once the tests pass.

I think it is clearer if I mark this as fixed because it has been committed. I have created #3062352: Add tests for RecipientHandler for the tests.