Spool after saving instead of in pre-save

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.

I picked up the direction agreed above (move the spooling from presave to insert/update) and got it working: new MR against 4.x.

The reason !53 stalled is that it removed the "save except if already saving" protection from addIssue() and made the save unconditional, which caused recursive saves and broke tests all over. This MR keeps addIssue() untouched, exactly as requested in #6. Instead, the insert/update hook persists the status change itself with one guarded save, so custom code calling addIssue() keeps working as before.

Coverage:

• A kernel test using a small helper module that flips the published state in hook_entity_presave(), the same hook ordering as Scheduler but without depending on it. It checks both directions: a node that ends up unpublished is not sent, and a node that ends up published is sent, including when it only becomes published after simplenews's own presave used to run.
• A functional test with the real Scheduler module (added to test_dependencies) covering the exact scenario from the issue summary, cron publishing when the date arrives, and the publish-past-date-immediately setting on a brand new node.

Moving to insert/update also fixes #2625412: Cannot send on publish if node is new: a new node has an ID by the time it is spooled, so creating an issue that is published on its first save no longer fatals. That test is included here too, so this supersedes the MR over there.

Verified on Drupal 10.6 and 11.4, and the whole existing functional suite still passes with the change. Note the functional tests will show red on CI until #3542503: Fix tests lands (the body-field breakage affects every test that creates an issue node); the kernel tests are the ones to look at.

Tests are now fixed needs reroll

Tests are green.

Thanks.

1) The MR here makes an extra call to save() and then introduces a flag to prevent recursion. This is done to ensure the changes from addIssue() are actually saved.

The patch on #2625412: Cannot send on publish if node is new is the same as already approved by Berdir, and it now has a test. It is careful to avoid a double-save, following various comments exchanged on that topic further up the issue. This seems good, but does it actually work given the above?? Does it fail to save the subscriber count and status?? Does it also need a call to save()? Or is the code in this MR unnecessary?

2) I feel that the code comment in addIssue() where it says "Save except if already saving" is intended for the case of a presave. We are already saving, and the changes we make will be saved as part of that save hence we don't need to call save again. In the case of insert/update this code is wrong - it means data isn't saved. You have compensated by making an extra call to save() after calling addIssue(). It took be quite a while to fully understand this, so I would like to make it clearer, keeping the ownership of saving inside the addIssue() function.

Let's add a $postSave flag that defaults to FALSE, and should be set to TRUE by any caller in update/insert hooks (in this way the API change is BC). We can change to comment to explain that the save is only skipped during presave when the entity will already be saved. Then we can also move the simplenews_spooling guard code inside the addIssue() function, similar to fillFromAccount().

This also gives the maximum help towards anyone who might be using this module with another entity type.

3) This module puts the guard on the node rather than static which makes some sense - can you confirm it isn't actually saved?

4) There seems to be a lot of duplication in the 2 tests lets combine them.

Thanks for the review.

The double save is only needed in one direction. $entity->original is set on update but never on insert, and it isn't cleared until after the insert/update hooks run. So on insert, addIssue() was already saving internally, the wrapper's extra save() was a second, redundant write. On update it stays set, so that explicit save was needed, without it the subscriber count and status never reach the database.

Moved the save and the guard into addIssue() itself behind a $postSave flag, default FALSE so every other caller ("Send now" form, SendIssue action, the direct calls in the functional tests) behaves exactly as before. hook_ENTITY_TYPE_insert()/_update() pass postSave: TRUE, and addIssue() handles both the save and the recursion guard, one save on insert instead of two, and any other entity type calling this from its own hooks gets the same behavior for free.

simplenews_spooling is never saved. An undefined property on a content entity lands in the same in-memory bucket core uses for $entity->original, it never touches field storage.

Merged the two kernel tests into one class with shared setup, still covers #2625412: Cannot send on publish if node is new and #3438042: Spool after saving instead of in pre-save, and fixed the comment that claimed ->original is set on both insert and update.

Great thanks this seems like a really high quality fix now