Allow publish_on dates in the past for advanced use cases.

For example:

  • When creating nodes automatically (via an API e.t.c) from an external source, where time offsets and time zone differences may cause the publish_on field of a node not to validate.
  • When creating nodes automatically (via an API e.t.c) from an external source, where the publish_on field is manually set may cause the publish_on field of a node not to validate for past entries.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dolphinonmobile’s picture

Status: Active » Needs review
FileSize
2.4 KB

Here is a patch that adds the following changes:

  • A scheduler_publish_ignore variable that allows publish_on dates in the past to be ignored if set.
  • A scheduler_publish_ignore check box option for each node type: Ignore publish time that is not in the future (Advanced).
jonathan1055’s picture

pfrenssen’s picture

pfrenssen’s picture

Status: Needs review » Needs work

Reading through this issue and the duplicates there seem to be two different approaches for this:

  1. Nodes saved with a publication date in the past should be published immediately. This is the approach taken by raymondwanyoike, and is perfect for nodes that are saved through the user interface, as it gives immediate confirmation to the site editor that the node is in fact published.
  2. Nodes saved with a publication date in the past should be published on the next cron run. This is how bartoll does it, and is interesting when mass importing a large number of nodes, as the publishing can be done at off-peak hours to reduce server load.

I agree with Raymond that these are both advanced cases, so I would suggest to keep the current behaviour as the default, and allow the administrator to choose between either of the two approaches to fit their particular use case.

I do think this should be a site-wide setting rather than something that can be configured separately for each content type. Having different content types react differently just makes it more complicated I think. Also, since this probably does not affect most users, shall we put this under a separate "Advanced options" section?

pfrenssen’s picture

Hmm maybe this should be configurable per content type, seeing that both raymondwanyoike and bartoll do it this way I am probably mistaken in my opinion.

jonathan1055’s picture

Thanks for summarising this problem so accurately. Yes I knew it was not an obvious solution. Maybe a nice idea for config.

pfrenssen’s picture

Assigned: dolphinonmobile » pfrenssen

Going to work on this.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
8 KB

Here is a patch that offers both options. They have been put in an "Advanced options" fieldset on the node type edit forms. I've also included a test.

This patch has been written from scratch, but I have taken inspiration from the approaches of raymondwanyoike and bartoll, so they should be credited if this patch would get accepted.

jonathan1055’s picture

Hi Pieter,
This is excellent work - thanks very much. I like the way you have reviewed the other issues and decided on a solution to help everyone. I have fully tested the changes and everything works as expected. However, I have made a few minor alterations so have re-rolled the patch. The main processing is unchanged, that all works fine.

  1. Changes to the menu text. I expanded the label, removed the description, adjusted some of the text and made the 'advanced' fieldset only show up if the 'enable' checkbox is ticked.

    Original
    Only local images are allowed.
    New
    Only local images are allowed.

  2. In scheduler_node_presave() I moved the new code outside the foreach loop allowing the testing to be simplified. The function now looks like:
    function scheduler_node_presave($node) {
      foreach (array('publish_on', 'unpublish_on') as $key) {
        if (empty($node->$key)) {
          // Make sure publish_on and unpublish_on are not empty strings.
          $node->$key = 0;
        }
        elseif (!is_numeric($node->$key)) {
          $node->$key = _scheduler_strtotime($node->$key);
        }
      }
      // Publish the node immediately if the publication date is in the past.
      if (variable_get('scheduler_publish_past_date_' . $node->type, 'error') == 'publish' && is_numeric($node->publish_on) && $node->publish_on < REQUEST_TIME) {
        $node->publish_on = 0;
        $node->status = 1;
      }
      // Otherwise, if a pubishing date has been set, make sure the node is
      // unpublished since it will be published by cron later.
      elseif (is_numeric($node->publish_on) && $node->publish_on > 0) {
        $node->status = 0;
        $date_format = variable_get('scheduler_date_format', SCHEDULER_DATE_FORMAT);
        drupal_set_message(t('This post is unpublished and will be published @publish_time.', array('@publish_time' => format_date($node->publish_on, 'custom', $date_format))), 'status', FALSE);
      }
    }
    
  3. I have added a few more assertions into the tests. However, in #1069668: Default time with user override I have redesigned the testing structure, so if that patch gets committed first, we will need to adjust the tests here.

Great work! I think this will be a useful addition to make scheduler more flexible.

Jonathan

pfrenssen’s picture

Issue tags: +needs backport to 6.x

#1125772: If publish_on date is today publish now has been marked as a duplicate of this issue, but for the 6.x branch. Tagging this for backporting to 6.x.

pfrenssen’s picture

FileSize
8.23 KB

This is the interdiff between patches #8 and #9.

Status: Needs review » Needs work

The last submitted patch, 1819074-9-scheduler-publish_past_dates.patch, failed testing.

pfrenssen’s picture

Reviewed the changes. Overall the changes are looking really good, love the improvements to the text strings :)

+++ b/scheduler.moduleundefined
@@ -688,17 +692,16 @@ function scheduler_node_presave($node) {
     elseif (!is_numeric($node->$key)) {
       $node->$key = _scheduler_strtotime($node->$key);
-
-      // Publish the node immediately if the publication date is in the past.
-      if ($key == 'publish_on' && variable_get('scheduler_publish_past_date_' . $node->type, 'error') == 'publish' && !empty($node->$key) && $node->$key < REQUEST_TIME) {
-        $node->$key = 0;
-        $node->status = 1;
-      }
     }
   }
-  // Right before we save the node, we need to check if a "publish on" value has been set.
-  // If it has been set, we want to make sure the node is unpublished since it will be published at a later date
-  if (!empty($node->publish_on) && is_numeric($node->publish_on) && ($node->publish_on > REQUEST_TIME || variable_get('scheduler_publish_past_date_' . $node->type, 'error' == 'schedule'))) {
+  // Publish the node immediately if the publication date is in the past.
+  if (variable_get('scheduler_publish_past_date_' . $node->type, 'error') == 'publish' && is_numeric($node->publish_on) && $node->publish_on < REQUEST_TIME) {
+    $node->publish_on = 0;
+    $node->status = 1;
+  }
  • If I read this correctly, then this will cause a new unpublished node that is saved with no publication date to be published immediately. You should add a check to see if $node->publish_on is not empty before deciding to publish. If this is the case then this indicates that we are lacking test coverage for this use case.
  • The is_numeric() check is not necessary. In the foreach loop just before this code this value is set to a numeric value.
  • Moving this out of the loop is a good idea. It is much more readable this way.
+++ b/scheduler.moduleundefined
@@ -688,17 +692,16 @@ function scheduler_node_presave($node) {
+  // Otherwise, if a pubishing date has been set, make sure the node is

'pubishing' misses a letter :)

+++ b/scheduler.moduleundefined
@@ -688,17 +692,16 @@ function scheduler_node_presave($node) {
+  elseif (is_numeric($node->publish_on) && $node->publish_on > 0) {

The is_numeric() check is not needed, the other check is sufficient.

+++ b/scheduler.testundefined
@@ -71,42 +71,44 @@ class SchedulerTestCase extends DrupalWebTestCase {
-    $this->assertRaw(t("The 'publish on' date must be in the future"), 'An error message is shown when the publication date lies in the past and the "error" behavior is chosen.');
+    $this->assertRaw(t("The 'publish on' date must be in the future"), 'An error message is shown when the publication date is in the past and the "error" behavior is chosen.');

The way the assert messages are rendered has changed a while ago, we should not be using t() any more, but format_string().

jonathan1055’s picture

Hi Pieter,
Thanks for the review. I've been away for a few days, hence only just seen this.
Glad you like the text changes. Yes, I will look at the is_numeric() test. We also had some feedback on this issue from someone else but they put their comments in the wrong issue, over on #1069668: Default time with user override in #42, so I will incorporate their point that we forgot to change the content creation time to match the scheduled publish time if that option is set.

By the way, did you enable the automated testing of patches? That's very useful.

Jonathan

jonathan1055’s picture

Status: Needs work » Needs review
FileSize
8.78 KB
if (variable_get('scheduler_publish_past_date_' . $node->type, 'error') == 'publish' && is_numeric($node->publish_on) && $node->publish_on < REQUEST_TIME)

If I read this correctly, then this will cause a new unpublished node that is saved with no publication date to be published immediately. You should add a check to see if $node->publish_on is not empty before deciding to publish.

You are right, that test should use !empty() instead of is_numeric(). Also I have removed the other is_numeric() test because it is redundant as you pointed out. But for safety I have made sure that we get $node->key set to zero if _scheduler_strtotime returns false (which it should not do here, because validation has already passed).

I have fixed the typos, added the 'change creation date to match publish date' lines, and also changed t() to format_string() in the new tests in .test file (not changed the existing ones here). I have removed the t() around 'Basic page' when it is a parameter inside format_string because I think you are saying that the test text does not get translated.

Here is a patch against the new dev 1.1+19. It also does not conflict with the code review standards changes.

Jonathan

pfrenssen’s picture

FileSize
3.76 KB

This is the interdiff between patches #9 and #15.

jonathan1055’s picture

Seems like I left a few of the calls to t() in the .test file. I only changed the first one to format_string(). Re-rolled for web_user to admin_user recent commit.

pfrenssen’s picture

@jonathan1055: Yes, I enabled the automated testing, I find that incredibly useful :) Also, it is not really needed to check compatibility inbetween patches ahead of them being committed, these conflicts are usually very easy to solve and if not we can always tag them with "Needs reroll".

On to the review!

  1. +++ b/scheduler.module
    @@ -703,17 +703,22 @@ function scheduler_node_presave($node) {
    -      $node->$key = _scheduler_strtotime($node->$key);
    +      // Convert to unix timestamp, but ensure any failure is converted to zero.
    +      $node->$key = _scheduler_strtotime($node->$key) + 0;
    

    This is some clever trickery! I would use the boring (int) type casting, but as this works just as well and has a nice comment explaining it :)

  2. +++ b/scheduler.test
    @@ -40,6 +40,7 @@ class SchedulerTestCase extends DrupalWebTestCase {
         // Add scheduler functionality to the page node type.
         variable_set('scheduler_publish_enable_page', 1);
         variable_set('scheduler_unpublish_enable_page', 1);
    +    variable_set('scheduler_publish_touch_page', 1);
       }
    

    This does not really belong in the setUp() of the test, since it will then apply to all tests in the class. There might be some tests that require this setting to be off.

  3. +++ b/scheduler.test
    @@ -113,12 +114,13 @@ class SchedulerTestCase extends DrupalWebTestCase {
    +    $this->assertEqual(format_date($node->created, 'custom', 'Y-m-d H:i:s'), $edit['publish_on'], 'The node "created date" has been changed to match the "publish on" date');
     
    

    If you want to test it here you would need to test both the touch option enabled or disabled. This does highlight that this behaviour is currently not tested. I think we better delegate this to a specific unit test that saves nodes.

  4. +++ b/scheduler.test
    @@ -113,12 +114,13 @@ class SchedulerTestCase extends DrupalWebTestCase {
    -    $this->assertNoRaw(t("The 'publish on' date must be in the future"), 'No error message is shown when the publication date is in the past and the "schedule" behavior is chosen.');
    +    $this->assertNoRaw(format_string("The 'publish on' date must be in the future"), 'No error message is shown when the publication date is in the past and the "schedule" behavior is chosen.');
         $this->assertRaw(t('@type %title has been updated.', array('@type' => t('Basic page'), '%title' => check_plain($edit['title']))), 'The node is saved successfully when the publication date is in the past and the "schedule" behavior is chosen.');
    

    Oh no, I suggested this right? I was completely wrong, we do need to use t() here. It was actually fine before. I must have misread it as the assert message being wrapped by t(). My excuses!

pfrenssen’s picture

@jonathan1055, I still had my review open from yesterday so I didn't see your last post. I'm sorry for setting you on the wrong foot with the format_string() issue. The assert messages should not be put through t() since then they become available for translation on localize.drupal.org, which is useless since these assert messages are only ever read by developers and do not need to be translated. When we are testing translatable strings in the interface, we should run it through t() though. format_string() is helpful since it allows to put placeholders in the string without translation.

So a typical example would be:

  $this->assertRaw(t('A translatable string to test for.'), format_string('This is the assert message for @test.', array('@test' => $test)));

I'm very sorry for letting you waste your time on this :(

pfrenssen’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)

Created a followup issue for testing the 'touch' functionality as well as all the other logic in scheduler_node_presave(): #2112867: Provide a unit test for saving scheduled nodes.

I've incorporated the update of the $web_user in #17 and committed it to 7.x-1.x: 8d40d8a. Thanks everybody!

Marking as needs backport to 6.x.

jonathan1055’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Fixed
Issue tags: -needs backport to 6.x

Should we really be back-porting this to Drupal6? I know in #10 above you linked #1125772: If publish_on date is today publish now as a duplicate and tagged this issue for port to D6. But this is more than just a bug-fix, it has entirely new functionality and features. I suggest that the D6 version should remain supported but only for fixing bugs or serious usability problems. We could be making a whole lot of work for ourselves entirely unnecessarily by back-porting, particularly as the original creators of these issues no longer seem to be active. We have enough open issues on D7 to work on.

I propose to leave this issue as fixed. If anyone wants to re-open it and make a good case for back-porting then we can discuss it.

Jonathan

pfrenssen’s picture

Make sense! If someone needs this for D6 then patches are always welcome.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

temaruk’s picture

I need a patch for this issue that applies against 7.x-1.1 and I need it to be referenceable from D.org, so forgive me commenting on a closed issue, but here it is.

jonathan1055’s picture

OK, no problem. I presume you need to share it with others. Looks like you based it on the patch in #15, is that right?

I'm not sure why you patched the .test file though.

Jonathan

temaruk’s picture

I based it on the actual commit that was made in 7.x-1.x. I created a branch off the commit that was tagged with 7.x-1.1 and cherry picked the commit (and fixed the resulting conflict) that is mentioned in #20 (8d40d8a).