Simplenews module doesn't work with scheduler

It's annoying for simplenews to have to do this, but I don't see an alternative solution and scheduler has 65k D8+ installs. Scheduling an email newsletter to go out at a future time would seem to be a common use case.

I understand your situation and I can see that this would solve it.

However I feel that simplenews is completely correct. The problem is that scheduler module is allowing the node to have the published flag set briefly when in fact it the node isn't published. So this patch is adding code in simplenews module (also quite popular with 20k D8 installs) to workaround an issue in scheduler. The code relies on a detailed understanding of the workings of scheduler. It would become particularly messy if the required workaround would vary depending on the major version of scheduler (which would be allowed by Drupal compatibility rules).

I have an idea. It seems like it could solve the bug if the presave() functions ran in the opposite order. The current order is presumably random chance based on the alphabetic order of module names.

So scheduler module could alter the implements order to ensure its own presave runs first, or even change its module weight - this would ensure that the temporary incorrect published status was not seen by other modules.

What do you think?

Thanks for the reply Adam. I was about to open an issue with scheduler, but then I realised what their maintainer would probably say:
why is simplenews taking a heavy action like spooling an email in a presave hook.

Surely presave hooks are for modifying the state of an entity before it is saved, for taking a downstream action consequent on an entity being saved in a particular state we should wait for the entity to actually be finished saving, and use an insert/update hook instead.

Potentially this could also improve the maintainability of SpoolStorage::addIssue() which is called from simplenews_node_presave(). It does things like:

// Save except if already saving.
    if (!isset($issue->original)) {
      $issue->save();
    }

which seems a bit twisty.

I'm open to the idea of a patch that changes from simplenews_node_presave to simplenews_node_update. It feel like a much cleaner approach than the original patch. Beware though, often things in the simplenews code were done for a reason (after hitting a problem) without leaving a comment to explain why, and it can be quite subtle.

I wouldn't like to delete the protection of // Save except if already saving. because any site could have custom code that calls addIssue() and relies on this.

I've made an attempt, let's see what the tests say.

It turns out that this bit of SpoolStorage::addIssue():

// Save except if already saving.
 if (!isset($issue->original)) {
    $issue->save();
 }

is a problem. It's actually already a problem. It's designed to prevent an unnecessary recursive save when addIssue() is called from simplenews_node_presave(). But ->original is still set during insert and update hook invocations, so it also currently stops the updated issue stats and status from being set properly when addIssue() is called from an update hook. Which doesn't matter for nodes currently, but messes with anyone using simplenews with another issue entity type and sensibly using insert/update hooks instead of presave.

Unfortunately, I cannot see a sensible way to detect whether addIssue() is called from a presave hook or an insert/update hook. We can in theory play around with setting a flag presave and removing it postsave, but it's more or less impossible to guarantee this works given the places stuff can be called from like hook_ENTITY_TYPE_presave, hook_entity_presave and just EntityBase::preSave(); even trying to get this to work imperfectly would involve fiddly stuff with hook_module_implements_alter().

I think the best solution is to accept that non-node entities are getting buggy results right now, that even if they are calling addIssue from a presave hook they will probably survive us triggering a recursive save OK, so we'd best just fix this right as there's nothing else we can do to help them. Unless we're willing to deprecate addIssue() altogether as beyond BC redemption.

And the tests fail all over the place and weirdly ... some debugging needed.