Tidy up and fix anomolies in _scheduler_publish() and _scheduler_unpublish()

  1. Comments on both API calls should say 'react to the node ...' not 'alter the node ...'
  2. The two 'pre' API calls should be further up the function, before any data has been changed (to be done in 8.x)
  3. Remove the if-test for $status=1 in _scheduler_unpublish(). If the node is scheduled for unpublishing then it should be processed as such, regardless of the actual current status. If the node has been manually unpublished this does not remove the unpublish-on date in the scheduler table. Without this test the table will get tidied up in due course. Moved to #2355129: Remove test for $status=1 before unpublishing
  4. The selection of nodes should be <= not < to catch the cases when only using granularity of minutes and a node is scheduled for the same time as a cron run.
  5. Add new comments:

    // Allow other modules to add to the list of nodes to be published.

    // Allow other modules to add to the list of nodes to be unpublished.
  6. Move $action = 'unpublish' further up the function, and use it instead of hardcoding _scheduler_scheduler_nid_list('unpublish')
  7. Remove the setting and returning of $result.
CommentFileSizeAuthor
#14 2311273_14.tidy_up_8x_publish_unpublish_functions.patch2.13 KBjonathan1055
#10 2311273_10.tidy_up_publish_unpublish_functions.patch4.47 KBpfrenssen
#6 2311273_6.tidy_up_publish_unpublish_functions.patch4.39 KBjonathan1055
#2 2311273_2.tidy_up_publish_unpublish_functions.patch6.51 KBjonathan1055

Comments

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes
jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 2311273_2.tidy_up_publish_unpublish_functions.patch6.51 KB

Here's the patch which addresses points 1-6 above. Point 7 is not done - I thought that even though we are not using the $result anymore, it may be useful to leave in, as we don't know how the module may evolve in D8.

pfrenssen’s picture

pfrenssen
Location Sofia
commented 
  1. +++ b/scheduler.module
@@ -1329,17 +1329,19 @@ function _scheduler_publish() {
-  $query->condition('s.publish_on', REQUEST_TIME, '<');
+  $query->condition('s.publish_on', REQUEST_TIME, '<=');
   $query_result = $query->execute();

    Excellent catch, especially considering that this code usually runs on cron. It will solve some mysterious cases where nodes seem to be (un)published too late. 

  2. +++ b/scheduler.module
@@ -1350,6 +1352,9 @@ function _scheduler_publish() {
+    // Invoke Scheduler API for modules to react before the node is published.
+    _scheduler_scheduler_api($n, 'pre_' . $action);

    This makes sense but I'm afraid we can't change this any more. It might break sites that rely on the current behaviour. For example, some developers might rely on the changed date or created date being set to the publish_on date.

    I would certainly add a comment here to fix this in D8. 

  3. +++ b/scheduler.module
@@ -1434,42 +1437,39 @@ function _scheduler_unpublish() {
+    // Invoke scheduler API for modules to react before the node is unpublished.
+    _scheduler_scheduler_api($n, 'pre_' . $action);
+

    Same here, we cannot change this behaviour because of the risk it might break existing implementations. Let's add a comment to fix this in D8. 

  4. +++ b/scheduler.module
@@ -1434,42 +1437,39 @@ function _scheduler_unpublish() {
-    if ($n->status == 1) {
-      $create_unpublishing_revision = variable_get('scheduler_unpublish_revision_' . $n->type, 0) == 1;

    This check to see if the node is actually published before attempting to unpublish it has been removed. Is this intentional?

    It might be better to discuss the potential implications of this in a separate issue.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs review » Needs work
jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes
Related issues: +#2355129: Remove test for $status=1 before unpublishing

Thanks for the review.
Yes, I take your point about not moving the scheduler API 'pre' calls within D7. I will remove them but add a TODO comment for D8.

Regarding the removal of status=1 check, yes that was intentional - see point 3 in the summary. However, I agree that we can discuss it in a separate issue #2355129: Remove test for $status=1 before unpublishing.

I will re-roll the patch without these.

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 2311273_6.tidy_up_publish_unpublish_functions.patch4.39 KB

Here's a new patch, against 7.x-1.2+11, covering the changes discussed.
Using the numbering in the summary, it only includes items 1, 4, 5 and 6.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Assigned: jonathan1055 » pfrenssen

Going to work on this.

jonathan1055’s picture

jonathan1055 commented

ha ha! OK, lets avoid cross-posting patches this time.

pfrenssen’s picture

pfrenssen
Location Sofia
commented

You catch my drift :D

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Assigned: pfrenssen » Unassigned
StatusFileSize
new 2311273_10.tidy_up_publish_unpublish_functions.patch4.47 KB

Looks good, patch needed a reroll. If this is green this is RTBC.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs review » Fixed

Great! Committed to 7.x-1.x, thanks!!

  • pfrenssen committed 435bf63 on 7.x-1.x authored by jonathan1055 
    Issue #2311273 by jonathan1055, pfrenssen: Tidy up _scheduler_publish()...

Status: Fixed » Closed (fixed)

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

jonathan1055’s picture

jonathan1055 commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Issue summary: View changes
Status: Closed (fixed) » Needs review
StatusFileSize
new 2311273_14.tidy_up_8x_publish_unpublish_functions.patch2.13 KB

There were some @todo tasks regarding moving the 'pre' actions which we had to leave until D8 (see #3 point 2 and 3). It would be good to get these done now, before I start on expanding the API tests #2655666: API Testing module - conversion to 8.x.

Also as we work on #2651338: Create a service for the Scheduler API and #2669164: Introduce event subscriber for the Scheduler hooks the API test module will change, but the tests might/should stay the same, and the calls should be moved to their correct places before any of this work is started.

Patch attached, but currently these api functions are not covered by testing.

jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Fixed

Here's the commit http://cgit.drupalcode.org/scheduler/commit/?id=4ecd062
Auto commit comments still not working

  • jonathan1055 committed 4ecd062 on 8.x-1.x 
    Issue #2311273 by jonathan1055: Move pre api calls in _scheduler_publish...

Status: Fixed » Closed (fixed)

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