I'm trying to convert inactive_user to Drupal 7 and could use some help with hook_cron(). My work so far includes:

  • Create a solution for _inactive_user_admin_mail().
  • Repair hook_permission() and hook_menu() so permissions work and the menu path is admin/people/inactive-user
  • Converted all SELECT queries to static queries, specifying all literal values with placeholders
  • Converted all UPDATE, INSERT, and DELETE queries to dynamic queries, specifying all literal values with placeholders
  • Removed _format_interval() since format_interval() in common.inc now includes months
  • Renamed inactive_user_custom_settings_validate() to inactive_user_validate(). This made the screen error go away although I can't say that I know for sure where $edit and $count come from

inactive_user_custom_settings() seems fine as does inactive_user_schema(). I'm asking for help to make the queries in inactive_user_cron() actually do something.

Since db_fetch_object() and db_affected_rows() were deprecated in D7, the structure of the queries needs to be changed. I think I'm on the right track but am missing some knowledge of DBTNG.

I've attached a patch of my work. Thanks for your help.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

esod’s picture

I guess the lack of response to this issue is because my patch had all kinds of context switching. I'll rephrase my question and describe where I'm stuck.

In Drupal 6, a query in inactive_user_cron is:

    // reset notifications if recent user activity
    $users = db_fetch_object(db_query('SELECT uid FROM {inactive_users} WHERE uid <> 1'));
    if ($users) {
      foreach ($users as $uid) {
        $u = db_fetch_object(db_query('SELECT access, name FROM {users} WHERE uid = %d', $uid));
        if ($u->access > time() - 604800) {
          // user activity in last week, remove from inactivity table
          db_query('DELETE FROM {inactive_users} WHERE uid = %d', $uid);
          watchdog('user', 'recent user activity: %user removed from inactivity list', array('%user' => $u->name), WATCHDOG_NOTICE, l(t('edit user'), "user/$uid/edit", array('query' => array('destination' => 'admin/user/user'))));
        }
      }
    }

For Drupal 7, I've written the same query as:

    // reset notifications if recent user activity
    $users = db_query('SELECT uid FROM {inactive_users} WHERE uid <> :one', array(':one' => 1));
    $record = $users->fetchObject();
    if ($record) {
      foreach ($record as $uid) {
        $u = db_query('SELECT access, name FROM {users} WHERE uid = :uid', array(':uid' => $uid));
        $u_object = $u->fetchObject();
        if ($u_object->access > REQUEST_TIME - 604800) {
          // user activity in last week, remove from inactivity table
          db_delete('inactive_users')
            ->condition('uid', $uid)
            ->execute();
          watchdog('user', 'recent user activity: %user removed from inactivity list', array('%user' => $u_object->name), WATCHDOG_NOTICE, l(t('edit user'), "user/" . $uid . "/edit", array('query' => array('destination' => 'admin/user/user'))));
        }
      }
    }

Looking at the $users query, for instance, and knowing that db_fetch_object() has been deprecated in Drupal 7, I thought up:

    $users = db_query('SELECT uid FROM {inactive_users} WHERE uid <> :one', array(':one' => 1));
    $record = $users->fetchObject();
    if ($record) {
      foreach ($record as $uid) {
    ....

instead of:

    $users = db_fetch_object(db_query('SELECT uid FROM {inactive_users} WHERE uid <> 1'));
    if ($users) {
      foreach ($users as $uid) {
    ...

For Drupal 6.

If someone can help me with this one query, I'm pretty sure I can apply that knowledge to the rest of queries in inactive_user_cron, at which point the module will be upgraded to Drupal 7.

Thanks for your help.

thebruce’s picture

Status: Needs work » Needs review
FileSize
32.87 KB

Hi,

Here is a patch with updates to the module and tests. The module works to the tests and works functionally.

I deviated a bit from the plan above in the query structure, on the selects I used db_select. I realize this has some performance concerns as indicated by Crell in the select api docs comments. What I thought this might lead to is the the creation of an implementing class where, since many of the criteria clauses are re-used, could be made methods of the inactive user db object. I think this might be a fair choice for extension and maintenance.

Anyway, I wanted to get the patch for the straight up port contributed since this issue had been outstanding for quite a while.

Hope this helps.

----
thebruce

rgristroph’s picture

I looked over this patch, and tried it out.

It looks good to me. The only comments I have are:

  1. We should probably make a branch named 7.x-1.x and work from that instead of using master. There's a guide on what the standard process is somewhere, but I couldn't find it, maybe that's part of the problem :)
  2. I could not find a link to /admin/people/inactive-user in any part of the admin interface, even on the modules page, I had to type it in by hand.
  3. The pulldowns for the time periods, that used to be things like "1 week" or "2 months", are now just the number of seconds.

I don't think any of those things should stop this patch from going forward, it's a big improvement. Doing all the queries correctly is a big win.

--Rob

deekayen’s picture

Status: Needs review » Needs work

I committed #2 just to keep things moving.

The guide you referenced is probably https://drupal.org/empty-git-master.

rgristroph’s picture

Thanks !

Just to confirm, since I did the test of the patch at #2 my test site has sent the appropriate notifications on the appropriate users I created, so it does seem to work right.

Besides the two things I noted - a link to the configuration and numeric vs. human readable times - is there anything else keeping this issue from being actually closed ?

--Rob