Cleanup and use simplenews api functions in _simplenews_scheduler_new_edition() [#1461944]

Follow-up of #1399626: further refactoring in and around _simplenews_scheduler_new_edition()

Copy of my review in that issue:

+    // Get the node id of the parent newsletter.
+    $pid = $row->nid;

$pid is never used, looks like those lines can be removed.

+  // Prepare the correct status for Simplenews to pickup.
+  db_update('simplenews_newsletter')
+    ->fields(array('status' => 1))
+    ->condition('nid', $edition_node->nid)
+    ->execute();

There is an API function for this: http://api.worldempire.ch/api/simplenews/simplenews.module/function/simp...

+  // Save taxonomy terms.
+  watchdog('simplenews_sched', 'Saved new node ready to be sent. Node ID: !nid', array(
+    '!nid' => $edition_node->nid,
+  ));

Weird comment :) Also, we might want to use the $link argument to watchdog instead of putting the node id in there and re-word the message (e.g. newsletter edition instead of node)

+  // Let other modules change the cloned node too
+  // module_invoke_all('simplenews_scheduler_edition_clone', $edition_node);

Commented out code that has been replaced by the hook below.

+ * @param $eid
+ *  The node id of the new edition to send. This should already have been
+ *  created by _simplenews_scheduler_new_edition().

Wouldn't it make sense to pass the node object around instead of $eid? Kinda pointless to first explicitly only return the node id and then load it again a few lines down. Sure, it's cached, but still not necessary. It makes perfectly sense to separate the logic into separate functions, but you're still not going to use them separately if and you really do, it's trivial to do a node_load() yourself if you just have the id.

Comment	File	Size	Author
#6	without_module_weight.patch	3.41 KB	Berdir
#6
#2	1461944-2.simplenews_scheduler.simplenews-api-cleanup.patch	3.21 KB	joachim
#2
#1	new_edition_cleanup.patch	4.33 KB	Berdir
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Berdir

German

Switzerland

CreditAttribution: Berdir commented 9 March 2012 at 19:14

Title:	Cleanup and update to latest API	» Cleanup and use simplenews api functions in _simplenews_scheduler_new_edition()
Status:	Active	» Needs review

File	Size
new_edition_cleanup.patch	4.33 KB

Here's a patch that improves the watchdog entry, uses the simplenews API function and removes to additional unnecessary argument to the alter hook.

I also updated the tests to actually verify that we are sending the newsletter and the new usage of the API works. I noticed, while doing that, that our cron by default runs after Simplenews, which means that we can't immediately start sending the newsletter. So I also added an install/update hook to set the module weight to -1 to fix this.

Will create a separate issue for improving the alter hook (passing the parent node to it) and the $edit stuff.

Comment #2

joachim CreditAttribution: joachim commented 12 March 2012 at 20:15

File	Size
1461944-2.simplenews_scheduler.simplenews-api-cleanup.patch	3.21 KB

I'd like to break out the cron fix to an issue of its own so we can backport it: #1479240: newsletters take 2 cron runs to send: run our cron before Simplenews?

Here's your patch with the cron bits taken out -- can you check it's ok? In particular I wasn't sure whether to leave this in:

     // Call our hook to generate it.
-    simplenews_scheduler_cron();
+    drupal_cron_run();

Comment #3

Berdir

German

Switzerland

CreditAttribution: Berdir commented 12 March 2012 at 20:17

Yes, that's necessary so that simplenews_cron() is executed as well.

The problem is that the current tests depend on the cron fix so we need to postpone this issue on the other one or call the two cron implementations directly/manually in the correct order.