Disallow publish_on and unpublish_on dates in the past

Go to the content type configuration (e.g. /admin/content/types/story for stories) and activate "Alter published on time".

If the option is activated for a certain node type scheduler will set the created_on date to the publish_on value in the same operation that does the publishing.

Could you post your workflow for publishing via scheduler (what do you do, step by step)?

Eric

Now this is weird.
You should not be able to enter a date without a time. You should get "The entered publication date is invalid." on submit. Can your try it again with a time (like 00:00:00 or any other)?

Also you do not need to set "publishing options > published" since scheduler deactivates this. All scheduler does for scheduling is setting this checkbox value after the scheduled time passed.

From what I can make out the "created" date/timestamp only gets modified when scheduler flips the node's "published" flag during a cron run. Maybe this has something to do with this question.

So, for example, if I set a node to be published in the past, then the created date will never be modified.

If I set a node to be published in the future, then the "created" date will remain the date the node was originally created until such time as scheduler publishes the node.

Since I will be using Views to sort nodes on "created" date this is going to make listings (for admins) change order...

Aha, I'm getting the hang of it more ... it's not quite so straightforward.

1. If I set a node to be published in the past, and the node's current status is "published", then the created date will never be modified and the "publish on" date will remain set in perpetuity.

2. If I set a node to be published in the past, and the node's current status is "unpublished", then everything will happen normally on the next cron run: the node gets published, the created date is updated (with the publish on date, not necessarily the same as the time "now"), and the "publish on" date is cleared.

3. If I set a node to be published in the future, then the current status of the node is forced to be "unpublished" and behaviour is then as in case 2., except of course that it only gets published on the first cron run after the "publish on" date.

Case 1. is a bit awkward, there's probably no ideal way of handling it.

I am not sure if we have deviated from the original reported problem, but there does seem to be an issue with entering a publish_on date that is in the past. It would be useful to hear from the original poster cheriejd to find out if the problem was due to entering dates in the past. I don't know the D5 version, but in D6 you can set the input format to have no time, so effectively if you enter todays date it will be in the past because 00:00 is taken as the time (although in #6 Eric said that you can't have a date without a time, so maybe the validation of the custom input format is different in D5)

Anyway, I agree with gpk in #8 that there is a slight problem here. I tested on D6 and it exhibits the same behaviour, namely if you enter a publish_on date in the past then the node appears as published immediately, but a row is still inserted into the scheduler table. Scheduler_cron never selects this node because the status is already 1, ie published, so the created date remains unchanged, and the row remains in the scheduler table forever.

(Yes I know you may ask why would you enter a publish_on date in the past, but some users could use this to back-date their posts if they do not have admin nodes permission, so we need to cater properly for it. And we don't want rows in the scheduler table that are never deleted, because they show up in lists of 'to be published' content, which is confusing).

The problem is in scheduler_nodeapi(). For op=presave, the status of the node is only forced to 0 (unpublished) if the publish_on time is in the future. But during op=insert, a row is added to the scheduler table if publish_on has a value, any value, even in the past.

There are at least two ways to change the behaviour so that it becomes consistent. Either (a) in op=presave, we force the node status to 0 for all cases when a publish_on time is entered, even if it is in the past. Then the node will not be immediately published, but will be correctly published on the next cron run, and the created_on date altered if applicable. Or (b) in op=presave, if the publish_on time is in the past then we alter the created time immediately (if applicable) and then clear the publish_on time, so that a row will not be erroneously inserted in the scheduler table during op=insert.

Jonathan

@jonathan1055: I've been thinking along similar lines and was wondering whether to open a separate issue but this one seems to be well and truly hijacked now, and as you imply we may actually be addressing the original problem after all.

As you say, if publish_on is in the future then the status of the Published checkbox is ignored (the node is immediately set to unpublished upon submission of the node form).

For consistency, I would vote for the same sort of thing if publish_on is <= now. i.e. the node is immediately set to Published, and the created time is touched per the time specified (if that option is selected on the node type config page). It doesn't make sense to me to have to wait to the next cron for same to happen. I don't think that "forc[ing] the node status to 0 for all cases when a publish_on time is entered, even if it is in the past" is the best option since that will cause an already published node to be unpublished until the next cron. So I think I am voting for your option b).

[This also simplifies things slightly when setting up Views of nodes since you know that if there is a publish_on time then the node is not published.]

What then needs to be clear to the user is that the publish_on time overrides whatever is selected for the Published checkbox. The only time to use the latter is (for a user who has administer nodes permission) when you want to change the published status but without scheduling that change - which is still a very reasonable thing to want to do.

Similar considerations probably apply for the unpublish_on date/time; though that's not something I've used yet. I don't know whether scheduler requires the unpublish time to be after the publish time, but maybe that's something that would need to be checked. Possibly there would be no harm though for a node to be first unpublished and subsequently published again, though I can't see any use case for this! Also the best behavior to go for in the case where the interval between publish and unpublish is less than the interval between cron runs is unclear to me.

HTH,

I also have a preference for which option to do, but I purposely have not said it, so as to get an unbiased response.
[update: Actually I've coded a fix for this now, in anticipation that my preference matches Eric's.]

Yes, there is already validation to ensure that unpublish_on is not earlier than publish_on.

(btw, you mentioned twice about using views to list scheduled nodes. I have already coded what I think you want, see comment #16 in #344138: Displaying list of scheduled nodes (contrib) which provides default views which the admin can modify. I'll create a new thread here when the final tweaks are done and it is ready for review)

Jonathan

I don't think a publish_on time in the past is the right way to change the created time of a node. If we allow times in the past 'created' should be set to $now, because the time is bogus anyways.
Why don't we disallow the use of past dates and times? Is there a real use case for them?

If times in the past are allowed then the created time should be set to $now, publish_on should be blank and published should be 1.

BTW: A time value is only required if the format says so.

Well, yes I said there were at least two ways to solve this, and I think Eric has suggested a third option which is better than both of mine. If we dis-allow publlsh_on and unpublish_dates in the past then the problem goes away. There is not really a good case to use past dates, as it allows users to fake old posts, which is not a good idea.

I may have some time to look at coding this today.

Jonathan

Well, I coded the validation changes, that was quite easy, but in testing I discovered a whole new edit/validate/preview problem that I think was already in there. The new problem I have found comes when you enter a publish_on date and save the node. Then edit it again and attempt to clear the publish_on date. When you preview the date is cleared, but it gets loaded again from the db in hook_form_alter() so the cleared value re-appears in the form.

This will take a bit more thinking through. We need to fully work out the interaction between hook_nodeapi op=validate and then hook_form_alter, so that the correct data flows through.

Jonathan

Here is the patch file of my changes. It is a bit rough and ready, and I have left the debug in because you may want to use this as a starting point, and continue the work, as we fix the form_alter() problem.

I have improved the format of the error messages, ie it used to say the date was invalid when actually what it should say is that the date does not match the expected format. An example of the expected format is now shown in the message.

I also added the new value into the array returned by hook_perm that Eric forgot on the last change ;-)

Jonathan

Here is an updated patch, created against the dev version of 4th May. It includes changes for:

In scheduler_perm()
1. Add the missing 'administer scheduler' value to the returned array

In scheduler_form_alter()
2. When doing a preview get the default values from the form not the db
3. Testing is now done on the $defaults object not $node
4. Consolidate the logic tests for whether to call scheduler_strToTime
5. Replace tabs with spaces as per drupal coding standards so that indentation is consistent

In scheduler_nodeapi()
6. Set error if either the publish_on or unpublish_on values are in the past
7. Change the terminology from 'expiration' to 'unpublished' to match everywhere else.
8. Testing for isset and not numeric is not strong enough, also need to make sure that there is a value entered
9. Improve the error message wording when the entered format does not match the expected scheduler date format, ie say '... does not match the expected format' instead of '... is invalid'
10. Give example of the expected format in the error message
11. When setting $node->status to 0 remove the test for the publish_on date to be in the future as this is now already done before this stage.

I have numbered them just for ease of reference if we need to discuss any of them. I have tested it and am happy with the changes, so would like someone else to test it too. I changed the title so that it more accurately reflects what this thread is now fixing. Hope that's OK.

Jonathan

I realised that doing one patch to change three functions which are not directly related is not a good idea. I have separated out the hook_perm
change and added a patch file to #442554: "access admin pages" is too permissive for allowing user to change scheduler settings and created a new issue #457630: Get values from form not db when doing preview

So here is the patch that just relates to the issue being fixed here, ie not to allow dates in the past. The changes, all in scheduler_nodeapi() as follows:

1. Set error if either the publish_on or unpublish_on values are in the past
2. Change the terminology from 'expiration' to 'unpublished' to match everywhere else.
3. Testing for isset and not numeric is not strong enough, also need to make sure that there is a value entered
4. Improve the error message wording when the entered format does not match the expected scheduler date format, ie say '... does not match the expected format' instead of '... is invalid'
5. Give example of the expected format in the error message
6. When setting $node->status to 0 remove the test for the publish_on date to be in the future as this is now already done before this stage.
7. Changed a couple of 'else if' to 'elseif' as this is the recommended syntax

Jonathan

[edit: before applying the patch you may need to correct the filename on line 1. I forgot to change it from my temporary name 'scheduler.module.b.before' I had to use when creating the diff]

Very nice. Works like a charm. Committed.

Thanks all, this (i.e. #12, #17) looks to be a more sensible approach than what I was suggesting :-)

Note that from what I can see the patch at #17 has only been committed to the Drupal 6 branch, and will be included in the next release (6.x-1.4).

@cheriejd: apologies, I think I hijacked your support issue but glad that everything is working for you now.

Reopen because of #516946: Nodes are not being published, only the publishing date gets updated.
I tested every revision of scheduler.module between 6.x-1.3 and 6.x-1.4 and could pinpoint the revision that received the patch in #17.
Stand by...

OK, I nailed it: The condition for setting the status of a scheduled node to unpublished if there is a valid publishing time also included a check if the time is in the future. Since now past times are not allowed anyways the future-check was removed. On publishing the scheduled node scheduler_nodeapi is called again (via node_publish_action) and in this case the publishing time is not removed yet, but is also not in the future anymore. In this case the node status must not be set to unpublished (0). That's it.

Please review and test the patch asap if you can (it is very tiny). Jonathan, is my explanation correct?

Committed: http://drupal.org/cvs?commit=236372

I want to roll a new release as soon as possible, because the bug breaks scheduler.
Please review and test soon and report. Thanks alot.

Yes Eric, I just checked this and your reasoning is correct.
When a node is published by Scheduler hook_nodeapi(op=presave) is called. The check you have added for $publish_on > time() does the trick and the status is not set to zero. So I think you can create a new version with this fix.

The strange thing is that this bug got past Eric, Cheriejd, gpk and me. We all said OK!

I have just checked my live site and scheduler has been working fine for weeks with the version as supplied in #17. Has core changed such that nodeapi op=presave is now called when it was not before?

Jonathan

Here comes 6.x-1.5...

I have just checked my live site and scheduler has been working fine for weeks with the version as supplied in #17. Has core changed such that nodeapi op=presave is now called when it was not before?

That would be an explanation. I will check the last core changes.

I have finally discovered how this got through my testing on my local site, and therefore how my live site has been working correctly ever since I put the patch in #17 live. It took a while to track down, but when I found it, I was so simple! It is all to do with the permissions assigned to the cron job. Scheduler_nodeapi() is called with op=presave during the cron publishing process, but the section of code that erroneously resets the status to zero does not get executed if the user does not have 'schedule (un)publishing of nodes' authority. I have not given this to anonymous users, so my cron job does not have this authority, hence the scheduler_nodeapi did not do anything for op=presave and my newly-published nodes remained published and did not get their status reset to zero. I suspect that this is how the problem was not spotted by any of the folk above who checked the patch.

I have attached two screen grabs with lots of debug written to the log, for anyone who is interested in the full process that happens when scheduler publishes a node. [well, there is a lot more, but I limited debug to just the scheduler-related hooks and in nodeapi only wrote out debug for op=presave]. The first is where the cron run is initiated by Admin, who does have the scheduler perm, and the second is by a time-triggered crontab run, which does not have the authority.

I would be surprised if any site admin actually gave scheduling authority to anonymous users, so I think (I hope) that this "bug" did not actually impact real crontab-initiated runs, and the incorrect reset was only executed when an site admin manually ran a cron via the admin menu.

Well, I feel a bit better now. It was not good when I thought I had introduced a major bug which had made it into a production release! Given that the original issue #516946: Nodes are not being published, only the publishing date gets updated was raised by Eric, I guess from a manually-triggered cron run, I am hoping that not too many sites had actual live problems.

Jonathan

Yes, I did a manual cronrun (/cron.php) as admin. Thanks for debugging this.

Do you use poormanscron?

No I don't use poormanscron. My host allows me to configure proper crontab entries, so I have also done this on my local machine so that it replicates the live site. [I am using MAMP on a Mac]

I recently submitted this bug report: #431776: Cron should run as anonymous when invoked via the run-cron link on the status report page. poormanscron should probably also make sure that cron runs as anonymous (although I appreciate it is not implicated in the discussions at hand). AFAIK there is no issue to address this at the moment.

Originally I was thinking that cron should always be forced to run as anonymous, but on reflection I concluded that this would be too restrictive. At the moment, if problems arise from cron running as different users then they usually stem from the run-cron link or from poormanscron etc. Seems best to fix it in these places rather than go for a blanket solution that restricts what's possible in special situations.

Is there a possibility to add back the dates in the past checking to hook_cron and add the configuration option for turning the form fields validation on publish_on and unpublish_on fields off?

As if I'm trying to fill the proper dates (which are in the past however) for bulk unpublishing old (for which I know that these are old) nodes using the scheduler, validation doesn't lets me to do it, I need to unpublish these by hand.

What is the benefit of unpublishing a node with scheduler and a past date? If you are editing the node, you can just remove the "published" check mark? So what are you gaining?