Deleting a project does not delete maintainers [#1003674]

Just discovered this while working on #1003552: Pass the project to hook_project_permission_info(). Creating a project automatically updates the maintainer information in project_project_insert and again in project_proejct_update(), but project_project_delete() contains no attempt to delete from project_maintainers and invoke a hook to allow other modules to do the same.

A close look at the existing hooks, reveal that they aren't quire adequate for handling this unless we expect other modules to check for $uid being set in hook_proejct_maintainer_remove() and adjust their query respectively.

/**
 * Remove a maintainer from a given project.
 *
 * @param $nid
 *   The Project NID to remove the maintainer from.
 * @param $uid
 *   The user ID of the maintainer to remove.
 */
function project_maintainer_remove($nid, $uid) {
  db_query("DELETE FROM {project_maintainer} WHERE nid = %d and uid = %d", $nid, $uid);

  // Invoke hook_project_maintainer_remove() to let other modules know this
  // maintainer is being removed so they can take any actions or record any
  // data they need to.
  module_invoke_all('project_maintainer_remove', $nid, $uid);
}

For that reason I'd just adding a new hook, such hook_project_maintainer_delete_project() or something similar.

Comments

Comment #1

dww

we/he/they

commented 12 August 2011 at 05:45

Priority:

Critical

» Normal

This doesn't seem critical. Worst case is some stale rows in a few tables. Everything still functions fine.

I'm not sure we need a whole new hook. Why not just say (and document) that if hook_project_maintainer_remove() is called with $uid = 0 it means to delete everyone? Or, for that matter, just iterate and call the hook N times -- project deletion is an exceptionally rare operation, so I don't mind if it's a bit inefficient.

Also, in the case of the _save() hook, we already *do* iterate over all the users and invoke the hook separately for each one. So there's precedent for just iterating and invoking these hooks N times for N maintainers.

If we're going to change the API at all, it'd be nice to do it in conjunction with #1171828: Let other modules know when a new maintainer is added which is going to be breaking the API anway. ;)

Comment #2

mikey_p commented 27 April 2012 at 18:55

I think it'd be better to just recommend that modules implement hook_nodeapi or hook_node_delete (d7) than introduce a weird API or loop over these items.