Flag database data not cleaned up on comment & node delete

Thanks for the report. Looks like you're correct and Flag does not clean up its data for comments. It does implement hook_user_cancel() and hook_node_delete(), it's only comments that have this problem.

First of all, sorry for my bad english. I have fixed the problem implementing hook_comment_delete and doing a little refactorization on hook_node_delete.

Re-roll with fixed doc-block and indent

This is a bug.

This require hook_update_n
with

DELETE FROM {flag_content} WHERE content_type = 'comment'
 AND NOT EXISTS (SELECT 1 FROM {comments} c WHERE content_id=c.cid)
 
DELETE FROM {flag_counts} WHERE content_type = 'comment'
 AND NOT EXISTS (SELECT 1 FROM {comments} c WHERE content_id=c.cid)
 

You have actually fixed 2 bugs with #7. See #1650810: flag_node_delete() does not delete nodes. I've included a patch there that would clean-up and fix both issues.

Fixed patch:
- added cleanup for nodes from #1650810: flag_node_delete() does not delete nodes
- comment module could be disabled so we need to check for it's table.
- fixed table name {comment}
- wrong $cid changed to $content_id parameter

Credits to iStryker for update function for {node}

Thank you for change and update the patch, this was my first patch and I didn't know the rules for patches, sorry.

Thanks for the patch. The patch did not apply cleanly for me. I got the following errors trying to apply the patch:
patch:9: Trailing white space.
patch:10: Trailing white space.
/**
patch:11: Trailing white space.
* Clean-up flag records for deleted nodes and comments.
patch:12: Trailing white space.
*/
patch:12: Trailing white space.
function flag_update_7202() {
fatal: corrupt patch at line 89

@rwilson0429 do you trying to apply patch against 7.x-2.x-dev?

@andypost, yes I am trying to apply the patch against 7.x-2.x-dev.

When I tried to apply the patch on Windows7 using cgywin, I got the error messages in #11.

When I tried to apply the patch on a Lynux server, I didn't get the whitespace or function errors but, I did get the 'fatal: corrupt patch at line 89' error.

Looks pretty good! A few problems though:

+++ b/flag.install
@@ -462,3 +462,18 @@ function flag_update_6208() {
+  db_query("DELETE FROM {flag_content} WHERE content_type = 'node' AND NOT EXISTS (SELECT 1 FROM {node} n WHERE content_id=n.nid)");

db_query() can only be used for SELECT. These should be http://api.drupal.org/api/drupal/includes!database!database.inc/function...

+++ b/flag.module
@@ -584,6 +579,38 @@ function flag_node_translation_change($node) {
+ * Deletes flagging records for the node or comment.

Given we're about to go to any entity, let's keep this comment generic about content types!

+++ b/flag.module
@@ -584,6 +579,38 @@ function flag_node_translation_change($node) {
+ *   The type of content being deleted, 'node' or 'comment'.

Ditto.

+++ b/flag.module
@@ -584,6 +579,38 @@ function flag_node_translation_change($node) {
+ *   The ID of the node or comment being deleted.

And here too.

+++ b/flag.module
@@ -745,7 +772,7 @@ function flag_session_api_cleanup($arg = 'run') {
- * Implements hook_node_type().
+ * Implements hook_node_type_delete().
  */

That's a separate problem, shouldn't be in this patch.

Also, we should refactor flag_user_account_removal() to make use of the new helper -- but that can be done as a follow-on, or we do all the the refactoring to _flag_content_delete() first as a task of its own.

Not sure that it's possible to hook all entities by the same way because for nodes and users we need special handlers :( So I changed signature

patch should have to user db_query() to delete records because of core limits, see #1267508: Subselects don't work in DBTNG conditions, except when used as value for IN

Filed RTBC'ed #1686644: Chnge php-doc hook_node_type() to hook_node_type_delete()

Let's fix this bug and start unification in #1035410: Flag any entity

Agreed.

Though I don't know how much of a clean-up this issue should be, as I've just spotted this:

/**
 * Implements hook_translation_change().
 */
function flag_node_translation_change($node) {

I can't find any hook that matches either hook_translation_change() or hook_node_translation_change().

So this comment is a lie:

    // If the flag is being tracked by translation set and the node is part
    // of a translation set, don't delete the flagging record.
    // Instead, data will be updated in the 'translation_change' op, below.

But I'm happy for that to be another issue.

@joachim this hook related to i18n staff, and implemented by http://drupal.org/project/translation_helpers not sure how this works but prefere do not touch that places see #318328: Hook to respond to change of source translation

EDIT Let's fix the bug and then continue unification and clean-up in follow-ups, they are out of scope for this patch

> patch should have to user db_query() to delete records because of core limits, see #1267508: Subselects don't work in DBTNG conditions, except when used as value for IN

I added a note about this to the update hook, and also fixed the spacing in the queries there.

Committed the slightly tweaked version. Thanks for everyone's work on this!

Issue #1596492 by andypost, rafamatito: Fixed flag database data not cleaned up on comment & node delete.

Changing the version for backporting this.

The patch is going to be significantly different on D6:

- implement http://api.drupal.org/api/drupal/developer!hooks!core.php/function/hook_... to delete a comment's flagging records
- in hook_nodeapi, find the node's comments and clean those up too (which may prove problematic if http://api.drupal.org/api/drupal/modules!comment!comment.module/function... has fired first... gee thanks comment module for not firing hook_comment() there!
- clean up data from the table in an update hook

I'd say let's not bother with the refactoring to a helper.

This blocks beta 8 if the both 2.x branches' releases are to remain in sync.

What if we schedule a cron job to delete any "orphaned" comment flags? That will take care of anything that hook_comment() doesn't bring to our attention?

That seems a bit heavy-handed to me.

Here's how another module that adds stuff to comments tackles it:

 238 if ($op == 'delete') {
239 $result = comment_upload_fetch_all($node->nid, TRUE);
240 while ($row = db_fetch_array($result)) {
241 // comment_upload_delete_file just needs a filepath and a fid.
242 comment_upload_delete_file($row);
243 }
244 }

http://drupalcode.org/project/comment_upload.git/blob/refs/heads/6.x-1.x...

Given this is in use on d.org too, I think we can safely assume it's the Right Way(TM) :)

Understood. Here's my first stab at the patch for 6.x-2.x. (Universe; I hope I did that right.)

Thanks for working on this!

Patch is looking good, just a few small tweaks:

+++ b/flag.install
@@ -660,6 +660,22 @@ function flag_update_6208() {
+ * Clean up orphaned flags due to node and comment deletion

Needs a final full stop.

+++ b/flag.install
@@ -660,6 +660,22 @@ function flag_update_6208() {
+  db_query("DELETE FROM {flag_content} WHERE content_type = 'node' AND NOT EXISTS (SELECT 1 FROM {node} n WHERE content_id=n.nid)");

Needs a space either side of the '='.

+++ b/flag.module
@@ -474,6 +474,18 @@ function flag_nodeapi(&$node, $op, $teaser = NULL, $page = NULL) {
+          ¶

Some whitespace errors here.

+++ b/flag.module
@@ -977,6 +989,28 @@ function flag_users($users, $action, $flag_name) {
+ *
+ * @param StdClass $comment
+ *   The comment object being modified.
+ * @param string $op
+ *   The operation being performed.

No need to give parameters for a hook implementation.

+++ b/flag.module
@@ -977,6 +989,28 @@ function flag_users($users, $action, $flag_name) {
+      WHERE content_type = 'comment'
+      AND content_id = %d", $comment->cid);

I'm not sure these queries are so long they warrant this, but it's your call. But if you do wrap them, these two lines need to be indented. The others should maybe be 2 rather than 4 spaces? But check with core / other modules.

Found out my IDE was still using tabs for tabs instead of spaces. Should be fixed now as well as all above issues.

The SQL statements are wrapped as they are as that's how a Enterprise DBA taught me. It's a hard habit to break, but it really keeps things clean for longer queries. Hopefully the spaces are preserved correctly in this version.

I'm all in favour of readability, so keep the habit :)

+++ b/flag.module
@@ -474,6 +474,18 @@ function flag_nodeapi(&$node, $op, $teaser = NULL, $page = NULL) {
+          ¶

Whitespace here.

Whitespace is a bane. I use a text editor that has a command to trim it, I use a git gui that shows me whitespace in uncommitted changes, and for patch reviews, dreditor which shows them too. And still they creep in!

+++ b/flag.module
@@ -977,6 +989,23 @@ function flag_users($users, $action, $flag_name) {
+ * Implements hook_comment()

Missing full stop.

+++ b/flag.module
@@ -977,6 +989,23 @@ function flag_users($users, $action, $flag_name) {
+  switch ($op) {
+    case 'delete':

That's very diligent of you, making a one-case switch() block. I'd have been lazy and made it an if(). ;)

I'm not sure that the nodeapi call to query for all comment ID's attached to node X is ideal ... unless there needs to be an event attached to this.
Although this will work, looping over every comment attached to delete it could be exceptionally slow.

http://www.postgresql.org/docs/8.2/static/sql-delete.html
http://dev.mysql.com/doc/refman/5.0/en/delete.html

I think this style query may be more appropriate:

DELETE FROM {flag_content} 
WHERE content_type = 'comment'
AND content_id IN (
 SELECT cid 
 FROM {comments}
 WHERE nid = ?
);

@joachim Hopefully I fixed the whitespace issue this. Stop should be added. As for the switch statement, I guess I just got back into the habit back from the few times I tried to write modules.

@wjaspers That is a great idea! In the attached patch.

I have a splitting headache this morning so I honestly can't tell what's changed with the query, but the whitespace is fixed.

Issue #1596492 by socketwench, andypost, rafamatito: Fixed flag database data not cleaned up on comment & node delete.