By seanbfuller on

I am working on a module that needs to delete an extended node after 30 days. I have the cron job set up fine and I am correctly calling the hook_cron. I have debug messages going into watchdog letting me know that I am getting the proper nids of nodes that are old from an sql statement. Now I am just wondering the best way to actually delete these nodes. I looked at node_delete and it seemed to be the cleanest way to delete the node and all accompanying data.

Currently I am calling:

    $this_edit['nid'] = $del->nid;
    $this_edit['confirm'] = TRUE;
    node_delete($this_edit);

The problem is that the nodes are not actually being deleted. I think it is because of the access control statement:

  if (node_access('delete', $node)) {

Since cron acts as an Anonymous user, it fails this access check.

For now I can just copy the code from node_delete into my module, but this seems like a bad programming practice and I'd rather have a cleaner function to do this for me. Is there one that I missed? If not, should this be a feature request for node.module?

Thanks in advance.

Comments

robertdouglass’s picture

robertdouglass commented

You could change users for that operation;

global $user;
$temp_user = $user;
$user = user loaded from $node->nid;
// delete node
$user = $temp_user;

- Robert Douglass

-----
If this helped you, please take the time to rate the value of this post: http://rate.affero.net/robertDouglass/

www.hornroller.com, www.robshouse.net

seanbfuller’s picture

seanbfuller commented

Thanks, that's exactly the kind of answer I was looking for. Here is my finished code for anyone else who needs it.

/*
 * Example implementation for hook cron
 * will delete nodes of type 'example' that are 30 days old
 */
function example_cron(){
  $today = getdate();
  $expired_time = strtotime("-30 days");
  $query = "SELECT nid, uid FROM {node} WHERE type='example' AND created <= '$expired_time'";
  $queryResult =  db_query($query);
  while ($del = db_fetch_object($queryResult)) {
    global $user;
    $owner_account = user_load(array('uid' => $del->uid));
    $temp_user = $user;
    $user = $owner_account;
    $this_edit['nid'] = $del->nid;
    $this_edit['confirm'] = TRUE;
    node_delete($this_edit);
    $user = $temp_user;  	
  }
}

--------------------
Sean B. Fuller
www.seanbfuller.com

robertdouglass’s picture

robertdouglass commented

The code might be a bit cleaner like this:

/*
 * Example implementation for hook cron
 * will delete nodes of type 'example' that are 30 days old
 */
function example_cron(){
  global $user;
  $temp_user = $user;
  $today = getdate();
  $expired_time = strtotime("-30 days");
  $query = "SELECT nid, uid FROM {node} WHERE type='%s' AND created <= '%s';
  $queryResult = db_query($query, 'example', "$expired_time");
  while ($del = db_fetch_object($queryResult)) {
    $user = user_load(array('uid' => $del->uid));
    $this_edit['nid'] = $del->nid;
    $this_edit['confirm'] = TRUE;
    node_delete($this_edit);  
  }
  $user = $temp_user;
}

Note that using %s placeholders for the strings and having them replaced by db_query is one of Drupal's security features that coders are responsible for taking advantage of. While it is not needed for the 'example' string, the second is dynamic and therefore a must. I replaced both since this example code is likely to be copied and modified and who knows how long before the 'example' string is also made to be dynamic.

- Robert Douglass

-----
If this helped you, please take the time to rate the value of this post: http://rate.affero.net/robertDouglass/

www.hornroller.com, www.robshouse.net

seanbfuller’s picture

seanbfuller commented

OK, so would it be correct to say that the general rule for best practice is to use the %s placeholders for the strings any time you have dynamic content going into an sql statement? I was under the impression that you should use it any time you are dealing with user input data. From what you are saying it sounds like it is better to just use it as much as possible. Thanks for the advice.

--------------------
Sean B. Fuller
www.seanbfuller.com

robertdouglass’s picture

robertdouglass commented

Rather than expect that every developer will always think "is there a chance that user input might get put into this variable?" I suggest using them for every non-literal. I'd be interested in hearing other developers' views as well - my answers are far from definitive. For the sake of an example in a forum thread, I would always use them no matter what, though, since the construct is new to most PHP developers not familiar with Drupal, and I want them to see the syntax and wonder what it is for.

- Robert Douglass

-----
If this helped you, please take the time to rate the value of this post: http://rate.affero.net/robertDouglass/

www.hornroller.com, www.robshouse.net

seanbfuller’s picture

seanbfuller commented

Here is the final code. Note that I got rid of the getdate call since I am not actually using it, and there was a parse error with the code above due to a missing quote at the end of one of the lines.

Thanks for the help.

/**
 * Implementation of hook_cron
 * Gets all nids for nodes of type 'example' that 
 * were created 30 or more days ago
 * and calls node_delete with the confirmation flag on each.
 */
function example_cron(){
  global $user;
  $temp_user = $user;
  $expired_time = strtotime("-30 days");
  $query = "SELECT nid, uid FROM {node} WHERE type='%s' AND created <= '%s'";
  $queryResult = db_query($query, 'example', $expired_time);
  while ($del = db_fetch_object($queryResult)) {
    $user = user_load(array('uid' => $del->uid));
    $this_edit['nid'] = $del->nid;
    $this_edit['confirm'] = TRUE;
    node_delete($this_edit);
  }
  $user = $temp_user;   
}

--------------------
Sean B. Fuller
www.seanbfuller.com

jeepfreak’s picture

jeepfreak commented

Sorry to butt in, but could you please explain (or point me to an explaination) of the %s placeholders? I don't understand how they work and %s isn't a very 'search friendly' term.
Thanks,
Billy

Torenware’s picture

Torenware commented

The %s is a "printf" style placeholder. You can get a full list of them from the PHP Manual.

Rob Thorne
Torenware Networks
http://www.torenware.com

Rob Thorne
Torenware Networks

jeepfreak’s picture

jeepfreak commented

Thank you sir! That's what I thought, I just couldn't understand how it new what to print in place of the placeholder (since I saw the same one used for two different variables).
Billy

slik’s picture

slik commented

Hi, this code is working width Drupal 6? And where I put this code to start delete old nodes?

Thank you.

luckysmack’s picture

luckysmack commented

I tried to get this to work under D6 by trying to make it a module but it doesnt work. how can i get this to work in D6? or at least something similar

-----------------------------------------------------------------------------------------------------------
"The level of our success is limited only by our imagination" -Aesop

mcjim’s picture

mcjim commented

Try this, but please test it thoroughly first! The devel module can generate lots of dummy content you can test it on, in your testing environment.
I've added in some code to select content created by anonymous users only. You can remove this if you wish.

If you haven't already created a module, create a new directory in sites/all/modules called delete_old_nodes. Inside this directory create a file called delete_old_nodes.info and put this in it:

name = Delete old nodes
description = Deletes nodes older than 30 days.
package = Custom
core = 6.x

Create a file called delete_old_nodes.module and put this in it:

<?php
/**
* Implementation of hook_cron
* Gets all nids for nodes of type 'example' that 
* were created 30 or more days ago
* and calls node_delete.
*/
function delete_old_nodes_cron(){
 $expired_time = strtotime("-30 days");
 $uid = 0; // 0 is the uid for anonymous users
 $type = 'example'; // change this to the node type you want to delete
 $result = db_query("SELECT nid FROM {node} WHERE type = '%s' AND created <= '%s' AND uid = %d", $type, $expired_time, $uid);
 while ($node = db_fetch_object($result)) {
   node_delete($node->nid);
 }  
}

You should now be able to go to admin/build/modules and enable this module.

You can run cron by going to example.com/cron.php

For testing, you can change the $expired_time to strtotime("-5 seconds")

slik’s picture

slik commented

Thank you, is working fine.
But I think that the wright way to delete old node will be to select only nodes width very few views.
Some how I need to check every node in node_counter table to test if the total node views are below 50 views. How can I do this?