Project: Drupal Version: cvs Component: comment.module Category: feature requests Priority: normal Assigned to: nysus Reported by: nysus Updated by: stefan nagtegaal Status: patch @ TDobes: The other issue is marked as 'duplicate', while here is the most interesting discussion going on about contentapi() vs. nodeapi()/commentapi().. Dries, what is you opinion about this? What needs to be done to get a patch like this in core? - Should we code the proposed contentapi()? - Another implementation? I really would like this in before we freeze HEAD, so i'm willing to cooperate in this.. stefan nagtegaal Previous comments: ------------------------------------------------------------------------ November 26, 2004 - 21:33 : nysus Like node forms which allow modules to use the nodeapi hook to prepend and append form elements to the node form, this simple change lets modules do the same for the comment form. Like node form, it also relies on the nodeapi hook. I plan to use it for an improvement to the subscriptions module that I have written which allows users to subscribe to individual comments without having to subscribe to an entire node. This change to the core was recommended by Moshe Weitzam found here: http://lists.drupal.org/archives/drupal-devel/2004-11/msg00256.html Thanks for the tip, Moshe! ------------------------------------------------------------------------ November 26, 2004 - 21:34 : nysus Attachment: http://drupal.org/files/issues/comment_8.patch (278 bytes) Oops, would help to attach the patch, eh? ------------------------------------------------------------------------ November 26, 2004 - 22:04 : TDobes A few things: 1. Please use the unified diff format for making patches -- see http://drupal.org/patch 2. I believe the suggestion was to add these features to hook_comment [1], not hook_nodeapi. nodeapi is just for nodes... comment is just for comments. 3. (not really related to yoru patch) Wouldn't it make more sense to have a hook_commentapi rather than a hook_comment? It seems like this would be more consistent with the naming of hook_nodeapi. 4. Many of the nodeapi $op values [2] could also apply to comments. Adding these to a commentapi could really open a lot of nice possibilities for modules. [1] http://drupaldocs.org/api/head/function/hook_comment [2] http://drupaldocs.org/api/head/function/hook_nodeapi ------------------------------------------------------------------------ November 26, 2004 - 22:36 : nysus TDobes, I don't know if you read the thread from the previous discussion, but there I pointed out that I didn't think the hook_comment seemed like the right function to modify. As far as creating a commentapi function, I thought about that. But I wondered if adding all the code necessary to support it was really worth it. You can use the pre-existing nodeapi to accomplish the same exact thing. And you won't have to create a new commentapi function in the comment.module and you won't have to create a new function in each of the modules that wants to use it. All you have to do is slap another case in the module's _nodeapi hook. So, you've got this in the comment.module's theme_comment_form() function: // Append extra comment form elements $form .= implode('', node_invoke_nodeapi($edit, 'comment form post')); And then in the subscription.module, you've got the following in the subscription_nodeapi() function: I understand your concern about maintaining naming conventions. Perhaps the nodeapi hook should be given a new name since it can have functionality beyond nodes. PS: Yeah, I know about patching with -u switch, just forgot. ------------------------------------------------------------------------ November 26, 2004 - 23:01 : TDobes I did read the thread that you linked to. At the moment (before your patch), hook_nodeapi is ONLY concerned with nodes. I believe it should stay that way. It is not that much more work for module authors to add a _comment (or _commentapi if we rename it) function to their module. On the other hand, it would be exceptionally confusing for people new to Drupal if we placed comment functions in a node hook. If we do decide to combine both hooks, I agree with you that nodeapi should be renamed. However, for the time being, I do not feel this is necessary. So... I agree with you that this would be a nice feature. However, I'd prefer a larger, more logical patch (adding to hook_comment) to a small and confusing patch (sticking this in hook_nodeapi, where it doesn't belong, IMO). But that's just my opinion... we'll have to see what others think. ------------------------------------------------------------------------ November 26, 2004 - 23:03 : nysus TDobes, OK, after rereading what you wrote and thinking about this some more, I think I may see a small advantage to having a separate hook_comment function. You are saying you could use the same op code string for both the hook_comment and nodeapi, right? But how about if the op code string were appended in some standardized way so the module _nodeapi hook could still be used? For example, you could prepend 'comment' to each of the codes for each operation that dealt with comments. So you'd have 'add' for when a node is added use 'comment add' for when a comment is added. 'insert' would be used for nodes and 'comment insert' for when comments are inserted. I guess it all boils down to the larger question I raised earler: Do we really need a separate _comment hook? ------------------------------------------------------------------------ November 26, 2004 - 23:06 : nysus Right, let's see what others think and go from there. Anyone? ------------------------------------------------------------------------ November 26, 2004 - 23:19 : Steven Hook_comment /is/ comment API. Renaming it to hook_commentapi seems like a good consistency idea. Putting all this inside hook_nodeapi is unacceptable and unclean. ------------------------------------------------------------------------ November 27, 2004 - 00:10 : moshe weitzman the best idea proposed by jhriggs was to combine nodeapi, commentapi, and user hooks into one contentapi hook. see http://lists.drupal.org/archives/drupal-devel/2004-07/msg00614.html in that msg, jhriggs prefers a single contentapi() hook over multiple $op specific hooks. Thats my slight preference as well. ------------------------------------------------------------------------ November 27, 2004 - 00:23 : Steven The contentapi puts us back in the discussion of "what is content?". Are users content? What about user profiles? etc. I don't think a generalized contentapi hook will help us though. A module will rarely affect nodes, comments and users at the same time. Almost all modules will implement contentapi and have a if ($type == 'node') or if ($type == 'comment') at the beginning. This makes the code more complicated than it should. It is also bad from a performance point of view to call every module which does something with nodes, comments or users when an operation is performed on any of them. ------------------------------------------------------------------------ November 27, 2004 - 04:51 : nysus After a little more consideration and after reading some of the comments here, I'll just recode this so that it uses the hook_comment. It just doesn't make sense to try to push through a fundamental change to Drupal's API to accomodate this small patch. ------------------------------------------------------------------------ November 27, 2004 - 05:37 : nysus Attachment: http://drupal.org/files/issues/comment_9.patch (876 bytes) OK, here's a second revision that works with hook_comment() instead. ------------------------------------------------------------------------ November 27, 2004 - 09:45 : Dries "Wouldn't it make more sense to have a hook_commentapi rather than a hook_comment? It seems like this would be more consistent with the naming of hook_nodeapi." No, but it would make sense to rename hook_nodeapi to hook_node. ------------------------------------------------------------------------ November 29, 2004 - 18:01 : jhriggs I still like the _contentapi() idea. I have discussed it several times with Kjartan, and although we have a few differences in opinion as to exactly how it should be coded, we both agree that it would be useful functionality. It does raise the "What is content?" question as Steven points out. Perhaps another name would be more appropriate. I do see tremendous benefit in having a single entry-point for this type of extensibility, though. Say for example that I want to append some type of disclaimer or a verification step (a la captcha or a similar module I wrote awhile back) in every submission form, be it node, comment, poll vote, user registration, profile, etc. I would only have to handle it in one place. Every time I receive a 'form pre' or 'form post', I can insert my content...or I can handle certain cases differently if desired (based on the type of content). Additionally, any module would be able to benefit from the logic. Instead of having somewhat overlapping _nodeapi(), _user(), and _comment() functionality like we do now (or will have even more of with the patch proposed here), we would have one invocation function that any module can use for any type of content that they define (i.e. user in user module, profile in profile.module, node in node.module, comment in comment.module, poll vote form in poll.module, etc.). Finally, as I have also discussed with Kjartan, functionality like this would mean that project.module would not have to use its own comment system (lots of duplicated code). It would be able to use the enhanced form-insertion and verification available through the hook. If there is sufficient interest, I will revive/rework the patch that I have started several times... ------------------------------------------------------------------------ December 24, 2004 - 07:48 : TDobes There's a similar discussion going on in another issue [3]... one of these should be marked as a duplicate. [3] http://drupal.org/node/14708 -- View: http://drupal.org/node/13539 Edit: http://drupal.org/project/comments/add/13539