Scheduler history: retaining un/publish_on data

These fields won't end up in Core (that's just the way things work around here). If you want to supply aletrations please supply a patch.

This issue is over two years old, so I'm highjacking it for the D6 version. This is a problem if you have any other module that might key off of this data. Here's the use case:

I have a node with scheduling enabled and auto_nodetitle. Auto_nodetitle is using the publish_on and unpublish_on dates to give the node a title. When this data is removed and the node is saved the title becomes corrupt/inaccurate.

I understand the reason that this data is wiped out, but I think that removing this data is more of a workaround than a 'fix' for the 'publish must be in the future' issue. Trying to find the issue where this was the decided way to go...

Looks like there is some work on this over at #782710: Views Calendar integration

This patch is being moved over here, split out from #782710: Views Calendar integration.

Pertinent comments from #782710: Views Calendar integration from Eric:

Your other patches contain some changes that are not really related to the history problem (e.g. drupal_write_record), but are useful refactorings. Maybe you could find and extract those changes and post them as separate issues to improve the code quality of scheduler. I still want scheduler to become more generic (e.g. schedule all kinds of actions), but that will be a gradual process. In this process scheduler will get a (optional) history. But this history will have a use case I do understand and use. Thats why I do not reject this issue but postpone it.

+++ scheduler.module Locally Modified (Based On LOCAL)
@@ -386,68 +392,122 @@
+          elseif (!variable_get('scheduler_history_' . $node->type, 0)
+                  && $publishtime < $now) {

maybe this is just a preference, but I've never seen this split in core before so I'm not sure it's completely kosher here.

+++ scheduler.module Locally Modified (Based On LOCAL)
@@ -386,68 +392,122 @@
+          drupal_write_record('scheduler', $node);

Eric said that these should be part of a separate patch. We might even want to remove the comment clarifications/capitalizations as part of a general cleanup patch.

+++ scheduler.module Locally Modified (Based On LOCAL)
@@ -386,68 +392,122 @@
+              drupal_write_record('scheduler', $node, 'nid');
...
+              drupal_write_record('scheduler', $node, 'nid');

Personally I don't think it hurts to use drupal_write_record here as this is new code, not refactoring, but Eric might have a different opinion.

+++ scheduler.module Locally Modified (Based On LOCAL)
@@ -386,68 +392,122 @@
+            drupal_write_record('scheduler', $node);

This one is def a re-write so should be reverted per Eric's comments.

+++ scheduler.module Locally Modified (Based On LOCAL)
@@ -466,54 +526,94 @@
+  $scheduled = db_query('
+    SELECT *
+    FROM {scheduler} s
+    LEFT JOIN {node} n ON s.nid = n.nid
+    WHERE s.unpublish_on > 0
+      AND s.unpublish_on < %d
+      AND s.perform_unpublish = 1

Personally I think this format for a query is MUCH easier to read, but we might need approval to keep it this way since all of the other queries are structured as one-liners.

+++ scheduler.module Locally Modified (Based On LOCAL)
@@ -466,54 +526,94 @@
-  while ($node = db_fetch_object($nodes)) {
-    $n = node_load($node->nid);
-    $n->changed = $node->publish_on;
-    if (variable_get('scheduler_touch_'. $n->type, 0) == 1) {
-      $n->created = $node->publish_on;
+  while ($scheduler = db_fetch_object($scheduled)) {
+    // if this node is to be unpublished, we can update the node and remove the record since it can't be republished
+    $node = node_load($scheduler->nid);

Using $scheduler instead of $node and $node instead of $n is a great improvement and i thin make sense to do in this patch since the original query is changing.

+++ scheduler.module Locally Modified (Based On LOCAL)
@@ -466,54 +526,94 @@
+  $scheduled = db_query('
+    SELECT *
+    FROM {scheduler} s
+    WHERE s.publish_on > 0
+      AND s.publish_on < %d
+      AND s.perform_publish = 1

Again, may need approval on formatting.

+++ scheduler.module Locally Modified (Based On LOCAL)
@@ -466,54 +526,94 @@
-    _scheduler_scheduler_api($n, 'unpublish');
+    _scheduler_scheduler_api($node, 'publish');
 
     $clear_cache = TRUE;
   }

As mentioned before, operating on $node instead of $n is highly recommended.

Powered by Dreditor.

The removal of the publish_on field on publishing is not a workaround of forbidden past dates. The fields already got removed long before past dates were forbidden. If a history should be kept the whole data design must be changed or the old values must be pushed into separate fields. I am not really satisfied with this design, but I also do not have the time to change it right away. It will be a gradual process. Maybe I will even move such a change to a 2.x branch, because scheduler is widely used today and I do not want to put all the users at risk. There are actually many things I want to change as time permits. Please don't hold your breath...

Thanks a lot for commenting on codys patch. I will have a look at it at the weekend.

Closing this, because it is actually an really old issue which is not really related to the problem at hand. I will create a new issue for changing the whole data storage. For a lot of stuff I want to do I need a different db layout (normalized). I don't know yet how it will look like because the most obvious normalized schema will not be easy to use for views integration. Watch out for the new issue...

For reference, here is the db change issue that Eric has created #1006766: Normalize and extend schema