Port to Drupal 7

+1

subscribing

subscribing

subscribing

Once the #1017318: Support for Simplenews 6.x-2.x and #970942: Saving or updating schedule settings for has been unsuccessful. have been resolved we will start on a D7 port, at the moment i see the simplenews 7.x branch is still dev-only release.

Both issues have been fixed. Do you think that simplenews 7.x is stable enough to start work on converting scheduler to 7.x branch? There are 2000 of reported installs of simplenews 7.x already.

I will be glad to help converting if you haven't started yet.

Subscribe

Curious about this development as well. There are almost 5000 installs of Simplenews 7.x now.

(And by the way, gagoo, you don't need subscribe anymore, there's a "Follow" button for that.)

+1

Any status on this?

We will initiate a port of this project as we need it for a customer project.

More information coming soon.

Joachim has done a really awesome job of finding some bugs left over in D6 version, would be great to start porting once these bugs are solved

The module is already ported (was working last week) and we're about to publish the 7.x version...

s_leu was working on this starting last wednesday. So all 6.x changes after around 2011-12-12 will need to be ported.
We just didn't want to publish the new 7.x release as it's currently part of internal quality checks.

> So all 6.x changes after around 2011-12-12 will need to be ported.

Uhoh... I've filed quite a few 6.x patches I plan to commit next week...

Just make sure to set them all to patch (to be ported) and we'll check them.

Patch coming up soon..

Here's an initial patch for the D7 Port.

Thanks!

Looks like this is still a work in progress.

A lot of the changes here are whitespace and formatting, so to simplify the patch noise here I'm going to do a whitespace and code style fix on the D6 branch now.

Also, I'd like these to be forward-ported:

- #1352672: allow token replacement of new edition node titles
- #1352616: _simplenews_scheduler_new_edition() should take care of database records
- #1352740: original node body appears twice in newsletter edition body
- #1352688: Views admin links appear in edition node body - render edition nodes as the anon user

Some of those could probably be achieved by rerolling this patch against the 6.x-2.x head.

A few review comments below:

+++ b/simplenews_scheduler.install
@@ -61,103 +67,27 @@ function simplenews_scheduler_schema() {
- * Implementation of hook_uninstall().
+/*
+ * Implements hook_update_last_removed().
  */
-function simplenews_scheduler_uninstall() {
-  // Remove tables.
-  drupal_uninstall_schema('simplenews_scheduler');
+function simplenews_scheduler_update_last_removed() {
+  return 6005;

See http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...

> A good rule of thumb is to remove updates older than two major releases of Drupal. See hook_update_last_removed() to notify Drupal about the removals.

Hence we should keep them in.

+++ b/simplenews_scheduler.module
@@ -47,124 +59,129 @@ function simplenews_scheduler_menu() {
+ * @todo implement the #states properties correctly since this will
+ * make the simplenews_scheduler.js needless.

Is this still relevant?

+++ b/simplenews_scheduler.module
@@ -172,350 +189,390 @@ function simplenews_scheduler_form_alter(&$form, &$form_state, $form_id) {
+function simplenews_scheduler_node_load($nodes, $types) {

Missing docblock.

+++ b/simplenews_scheduler.module
@@ -172,350 +189,390 @@ function simplenews_scheduler_form_alter(&$form, &$form_state, $form_id) {
-      $pid = $row["nid"];
+
+      $pid = $row->nid;
+
       // If returns with null don't do anything.
-      $first_run = intval($row['start_date']);
+      $first_run = intval($row->start_date);

To simplify this patch, we could keep these DB query results as arrays -- there's a constant you can pass to Database API for that.

+++ b/simplenews_scheduler.views.inc
@@ -0,0 +1,324 @@
+/**
+ * Implements hook_views_data().
+ ¶
+function simplenews__scheduler_views_data() {

Changes to this file should really be a patch on D6, as nothing changes from Views 6 to 7 and the current code in D6 actually dates back from D5. We also seem to be missing a hook_views_api() here.

Yep, we already have a bunch of more fixes in our internal git branch, we'll provide an updated patch soon.

- Will look into the commited issues. Especially the token replace stuff is something we need anyway and are currently doing in a custom module (to avoid making this patch even bigger). That also justifies to won't fix a feature request in simplenews.module and only do a token replace on the final newsletter title :)

- Disagree on the last_removed stuff. I know that sentence, but it's pointless. Core is doing the same, you can't directly upgrade from a 5.x database to a 7.x one. If we'd keep the update functions, we'd need to port them to 7.x syntax and test it and so on.

- Agree on the views part, the initial patch might also not (yet) contain the default view stuff for similar reasons.

Thanks for the review!

Here's the reloaded version of the patch that includes all of the patches joachim listed above. I also ported the editions overview page code. This code will be replaced by a view though, but I will open a separate issue for this and post a corresponding patch there.

Also I did change most of what you proposed joachim, except for the DB results as arrays cause it's a bit faster to code like that. And thanks for the hint about the missing hook_views_api().

Please review.

Going through the patch, posting some thoughts I had...

+++ b/simplenews_scheduler.install
@@ -94,106 +92,27 @@ function simplenews_scheduler_schema() {
+function simplenews_scheduler_update_7000() {
+  ¶
+  if (!db_field_exists('simplenews_scheduler', 'title')) {
+    $field = array(
+      'description' => 'The title of new edition nodes.',

Some trailing spaces here.

+++ b/simplenews_scheduler.module
@@ -8,147 +9,168 @@
   $items = array();
+  ¶
   $items["node/%node/editions"] = array(
-    'title' => 'Newsletter Editions',
-    'type' => MENU_LOCAL_TASK,
-    'weight' => 2,
-    'page callback' => 'simplenews_scheduler_node_page',
-    'page arguments' => array(1),
-    'access callback' => '_simplenews_scheduler_tab_permission',
-    'access arguments' => array(1),
+  'title' => 'Newsletter Editions',
+  'type' => MENU_LOCAL_TASK,
+  'weight' => 2,
+  'page callback' => 'simplenews_scheduler_node_page',
+  'page arguments' => array(1),
+  'access callback' => '_simplenews_scheduler_tab_permission',
+  'access arguments' => array(1),
   );
+    ¶
   return $items;

Indentation looks wrong, also some more trailing spaces.

+++ b/simplenews_scheduler.module
@@ -178,38 +215,49 @@ function simplenews_scheduler_form_alter(&$form, &$form_state, $form_id) {
+      /* @todo this is still D6 code, not sure wether it's still of any use
+        // If the node has attachments
+        if (count($form['attachments']['wrapper']['files']) && user_access('upload files')) {
         // Disable all attachment form elements and the delete button to avoid the deletion of the parent's attachments.
         $form['attachments']['#description'] = t('Attachments cannot be changed, this is a newsletter edition created by Simplenews Scheduler.');
         $form['attachments']['wrapper']['new']['upload']['#disabled'] = TRUE;
         $form['attachments']['wrapper']['new']['attach']['#disabled'] = TRUE;

I don't think attachments need any kind of special handling anymore. They should get a new file_usage entry for the new node if they're copied and shouldn't be deleted when you delete them on the edition. So, can probably be removed.

+++ b/simplenews_scheduler.module
@@ -217,218 +265,327 @@ function simplenews_scheduler_submit($form, &$form_state) {
       // this run's timestamp ($now_time) to the right time by adding a correct interval.
-      $this_run = $first_run + floor(($now_time - $first_run) / $seconds) * $seconds;
+      //$this_run = $first_run + floor(($now_time - $first_run) / $seconds) * $seconds;

Looks like the commented out line here could be removed.

+++ b/simplenews_scheduler.module
@@ -217,218 +265,327 @@ function simplenews_scheduler_submit($form, &$form_state) {
+        $tid = db_query("SELECT tid from {simplenews_newsletter} WHERE nid = :nid", array(':nid' => $node->nid))->fetchField();
+        $accounts = simplenews_get_subscriptions_by_list($tid);
+
+        module_load_include('inc', 'simplenews', 'includes/simplenews.mail');
+        simplenews_add_node_to_spool($node, $accounts);

The $accounts argument is actually dead code now and isn't doing anything, as I've justed noticed.

Instead, we need to make sure that the new node gets the same newsletter category assignment as the old one. Which he probably does, because otherwise it wouldn't work :).

Please confirm that the new nodes have the correct tid assignement in the simplenews_newsletter table and then you can remove everything with $tid and $accounts here.

+++ b/simplenews_scheduler.module
@@ -217,218 +265,327 @@ function simplenews_scheduler_submit($form, &$form_state) {
+ *
+ * @todo check for support of file fields of the node as attachments

Shouldn't this actually just work as we're now doing a clone of the node?

+++ b/simplenews_scheduler.module
@@ -217,218 +265,327 @@ function simplenews_scheduler_submit($form, &$form_state) {
+    // Switch to the anonymous user to render node content.
+    // This prevents things like Views admin links from showing in edition node body.
+    simplenews_impersonate_user(drupal_anonymous_user());

FYI: That function is a backport of the not yet commited user_impersonate_user() core function. Note that this is only a partial solution as anon users might see links like add comment as well.

Also, now that I think about it, as we're actually doing a clone then this is probably not necessary anymore as well. Because we're not actually rendering something.

+++ b/simplenews_scheduler.module
@@ -217,218 +265,327 @@ function simplenews_scheduler_submit($form, &$form_state) {
+    // Let other modules change the cloned node too
+    module_invoke_all('simplenews_scheduler_edition_clone', $edition_node);

Documentation for the new hook would be nice. Create a simplenews_scheduler.api.php file, add a simple function hook_simplenews_scheduler_edition_clone($edition_node) and add a quick docblock.

+++ b/simplenews_scheduler.module
@@ -217,218 +265,327 @@ function simplenews_scheduler_submit($form, &$form_state) {
+  if (!empty($rows)) {
+    $output .= theme('table', array('header' => array(t('Edition Node'),  t('Date sent')), 'rows' => $rows, 'attributes' => array('class' => array('schedule-history'))));
+    $output .= theme('pager', array('tags' => 20));
   }
   else {
-    $output .= '<p>' . t('No scheduled newsletters have been sent.') . '</p>';
+    $output .= '<p>' . t('No scheduled newsletter editions have been sent.') . '</p>';
   }

theme_table() supports an empty parameter, that is automatically displayed if there are no rows.

+++ b/simplenews_scheduler.module
@@ -217,218 +265,327 @@ function simplenews_scheduler_submit($form, &$form_state) {
 /**
+ * Theme the title of the newsletter edition.
+ */
+function theme_simplenews_scheduler_title($variables) {
+  $title = $variables['title'];
+  $serial = $variables['serial'];
+  return $title . " " . t('(edition !edition)', array(
+      '!edition' => $serial,
+    ));
+}

This looks like a left-over.

> + module_invoke_all('simplenews_scheduler_edition_clone', $edition_node);

Could this be done with drupal_alter()?

> function _simplenews_scheduler_new_edition

This seems to get removed and added in the patch. Is that just bad diffing, or has it been moved around? I'm all for function order being made more sensible, but best done in a follow-on issue rather than a big monster patch.

Oops, status clash.

Next patch taking into account proposed changes.

Tested with latest 6.x-2.x-dev, getting the following errors

/patches/simplenews-port_to_drupal_7_v1.2-1056388-22.patch:49: trailing whitespace.
 * The node is passed as node object and therefore passed by reference. This hook
/patches/simplenews-port_to_drupal_7_v1.2-1056388-22.patch:50: trailing whitespace.
 * is for example usefull if you have fields in the template node that contain
/patches/simplenews-port_to_drupal_7_v1.2-1056388-22.patch:51: trailing whitespace.
 * information about data that should get rendered dynamically into the edition
/patches/simplenews-port_to_drupal_7_v1.2-1056388-22.patch:406: trailing whitespace.
    // check prevents php notice if newsletter was sent and only a checkbox appears in the form
/patches/simplenews-port_to_drupal_7_v1.2-1056388-22.patch:414: trailing whitespace.

error: patch failed: simplenews_scheduler.info:1
error: simplenews_scheduler.info: patch does not apply

You need to apply the patch against a git checkout, not against a dev snapshot. Snaphots contain additional information in the .info file which leads to conflicts when trying to apply the patch.

I'm applying against a git checkout, not snapshot

I have applied the last patch successfully to a git checkout, but am receiving the following error:

Fatal error: Call to undefined method DateObject::getTimestamp() in path/sites/all/modules/simplenews_scheduler/simplenews_scheduler.module on line 256

reinstalled the date module with the latest version, but didn't seem to help.

http://ch.php.net/manual/en/datetime.gettimestamp.php. The function is only available in PHP 5.3.

Sounds like we need to switch to format('U') then, as php.net suggests. Feel free to update the patch.

Another patch that enables minute accurate start and stop date selection. Since used the date_select widget provided by the date_api module there are now no more ->getTimestamp method calls anymore. Please review and test.

@Berdir should this switch to format('U') be rolled into a different patch?

@dgtlmoon There are no more ->getTimestamp() method calls that could be replaced by format('U') in the latest patch. You shouldn't use the previous patch anymore.

Is there someone beside Berdir and s_leu that can do a qualified review of the latest patch #31?

Does not apply cleanly to --branch 6.x-2.x:

Checking patch simplenews_scheduler.install...
error: while searching for:
        'type' => 'varchar',
        'length' => 10,
      ),
      'start_date' => array(
        'type' => 'int',
        'not null' => TRUE,

error: patch failed: simplenews_scheduler.install:38

Rerolled, but now its my patch so I can't mark it RTBC.

JStanton, you still can state what you reviewed and what conclusions you have...
Just switching to RTBC wouldn't be a well enough qualified review anyway.

Cross-referencing #742652: Use Batch API to send mails, this might introduce an API that change needs to be accounted for here (the simplenews_add_node_to_spool() call).

Note that the call is already wrong and passes $accounts to it, which isn't created anymore and not used in the API anymore.

Subscribing

> - Disagree on the last_removed stuff. I know that sentence, but it's pointless. Core is doing the same, you can't directly upgrade from a 5.x database to a 7.x one. If we'd keep the update functions, we'd need to port them to 7.x syntax and test it and so on.

I'm still iffy about this.

Suppose a user only installed the first 6.x version of the module and hasn't updated to newer 6.x versions, and so has a system table at 6000. What happens when they upgrade to 7?

Is it standard policy that you have to take a module to its final 6.x version before upgrading to 7? Is that true of core?

Pretty much yes. See http://drupal.org/node/550152, the "Update Contributed Modules as Required" chapter.

I just think it makes it way easier to support the upgrade path, because otherwise, you will end up with a large number of possible combinations which you need to support.

The Batch API change was committed.

Note this is wrong:

-        $accounts = simplenews_scheduler_get_newsletter_accounts($tid);
-        simplenews_send_node($node, $accounts);
+        module_load_include('inc', 'simplenews', 'includes/simplenews.mail');
+        simplenews_add_node_to_spool($node, $accounts);

Remove the argument $accounts here.

> Pretty much yes. See http://drupal.org/node/550152, the "Update Contributed Modules as Required" chapter.

I notice flag module has updates from 6000 up still in its 7 branch.

> I just think it makes it way easier to support the upgrade path, because otherwise, you will end up with a large number of possible combinations which you need to support.

Actually not quite the case, I think.

Suppose we are currently at update 6002. Tomorrow we release the D7 version which begins with 7000.

Then next week we find a bug or add a feature and for that we need a 6003.

Now what?

There is no difference :)

If the bug is also present in 7.x-1.x, we need to add an upgrade function to 7.x-1.x that checks if the changes have already been made (D7 has much better API's for this, e.g. for indexes) and if not, apply them.

But, we do not also need to support all existing versions in the upgrade path including maybe even D5 versions.

In addition we'll always try to avoid changes on D6 branch that changes things like schema. Generally we're not interested in maintaining too many feature branches. Maintenance complexity is also why previous major versions are bugfixing only.

Corrected the function call mentioned in #42 in JStanton's patch:

Please review

Committed and pushed this.
Publishing -dev release asap.

>> Is it standard policy that you have to take a module to its final 6.x version before upgrading to 7? Is that true of core?

> Pretty much yes. See http://drupal.org/node/550152, the "Update Contributed Modules as Required" chapter.

We need docs for this then. At least in the release notes for the dev and for the eventual stable release; ideally on the project page too.

Though really I think we should have taken the time to run the 6xxx upgrade functions through coder upgrade and given our users an easier ride.