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!

CommentFileSizeAuthor
#11 comment_9.patch876 bytesSteve Dondley
#1 comment_8.patch278 bytesSteve Dondley

Comments

Steve Dondley’s picture

Steve Dondley commented
StatusFileSize
new comment_8.patch278 bytes

Oops, would help to attach the patch, eh?

TDobes’s picture

TDobes commented

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, 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 could also apply to comments. Adding these to a commentapi could really open a lot of nice possibilities for modules.

Steve Dondley’s picture

Steve Dondley commented

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:

    case 'comment form post':
      $edit = $node;
      // create subscribe checkbox form element 
      ...
      ...
      break;

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.

TDobes’s picture

TDobes commented

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.

Steve Dondley’s picture

Steve Dondley commented

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?

Steve Dondley’s picture

Steve Dondley commented

Right, let's see what others think and go from there. Anyone?

Steven’s picture

Steven commented

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.

moshe weitzman’s picture

moshe weitzman commented

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.

Steven’s picture

Steven commented

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.

Steve Dondley’s picture

Steve Dondley commented

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.

Steve Dondley’s picture

Steve Dondley commented
StatusFileSize
new comment_9.patch876 bytes

OK, here's a second revision that works with hook_comment() instead.

dries’s picture

dries commented

"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.

jhriggs’s picture

jhriggs commented

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...

TDobes’s picture

TDobes commented

There's a similar discussion going on in another issue... one of these should be marked as a duplicate.

Stefan Nagtegaal’s picture

Stefan Nagtegaal commented

@ 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..

dries’s picture

dries commented

I'd vote against a contentapi at this point: I don't think it adds real value. I'm OK with a comment API and a node API (which IMO should be called _node() not _nodeapi()).

killes@www.drop.org’s picture

killes@www.drop.org commented

Does not apply anymore.

moshe weitzman’s picture

moshe weitzman commented
Status: Active » Closed (duplicate)

see http://drupal.org/node/14708