Hi,

I've found a problem with sending out edited content. When I edit a newsletter which hasn't been sent, and then send it, the original text is sent out instead of the edited text.

The problem is that _sn_send() is called directly from simplenews_update(). At the point in node_save() when the hook_update() functions are called, the new node body has been saved to the database, but it isn't until after the hook_update() and hook_nodeapi('update') functions have all completed that the cache is cleared, so simplenews is sending out the old cached content.

I can see that the module needs to save the tid due to a similar ordering problem in node_save() where simplenews_insert() and _sn_send() are called before the nodes terms have been committed to the database.

A fix for both of these issues would be to move the _sn_send() call to hook_exit(), by which stage all the nodes data will be committed. It also means that the node_save() operation won't be caused to fail by a long mail-out exceeding the allowed script execution time.

I know you're currently rewriting a bunch of things in the module, so I thought I'd give you this to keep in mind.

Cheers,
Eric

Comments

DriesK’s picture

DriesK commented

Hi Eric,

Thanks for this one. The order in which functions are called in Drupal has given me headaches before...

Using hook_exit() however... it doesn't seem the right place to me (conceptually). Hook_exit() is called on every page, not only when newsletters are created/updated. It would also require carefully chosen conditional statements, to prevent other form submissions (also from unknown modules) from firing _sn_send().

I found a much simpler solution: when calling node_load() in _sn_send(), just reset the internal node_load cache:

$node = module_invoke('node', 'load', array('nid' => $nid->nid), NULL, TRUE);

This will ensure that the latest version is retrieved from the db. I tested it, and it does the job. Fix will be included in the upcoming simplenews version.

About the long mail-out exceeding the allowed script execution time: the maximum value of the "max send time" setting in simplenews is based on max_execution_time in php.ini, and is always smaller than this value (e.g. if max_execution_time is 30, "max send time" is maximum 25). In _sn_send, another (arbitrary) safety margin is built in.

DriesK’s picture

DriesK commented
Assigned: Unassigned » DriesK
Status: Active » Fixed

This bug was indeed rather critical, so I committed the fix to 4-6 and HEAD already.

Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
DriesK’s picture

DriesK commented
Version: » 7.x-1.x-dev
Status: Fixed » Closed (fixed)

Closed because the automatic close feature in project module is broken, and the issue list is becoming cluttered.