Allow other modules to prevent scheduled nodes from being published / unpublished

Hi,
I think you need to be clear exactly what the intention of this patch is. Originally in #14 of #1966876: Allow other modules to alter the list of nodes to be published or unpublished you just wanted to prevent the message 'This post is unpublished and will be published on date' because on sites which implement hook_scheduler_nid_list_alter() the content may be prevented from being published via scheduler. That was a good idea. But the changes you made in the patch in #16 on that issue actually prevent a node from being scheduled in the first place.

However, in the patch in #3 above, in scheduler_node_presave() the new function scheduler_is_allowed() is used to suppress the message but it does not alter whether the node can be scheduled or not.

Can you tell what the intended functionality is, then we can discuss how to achieve it.

Jonathan

I'm a bit apprehensive about committing this patch. I feel like this use case would better be served by a combination of workbench moderation + revision scheduler (http://drupal.org/project/revision_scheduler). This way you can use the workbench logic to get the node to a stage where it's ready to publish, and THEN you can schedule that revision to be published.

This works perfectly for me, in combination with #1955938: Publish only "approved" nodes .
Now "role1" can create a node and set the publish date. Role2 can approve the node, so the publish date will be honored. Otherwise it won't.

Can this please be committed?

Hi Pieter,
I still do not fully understand the purpose of these changes - see my comment in #4. Specifically my concerns are:

1. The change in scheduler_node_presave() only prevents the message. The node is still scheduled for publishing. If the node is scheduled then we should display the message, otherwise there is a mismatch between what is done and what the user is led to believe has happened. If you want to add additional warning messages you can use a secondary validate function added to $form['#validate'][] in form_alter. For example to say 'This node is scheduled but will not get published unless it is passed by an authorised user' or whatever
2. Additionally your $form['#validate'] could be used to prevent the node from being published immediately (if that config option is set and the user enters a past date) by checking whatever criteria you have and issueing a form_set_error()
3. The changes in _scheduler_publish() have the effect of skipping the scheduled node if it fails hook_scheduler_allow(). Why is this different from implementing hook_scheduler_nid_list_alter() and having that function remove $nid from the list?

So overall, the functionality in this patch can all be achieved by implementing existing hooks and form #validate functions. Atleast, that is how it appears to me. I'm happy to have a discussion about it, but as it stands I cannot pass this patch for committing.

Jonathan

OK, I see your point that using an extra validate handler may not cover all cases, so a specific hook in scheduler_node_presave is required.

I've followed through your logic changes in presave, and if scheduler_is_allowed() returns false, that will prevent the immediate publishing if that option was on. But then the node gets scheduled anyway. It will then be down to the call to scheduler_is_allowed() in _scheduler_publish() to allow or still prevent publishing. This is ok, but I think you need to add a bit more of this explanation into the comment in presave, as it appeared odd when I first read what you were doing.

Some other points to consider:

1. I'm not sure that hook_scheduler_allow() is the best name for other modules to implement. It's not clear whether we are allowing scheduling or allowing publishing/unpublishing. Something like hook_scheduler_activity_is_allowed() then the action 'publish' or 'unpublish' is the activity.
2. Once we fix on the the external hook name, we can look again the internal named function. It might be _scheduler_activity_is_allowed()
3. We already have hook_scheduler_nid_list() and hook_scheduler_nid_list_alter(). We will need to explain the difference and uses of the now three hook functions, so that module developers understand. Does this kind of documentation go in module onscreen help, or in a readme file?
4. In _scheduler_unpublish() on line 1029 you can use $action not the string 'publish'
5. Likewise in _scheduler_unpublish()
6. Coder review flags one point - would be nice to keep this output clean:
   Line 648: The correct use of the string is 'e.g.,' (with a comma after it). However, for clarity, consider changing 'e.g.' to 'for example,'
// converting it into the node property (e.g. 'publish_on').

Good work on the new test folder structure, thanks for starting that.

Jonathan

Now we need to do a second operation to change the status.

For the function name, 'is_allowed' is the wrong part of speech. It is passive, I think it should be a verb, ie. hook_scheduler_allow_...()

Also we should avoid the word 'action', as this will cause confusion with actions that Scheduler (will soon) provide as part of the Rules integration.

So some suggestions are, in php to give a flavour of how they will look:

hook_scheduler_allow_activity($node, $action);
hook_scheduler_allow_operation($node, $action);
hook_scheduler_allow_process($node, $action);

Alternatively, we could code it such that it does not use a second parameter, and the function name includes the action. Then modules would implement:

hook_scheduler_allow_publish($node);
hook_scheduler_allow_unpublish($node);
// or even
hook_scheduler_allow_publishing($node);
hook_scheduler_allow_unpublishing($node);  

We need to make it easy for other modules to implement the hooks, so using the two functions makes the names simple to remember and to understand. If we did this, then the internal function could be called _scheduler_allow() and would look like:

function _scheduler_allow($node, $action) {
  // Check if the requested action has been scheduled for the given node.
  $key = $action . '_on';
  $result = !empty($node->$key);

  // Allow other modules to prevent the action on the given node.
  $hook_name = 'scheduler_allow_' . $action . 'ing';
  foreach (module_implements($hook_name) as $module) {
    $function = $module . '_' . $hook_name;
    $result = $result && $function($node);
  }

  return $result;
}

Regarding the return value, I would say keep it simple as a boolean. In Scheduler we will not know whether the calling module has displayed those returned message(s) for the violations. If we just get back a boolean then it is clear to those who are writing their hook implementations that they need to display the messages.

Yes I think turning it round is even better, as that matches why the developer is implementing the hook - specifically to stop publishing. The default behaviour with no hook is to allow publishing. Good idea!

I have a slight preference for hook_scheduler_prevent_publishing()

Now it is maybe a bit weird to return FALSE if the (un)publication should be prevented

After the previous discussion, I thought again, and came to exactly the same conclusion. Having a negative purpose (prevent/deny) in the function name is problematic when returning a TRUE/FALSE. I believe it will be clearer if we reverted to hook_scheduler_allow_publishing() and hook_scheduler_allow_unpublishing() returning TRUE if allowed and FALSE if denied.

The internal function could be called _scheduler_allow($node, $action) and the code is more or less the same as you currently have. I just changed the comments slightly:

function _scheduler_allow($node, $action) {
  // Check if the requested action has been scheduled for the given node.
  $key = $action . '_on';
  $result = !empty($node->$key);

  // Check that other modules also allow the action.
  $hook = 'scheduler_allow_' . $action . 'ing';
  foreach (module_implements($hook) as $module) {
    $function = $module . '_' . $hook;
    $result &= $function($node);
  }

  return $result;
}

In the calls to this function, in _scheduler_publish() and _scheduler_unpublish(), it could be

    // Check that other modules allow the action on this node.
    if (!_scheduler_allow($n, $action)) {
      continue;
    }

Everything else is great. If you agree with the names as above, then we are RTBC. If you want to get the majority of this committed now then that's ok with me, and you could finish off the tests later in the issue. It's up to you.

Jonathan

I agree about the tests.
Good work. Excellent. RTBC :-)

It will be interesting, also, when you make this commit, to see if the -dev release gets updated. There are six commits which have been done since 18th Nov, and they are not showing yet on https://drupal.org/node/3292/commits

It will be interesting, also, when you make this commit, to see if the -dev release gets updated. There are six commits which have been done since 18th Nov, and they are not showing yet on

I deleted the 7.x-1.x branch test yesterday and today there's a new dev release! (Must be related to the d.o. upgrade.) Chances are that a next commit will fix the commit views display as well.

This issue is an example of #2146913: Issue auto-closing logic does not consider comment activity. Pieter commented only 6 days ago but the last node change was 14 days ago, hence the auto-closure now.

I have written a patch to revert back to the required D6 behavior, but it's not been reviewed yet.

Jonathan