[drupal-devel] [bug] comment stats don't update after approval in moderation queue
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: 4.5.2 Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: ankur Status: patch Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ankur
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: 4.5.2 Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: ankur Status: patch Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ankur Previous comments: ------------------------------------------------------------------------ March 1, 2005 - 22:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur
Issue status update for http://drupal.org/node/18215 Project: Drupal -Version: 4.5.2 +Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: killes@www.drop.org Status: patch Applies also to head. ankur: Please create patches from the Drupal root directory. killes@www.drop.org Previous comments: ------------------------------------------------------------------------ March 1, 2005 - 23:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 01:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: Dries Status: patch I'm not a big fan of hidden fields. I'll try to think of an alternative fix. Dries Previous comments: ------------------------------------------------------------------------ March 2, 2005 - 00:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 02:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ------------------------------------------------------------------------ March 14, 2005 - 00:06 : killes@www.drop.org Applies also to head. ankur: Please create patches from the Drupal root directory.
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: Steven Status: patch Is there any harm in always calling _comment_update_node_statistics() after editing a node? As far as I can tell, it's a pretty benign call. The only thing odd in there is: $node = node_load(array('nid' => $nid)); But I don't see $node being used anywhere in that function. Anyone have an idea why? ;) Steven Previous comments: ------------------------------------------------------------------------ March 1, 2005 - 23:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 01:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ------------------------------------------------------------------------ March 13, 2005 - 23:06 : killes@www.drop.org Applies also to head. ankur: Please create patches from the Drupal root directory. ------------------------------------------------------------------------ March 15, 2005 - 22:24 : Dries I'm not a big fan of hidden fields. I'll try to think of an alternative fix.
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: chx Status: patch please do not make node.module dependent on comment.module -- use module_invoke if you implement what Steven suggested. chx Previous comments: ------------------------------------------------------------------------ March 1, 2005 - 23:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 01:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ------------------------------------------------------------------------ March 13, 2005 - 23:06 : killes@www.drop.org Applies also to head. ankur: Please create patches from the Drupal root directory. ------------------------------------------------------------------------ March 15, 2005 - 22:24 : Dries I'm not a big fan of hidden fields. I'll try to think of an alternative fix. ------------------------------------------------------------------------ March 18, 2005 - 09:43 : Steven Is there any harm in always calling _comment_update_node_statistics() after editing a node? As far as I can tell, it's a pretty benign call. The only thing odd in there is: $node = node_load(array('nid' => $nid)); But I don't see $node being used anywhere in that function. Anyone have an idea why? ;)
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: Junyor@drupal.org Status: patch I don't see any reason for that node_load() call either. Junyor@drupal.org Previous comments: ------------------------------------------------------------------------ March 1, 2005 - 22:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 00:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ------------------------------------------------------------------------ March 13, 2005 - 22:06 : killes@www.drop.org Applies also to head. ankur: Please create patches from the Drupal root directory. ------------------------------------------------------------------------ March 15, 2005 - 21:24 : Dries I'm not a big fan of hidden fields. I'll try to think of an alternative fix. ------------------------------------------------------------------------ March 18, 2005 - 08:43 : Steven Is there any harm in always calling _comment_update_node_statistics() after editing a node? As far as I can tell, it's a pretty benign call. The only thing odd in there is: $node = node_load(array('nid' => $nid)); But I don't see $node being used anywhere in that function. Anyone have an idea why? ;) ------------------------------------------------------------------------ March 18, 2005 - 08:47 : chx please do not make node.module dependent on comment.module -- use module_invoke if you implement what Steven suggested.
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: Steven Status: patch chx: Actually I made a typo... it's about calling this after editing a comment, not a node. So there is no dependency. Steven Previous comments: ------------------------------------------------------------------------ March 1, 2005 - 23:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 01:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ------------------------------------------------------------------------ March 13, 2005 - 23:06 : killes@www.drop.org Applies also to head. ankur: Please create patches from the Drupal root directory. ------------------------------------------------------------------------ March 15, 2005 - 22:24 : Dries I'm not a big fan of hidden fields. I'll try to think of an alternative fix. ------------------------------------------------------------------------ March 18, 2005 - 09:43 : Steven Is there any harm in always calling _comment_update_node_statistics() after editing a node? As far as I can tell, it's a pretty benign call. The only thing odd in there is: $node = node_load(array('nid' => $nid)); But I don't see $node being used anywhere in that function. Anyone have an idea why? ;) ------------------------------------------------------------------------ March 18, 2005 - 09:47 : chx please do not make node.module dependent on comment.module -- use module_invoke if you implement what Steven suggested. ------------------------------------------------------------------------ March 18, 2005 - 10:03 : Junyor@drupal.org I don't see any reason for that node_load() call either.
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: Junyor@drupal.org Status: patch While looking through the code, I thought this would be a very nice case where a comment API would come in handy. @Steven: I agree, we should call _update_node_comment_statistics() any time a comment is added, edited, or deleted. Junyor@drupal.org Previous comments: ------------------------------------------------------------------------ March 1, 2005 - 22:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 00:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ------------------------------------------------------------------------ March 13, 2005 - 22:06 : killes@www.drop.org Applies also to head. ankur: Please create patches from the Drupal root directory. ------------------------------------------------------------------------ March 15, 2005 - 21:24 : Dries I'm not a big fan of hidden fields. I'll try to think of an alternative fix. ------------------------------------------------------------------------ March 18, 2005 - 08:43 : Steven Is there any harm in always calling _comment_update_node_statistics() after editing a node? As far as I can tell, it's a pretty benign call. The only thing odd in there is: $node = node_load(array('nid' => $nid)); But I don't see $node being used anywhere in that function. Anyone have an idea why? ;) ------------------------------------------------------------------------ March 18, 2005 - 08:47 : chx please do not make node.module dependent on comment.module -- use module_invoke if you implement what Steven suggested. ------------------------------------------------------------------------ March 18, 2005 - 09:03 : Junyor@drupal.org I don't see any reason for that node_load() call either. ------------------------------------------------------------------------ March 18, 2005 - 09:17 : Steven chx: Actually I made a typo... it's about calling this after editing a comment, not a node. So there is no dependency.
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: ankur -Status: active +Status: patch Attachment: http://drupal.org/files/issues/comment.module_7.diff (2.27 KB) Here's an alternative patch. This patch changes comment_admin_edit(): Previously, the function would check to see if edits to the comment were being $_POSTed(). If this was the case, there would be a call to comment_save() and then a drupal_goto() back to q=admin/comment. Otherwise, the comment would be loaded and an edit form would be displayed. The problem was that the comment count for the related node was not being updated and that in order to do so, you have to know the nid of the comment (the single parameter to _comment_update_node_statistics()). However, the form being posted did not contain the nid for the comment. The previous patch passed the nid (and prior publish status of the comment) through the form via hidden HTML form fields. With this patch, the loading of the comment is done first. After, loading the comment, we check to see if changes to the comment are being $_POSTed. Then we save and see if _update_comment_node_statistics() should be called. This patch is a way around having to pass hidden fields. However, the drawback (and I can't really quantify how serious of a performance penalty this would mean) is that a comment has to be loaded again when changes are submitted whereas with the previous patch, changing the comment would just mean saving the comment without having to reload it (since the nid and previous status were passed via hidden fields). In anycase, I don't know if there's another solution around this without causing some more major changes. Frankly, it would be cool if the update node could distinguish the approved comment as "1 new comment" rather than "1 comment". But then that would require some way of telling whether a comment was being approved for the first time or if the comment was edited and resubmitted to the approval queue... -Ankur ankur Previous comments: ------------------------------------------------------------------------ March 1, 2005 - 22:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 00:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ------------------------------------------------------------------------ March 13, 2005 - 22:06 : killes@www.drop.org Applies also to head. ankur: Please create patches from the Drupal root directory. ------------------------------------------------------------------------ March 15, 2005 - 21:24 : Dries I'm not a big fan of hidden fields. I'll try to think of an alternative fix. ------------------------------------------------------------------------ March 18, 2005 - 08:43 : Steven Is there any harm in always calling _comment_update_node_statistics() after editing a node? As far as I can tell, it's a pretty benign call. The only thing odd in there is: $node = node_load(array('nid' => $nid)); But I don't see $node being used anywhere in that function. Anyone have an idea why? ;) ------------------------------------------------------------------------ March 18, 2005 - 08:47 : chx please do not make node.module dependent on comment.module -- use module_invoke if you implement what Steven suggested. ------------------------------------------------------------------------ March 18, 2005 - 09:03 : Junyor@drupal.org I don't see any reason for that node_load() call either. ------------------------------------------------------------------------ March 18, 2005 - 09:17 : Steven chx: Actually I made a typo... it's about calling this after editing a comment, not a node. So there is no dependency. ------------------------------------------------------------------------ March 18, 2005 - 09:21 : Junyor@drupal.org While looking through the code, I thought this would be a very nice case where a comment API would come in handy. @Steven: I agree, we should call _update_node_comment_statistics() any time a comment is added, edited, or deleted. ------------------------------------------------------------------------ March 22, 2005 - 18:40 : Dries Not committing the patch as is. Let's see what the discussion results in.
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: Dries Status: patch Not sure. comment_save() actually calls _comment_update_node_statistics() ... and you added a call to _comment_update_node_statistics() right after a call to comment_save()? Dries Previous comments: ------------------------------------------------------------------------ March 2, 2005 - 00:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 02:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ------------------------------------------------------------------------ March 14, 2005 - 00:06 : killes@www.drop.org Applies also to head. ankur: Please create patches from the Drupal root directory. ------------------------------------------------------------------------ March 15, 2005 - 23:24 : Dries I'm not a big fan of hidden fields. I'll try to think of an alternative fix. ------------------------------------------------------------------------ March 18, 2005 - 10:43 : Steven Is there any harm in always calling _comment_update_node_statistics() after editing a node? As far as I can tell, it's a pretty benign call. The only thing odd in there is: $node = node_load(array('nid' => $nid)); But I don't see $node being used anywhere in that function. Anyone have an idea why? ;) ------------------------------------------------------------------------ March 18, 2005 - 10:47 : chx please do not make node.module dependent on comment.module -- use module_invoke if you implement what Steven suggested. ------------------------------------------------------------------------ March 18, 2005 - 11:03 : Junyor@drupal.org I don't see any reason for that node_load() call either. ------------------------------------------------------------------------ March 18, 2005 - 11:17 : Steven chx: Actually I made a typo... it's about calling this after editing a comment, not a node. So there is no dependency. ------------------------------------------------------------------------ March 18, 2005 - 11:21 : Junyor@drupal.org While looking through the code, I thought this would be a very nice case where a comment API would come in handy. @Steven: I agree, we should call _update_node_comment_statistics() any time a comment is added, edited, or deleted. ------------------------------------------------------------------------ March 22, 2005 - 20:40 : Dries Not committing the patch as is. Let's see what the discussion results in. ------------------------------------------------------------------------ April 6, 2005 - 23:12 : ankur Attachment: http://drupal.org/files/issues/comment.module_7.diff (2.27 KB) Here's an alternative patch. This patch changes comment_admin_edit(): Previously, the function would check to see if edits to the comment were being $_POSTed(). If this was the case, there would be a call to comment_save() and then a drupal_goto() back to q=admin/comment. Otherwise, the comment would be loaded and an edit form would be displayed. The problem was that the comment count for the related node was not being updated and that in order to do so, you have to know the nid of the comment (the single parameter to _comment_update_node_statistics()). However, the form being posted did not contain the nid for the comment. The previous patch passed the nid (and prior publish status of the comment) through the form via hidden HTML form fields. With this patch, the loading of the comment is done first. After, loading the comment, we check to see if changes to the comment are being $_POSTed. Then we save and see if _update_comment_node_statistics() should be called. This patch is a way around having to pass hidden fields. However, the drawback (and I can't really quantify how serious of a performance penalty this would mean) is that a comment has to be loaded again when changes are submitted whereas with the previous patch, changing the comment would just mean saving the comment without having to reload it (since the nid and previous status were passed via hidden fields). In anycase, I don't know if there's another solution around this without causing some more major changes. Frankly, it would be cool if the update node could distinguish the approved comment as "1 new comment" rather than "1 comment". But then that would require some way of telling whether a comment was being approved for the first time or if the comment was edited and resubmitted to the approval queue... -Ankur
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: ankur Status: patch Attachment: http://drupal.org/files/issues/comment.module_10.diff (2.1 KB) I'm submitting 2 more patches here. I didn't realize that comment_save() made the call to _comment_update_node_statistics(). Well, I took a look at comment_save() and realize that it passes $edit['nid'] as the single parameter to _comment_update_node_statistics(). Now, comment_admin_edit() passes $edit to comment_save() as the 2nd param. The problem now is that there is no 'nid' key in the $edit that is passed to comment_save(). This patch avoids passing the nid as a hidden field. It works similarly to the 2nd patch I submitted above by moving the comment loading to the beginning of comment_admin_edit() and then figuring out what to do (in terms of displaying the edit-form vs. saving an edit and redirecting back to q=admin/comment). It was noted above that hidden fields in this case are to be avoided. This patch doesn't do that, but the next follow-up by me has a patch that does it. I think that's the way to go (see below). -Ankur ankur Previous comments: ------------------------------------------------------------------------ March 1, 2005 - 22:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 00:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ------------------------------------------------------------------------ March 13, 2005 - 22:06 : killes@www.drop.org Applies also to head. ankur: Please create patches from the Drupal root directory. ------------------------------------------------------------------------ March 15, 2005 - 21:24 : Dries I'm not a big fan of hidden fields. I'll try to think of an alternative fix. ------------------------------------------------------------------------ March 18, 2005 - 08:43 : Steven Is there any harm in always calling _comment_update_node_statistics() after editing a node? As far as I can tell, it's a pretty benign call. The only thing odd in there is: $node = node_load(array('nid' => $nid)); But I don't see $node being used anywhere in that function. Anyone have an idea why? ;) ------------------------------------------------------------------------ March 18, 2005 - 08:47 : chx please do not make node.module dependent on comment.module -- use module_invoke if you implement what Steven suggested. ------------------------------------------------------------------------ March 18, 2005 - 09:03 : Junyor@drupal.org I don't see any reason for that node_load() call either. ------------------------------------------------------------------------ March 18, 2005 - 09:17 : Steven chx: Actually I made a typo... it's about calling this after editing a comment, not a node. So there is no dependency. ------------------------------------------------------------------------ March 18, 2005 - 09:21 : Junyor@drupal.org While looking through the code, I thought this would be a very nice case where a comment API would come in handy. @Steven: I agree, we should call _update_node_comment_statistics() any time a comment is added, edited, or deleted. ------------------------------------------------------------------------ March 22, 2005 - 18:40 : Dries Not committing the patch as is. Let's see what the discussion results in. ------------------------------------------------------------------------ April 6, 2005 - 21:12 : ankur Attachment: http://drupal.org/files/issues/comment.module_7.diff (2.27 KB) Here's an alternative patch. This patch changes comment_admin_edit(): Previously, the function would check to see if edits to the comment were being $_POSTed(). If this was the case, there would be a call to comment_save() and then a drupal_goto() back to q=admin/comment. Otherwise, the comment would be loaded and an edit form would be displayed. The problem was that the comment count for the related node was not being updated and that in order to do so, you have to know the nid of the comment (the single parameter to _comment_update_node_statistics()). However, the form being posted did not contain the nid for the comment. The previous patch passed the nid (and prior publish status of the comment) through the form via hidden HTML form fields. With this patch, the loading of the comment is done first. After, loading the comment, we check to see if changes to the comment are being $_POSTed. Then we save and see if _update_comment_node_statistics() should be called. This patch is a way around having to pass hidden fields. However, the drawback (and I can't really quantify how serious of a performance penalty this would mean) is that a comment has to be loaded again when changes are submitted whereas with the previous patch, changing the comment would just mean saving the comment without having to reload it (since the nid and previous status were passed via hidden fields). In anycase, I don't know if there's another solution around this without causing some more major changes. Frankly, it would be cool if the update node could distinguish the approved comment as "1 new comment" rather than "1 comment". But then that would require some way of telling whether a comment was being approved for the first time or if the comment was edited and resubmitted to the approval queue... -Ankur ------------------------------------------------------------------------ April 7, 2005 - 19:52 : Dries Not sure. comment_save() actually calls _comment_update_node_statistics() ... and you added a call to _comment_update_node_statistics() right after a call to comment_save()?
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: ankur Status: patch Attachment: http://drupal.org/files/issues/comment.module.diff2 (721 bytes) This patch does what the very first patch does, only now I know that comment_save() itself calls _comment_update_node_statistics(). Again, the problem is that the $edit parameter that is passed from comment_admin_edit() to comment_save() does not contain a value for key 'nid'. This is a problem because comment_save() calls _comment_update_node_statistics() and passes $edit['nid'] as the single parameter to _comment_update_node_statistics(). There was a comment above against fixing this error by passing the 'nid' as a hidden field on the comment_admin_edit() page. However, we already pass the comment's 'cid' as a hidden field. We need to pass the 'nid' to comment save as well. My feeling is that this is the logical way as it is consistent with how the 'cid' is transmitted and saved... -Ankur ankur Previous comments: ------------------------------------------------------------------------ March 1, 2005 - 22:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 00:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ------------------------------------------------------------------------ March 13, 2005 - 22:06 : killes@www.drop.org Applies also to head. ankur: Please create patches from the Drupal root directory. ------------------------------------------------------------------------ March 15, 2005 - 21:24 : Dries I'm not a big fan of hidden fields. I'll try to think of an alternative fix. ------------------------------------------------------------------------ March 18, 2005 - 08:43 : Steven Is there any harm in always calling _comment_update_node_statistics() after editing a node? As far as I can tell, it's a pretty benign call. The only thing odd in there is: $node = node_load(array('nid' => $nid)); But I don't see $node being used anywhere in that function. Anyone have an idea why? ;) ------------------------------------------------------------------------ March 18, 2005 - 08:47 : chx please do not make node.module dependent on comment.module -- use module_invoke if you implement what Steven suggested. ------------------------------------------------------------------------ March 18, 2005 - 09:03 : Junyor@drupal.org I don't see any reason for that node_load() call either. ------------------------------------------------------------------------ March 18, 2005 - 09:17 : Steven chx: Actually I made a typo... it's about calling this after editing a comment, not a node. So there is no dependency. ------------------------------------------------------------------------ March 18, 2005 - 09:21 : Junyor@drupal.org While looking through the code, I thought this would be a very nice case where a comment API would come in handy. @Steven: I agree, we should call _update_node_comment_statistics() any time a comment is added, edited, or deleted. ------------------------------------------------------------------------ March 22, 2005 - 18:40 : Dries Not committing the patch as is. Let's see what the discussion results in. ------------------------------------------------------------------------ April 6, 2005 - 21:12 : ankur Attachment: http://drupal.org/files/issues/comment.module_7.diff (2.27 KB) Here's an alternative patch. This patch changes comment_admin_edit(): Previously, the function would check to see if edits to the comment were being $_POSTed(). If this was the case, there would be a call to comment_save() and then a drupal_goto() back to q=admin/comment. Otherwise, the comment would be loaded and an edit form would be displayed. The problem was that the comment count for the related node was not being updated and that in order to do so, you have to know the nid of the comment (the single parameter to _comment_update_node_statistics()). However, the form being posted did not contain the nid for the comment. The previous patch passed the nid (and prior publish status of the comment) through the form via hidden HTML form fields. With this patch, the loading of the comment is done first. After, loading the comment, we check to see if changes to the comment are being $_POSTed. Then we save and see if _update_comment_node_statistics() should be called. This patch is a way around having to pass hidden fields. However, the drawback (and I can't really quantify how serious of a performance penalty this would mean) is that a comment has to be loaded again when changes are submitted whereas with the previous patch, changing the comment would just mean saving the comment without having to reload it (since the nid and previous status were passed via hidden fields). In anycase, I don't know if there's another solution around this without causing some more major changes. Frankly, it would be cool if the update node could distinguish the approved comment as "1 new comment" rather than "1 comment". But then that would require some way of telling whether a comment was being approved for the first time or if the comment was edited and resubmitted to the approval queue... -Ankur ------------------------------------------------------------------------ April 7, 2005 - 19:52 : Dries Not sure. comment_save() actually calls _comment_update_node_statistics() ... and you added a call to _comment_update_node_statistics() right after a call to comment_save()? ------------------------------------------------------------------------ April 7, 2005 - 20:50 : ankur Attachment: http://drupal.org/files/issues/comment.module_10.diff (2.1 KB) I'm submitting 2 more patches here. I didn't realize that comment_save() made the call to _comment_update_node_statistics(). Well, I took a look at comment_save() and realize that it passes $edit['nid'] as the single parameter to _comment_update_node_statistics(). Now, comment_admin_edit() passes $edit to comment_save() as the 2nd param. The problem now is that there is no 'nid' key in the $edit that is passed to comment_save(). This patch avoids passing the nid as a hidden field. It works similarly to the 2nd patch I submitted above by moving the comment loading to the beginning of comment_admin_edit() and then figuring out what to do (in terms of displaying the edit-form vs. saving an edit and redirecting back to q=admin/comment). It was noted above that hidden fields in this case are to be avoided. This patch doesn't do that, but the next follow-up by me has a patch that does it. I think that's the way to go (see below). -Ankur
Issue status update for http://drupal.org/node/18215 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: ankur Updated by: Dries Status: patch Committed to HEAD. Thanks for the research! Given this is an administrator feature, adding the nid as a hidden field would be mostly harmless. Dries Previous comments: ------------------------------------------------------------------------ March 2, 2005 - 00:47 : ankur Attachment: http://drupal.org/files/issues/comment.module_5.diff (861 bytes) I don't know if this is by design, but I experimenting with forcing anonymous comments through the comments approval queue. After I logged in as someone with appropriate permissions and marked the comment "published" in the comments approval queue, the link attached to the appropriate node teaser still said "add new comment" rather than "1 comment" or "1 new comment". Here's a patch for it. I believe the diff for the patch is taken against comment.module from 4.5.2. It makes some minor additions to function comment_admin_edit(). -Ankur ------------------------------------------------------------------------ March 2, 2005 - 02:38 : ankur Attachment: http://drupal.org/files/issues/comment.module_6.diff (889 bytes) Apologies for the extra e-mail. The previous patch file was not done correctly. Here's a diff against Revision 1.302.2.9, the last 4-5-2 tagged version of the module as of this post. -Ankur ------------------------------------------------------------------------ March 14, 2005 - 00:06 : killes@www.drop.org Applies also to head. ankur: Please create patches from the Drupal root directory. ------------------------------------------------------------------------ March 15, 2005 - 23:24 : Dries I'm not a big fan of hidden fields. I'll try to think of an alternative fix. ------------------------------------------------------------------------ March 18, 2005 - 10:43 : Steven Is there any harm in always calling _comment_update_node_statistics() after editing a node? As far as I can tell, it's a pretty benign call. The only thing odd in there is: $node = node_load(array('nid' => $nid)); But I don't see $node being used anywhere in that function. Anyone have an idea why? ;) ------------------------------------------------------------------------ March 18, 2005 - 10:47 : chx please do not make node.module dependent on comment.module -- use module_invoke if you implement what Steven suggested. ------------------------------------------------------------------------ March 18, 2005 - 11:03 : Junyor@drupal.org I don't see any reason for that node_load() call either. ------------------------------------------------------------------------ March 18, 2005 - 11:17 : Steven chx: Actually I made a typo... it's about calling this after editing a comment, not a node. So there is no dependency. ------------------------------------------------------------------------ March 18, 2005 - 11:21 : Junyor@drupal.org While looking through the code, I thought this would be a very nice case where a comment API would come in handy. @Steven: I agree, we should call _update_node_comment_statistics() any time a comment is added, edited, or deleted. ------------------------------------------------------------------------ March 22, 2005 - 20:40 : Dries Not committing the patch as is. Let's see what the discussion results in. ------------------------------------------------------------------------ April 6, 2005 - 23:12 : ankur Attachment: http://drupal.org/files/issues/comment.module_7.diff (2.27 KB) Here's an alternative patch. This patch changes comment_admin_edit(): Previously, the function would check to see if edits to the comment were being $_POSTed(). If this was the case, there would be a call to comment_save() and then a drupal_goto() back to q=admin/comment. Otherwise, the comment would be loaded and an edit form would be displayed. The problem was that the comment count for the related node was not being updated and that in order to do so, you have to know the nid of the comment (the single parameter to _comment_update_node_statistics()). However, the form being posted did not contain the nid for the comment. The previous patch passed the nid (and prior publish status of the comment) through the form via hidden HTML form fields. With this patch, the loading of the comment is done first. After, loading the comment, we check to see if changes to the comment are being $_POSTed. Then we save and see if _update_comment_node_statistics() should be called. This patch is a way around having to pass hidden fields. However, the drawback (and I can't really quantify how serious of a performance penalty this would mean) is that a comment has to be loaded again when changes are submitted whereas with the previous patch, changing the comment would just mean saving the comment without having to reload it (since the nid and previous status were passed via hidden fields). In anycase, I don't know if there's another solution around this without causing some more major changes. Frankly, it would be cool if the update node could distinguish the approved comment as "1 new comment" rather than "1 comment". But then that would require some way of telling whether a comment was being approved for the first time or if the comment was edited and resubmitted to the approval queue... -Ankur ------------------------------------------------------------------------ April 7, 2005 - 21:52 : Dries Not sure. comment_save() actually calls _comment_update_node_statistics() ... and you added a call to _comment_update_node_statistics() right after a call to comment_save()? ------------------------------------------------------------------------ April 7, 2005 - 22:50 : ankur Attachment: http://drupal.org/files/issues/comment.module_10.diff (2.1 KB) I'm submitting 2 more patches here. I didn't realize that comment_save() made the call to _comment_update_node_statistics(). Well, I took a look at comment_save() and realize that it passes $edit['nid'] as the single parameter to _comment_update_node_statistics(). Now, comment_admin_edit() passes $edit to comment_save() as the 2nd param. The problem now is that there is no 'nid' key in the $edit that is passed to comment_save(). This patch avoids passing the nid as a hidden field. It works similarly to the 2nd patch I submitted above by moving the comment loading to the beginning of comment_admin_edit() and then figuring out what to do (in terms of displaying the edit-form vs. saving an edit and redirecting back to q=admin/comment). It was noted above that hidden fields in this case are to be avoided. This patch doesn't do that, but the next follow-up by me has a patch that does it. I think that's the way to go (see below). -Ankur ------------------------------------------------------------------------ April 7, 2005 - 22:55 : ankur Attachment: http://drupal.org/files/issues/comment.module.diff2 (721 bytes) This patch does what the very first patch does, only now I know that comment_save() itself calls _comment_update_node_statistics(). Again, the problem is that the $edit parameter that is passed from comment_admin_edit() to comment_save() does not contain a value for key 'nid'. This is a problem because comment_save() calls _comment_update_node_statistics() and passes $edit['nid'] as the single parameter to _comment_update_node_statistics(). There was a comment above against fixing this error by passing the 'nid' as a hidden field on the comment_admin_edit() page. However, we already pass the comment's 'cid' as a hidden field. We need to pass the 'nid' to comment save as well. My feeling is that this is the logical way as it is consistent with how the 'cid' is transmitted and saved... -Ankur
participants (6)
-
ankur -
chx
-
Dries
-
Junyor
-
killes
-
Steven