Allow other modules to alter the list of nodes to be published or unpublished

Hi pfrenssen

This is a really nice idea, and so simple to implement! I did not know that implementations of hook_TYPE_alter() could be used to alter anything. The documentation talks about using it to alter content types and forms, but of course, if a module implements it corrctely then there is no reason that we cannot utilise the power of drupal_alter to do anything.

Your patch does not use the 4th parameter of drupal_alter. Can you think of anything that we could usefully pass for $context2?

Also, could you post your current implementation of hook_scheduler_node_list_alter, for interest, and to allow others to see why this is such a nice idea. I will also write my own hook_scheduler_node_list_alter to test your patch.

Jonathan

Hi,
Just trying this, and I get

Fatal error: Only variables can be passed by reference in /sites/all/modules/scheduler/scheduler.module on line 754

As you might have guessed, this is the call to drupal_alter, which I implemented as copied from your example above. $nids is an array but I presume you had this working, so is there something I need to do to enable it?

Just realised I am still using D7.17 so I will upgrade to 7.22 then test again.

Jonathan

Think I've fixed it. Nothing to do with old drupal version. The code in your patch needs to be:

  // Allow other modules to alter the node list.
  $context = 'publish';
  drupal_alter('scheduler_node_list', $nids, $context);

ie the context should be a variable not a fixed string.

Jonathan

Just discovered that the variable $context is already used in _scheduler_publish() and _scheduler_unpublish() - so it would need to be a different name, eg $context1, or $scheduler_context or something.

Forgot to change the status as the patch needs amending.
My tests are working now and this will be a very useful (and powerful) addition. Thanks for the idea pfrenssen.

Tested the patch in #9 and it works. Yes, $action is a good choice.

Just one minor question - we already implement the function hook_scheduler_nid_list() so is hook_scheduler_node_list_alter() ok? I guess the word 'alter' makes it distinct enough, just. I thought I would mention it now, as we will never be able to change it after other users/modules start implementing their hooks.

I am kind-of ok with the name as it stands, but would like you to think about it, and also to get the opinion of the co-maintainers and others.

Jonathan

Excellent. Reviewed and tested. Rick has already just made a 7.x-1.1 but hopefully release 1.2 will not be too far away.

To add to your example in #3 here is what I used to test the hook:

function d7testing_scheduler_nid_list_alter(&$nids, $action) {
  // Only allow to publish nodes authored by uid 1 or who has administer role.
  foreach ($nids as $key => $nid) {
    $node = node_load($nid);    $user = user_load($node->uid);
    dd('node ' . $node->nid . ' author is ' . $node->name . ' (' . $node->uid . ')');
    if ($node->uid != 1 && !isset($user->roles['3'])) {
      watchdog('d7testing', 'Blocked @action of @author @type %title.',
               array('@author' => $node->name . ' (' . $node->uid . ')', '@action' => $action, '%title' => $node->title, '@type' => $node->type), WATCHDOG_NOTICE, l(t('view'), 'node/'. $node->nid));

      // Move the time on by one hour, because this node is not going to be actioned.
      $arr = array($action . '_on' => time() + 3600);
      db_merge('scheduler')
      ->key(array('nid' => $node->nid))
      ->fields($arr)
      ->execute();
      dd('removed node ' . $node->nid . ' and set new ' . $action . ' time of ' . format_date(time() + 3600));

      // Remove this node from the list.
      unset($nids[$key]);
    }
  }
}

Thought you might be interested in the idea of pushing back the publish time. I have a calendar view of upcoming nodes to be published and the ones which were blocked were showing as upcoming in the past, which looked all wrong. This way, if the node is rejected it is continually pushed forward until the author is accepted.

Jonathan

Yes there might be better ways to put out that message, and also allow its content to be modified by modules which implement hook_scheduler_nid_list_alter()
Maybe that same hook function could be called directly with a specific parameter, instead of having a different hook to implement? Plenty of options to play with.

Maybe, as this issue is RTBC I would suggest opening a new one, not to delay this patch?

Hi,
Had a look at your patch, and I've got a few things to discuss

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 (isset($node->publish_on) && $node->publish_on != '' && is_numeric($node->publish_on) && $node->publish_on > REQUEST_TIME) {
   +  if (scheduler_is_scheduled($node, 'publish') && $node->publish_on > REQUEST_TIME) {
        $node->status = 0; 
   

   The line $node->status = 0 which ensures a scheduled node does not get immediately published is included in that conditional block. You are affecting too much here, it is only the message that you want to suppress.

2. You have implemented hook_scheduler_is_scheduled() in addition to hook_scheduler_nid_list_alter(). It is my guess that the code in these two functions would be very similar or at least big chunks repeated. I'm not sure that contrib users would understand why they need to implement two functions. There should be a better way we can do it with one hook.

3. +  // Convert the action into the node property (e.g. 'publish_on').
   +  $action = $action . '_on';
   +
   +  return $result && !empty($node->$key);
   

   The first line should be $key = $action . '_on';

4. Actually in that function you could initialise $result to !empty($node->$key) at the top, instead of TRUE. Then the return would be a simple return $result at the end.

Nice work though. It will be good when done.

Hi pfrenssen,

Thanks for your contribution and being willing to split this off into different issues. This has been committed:
http://drupalcode.org/project/scheduler.git/commitdiff/34ea8a2?hp=0d0be0...

-Rick