New hooks for comment handling

[urs.enke]

Hi, when trying to process comments in a plugin, I found that the existing hook frontend_saveComment is positioned before the actual database entry is made, making it lack information on the inserted comment's ID. Simply guessing that it is the latest comment's ID plus one could be wrong in case another comment comes in first. What I could not find at all are hooks for the approval or deletion of comments. Obviously, a hook is just a line that I could add myself, but it makes upgrades tedious... What about some hook inflation?

Four other things on my mind:
- The text "A draft of this entry has been saved" is a little bit misleading as the entry is never duplicated. What about "This entry has been saved and flagged as a draft"?
- In functions_entries.inc.php, a distinction between backend_publish and backend_save is made, stating the difference as "Send publish tags if either a new article has been inserted from scratch, or if the entry was previously stored as draft and is now published". What follows, however, is , put shortly (!isDraft and (isNew or isDraft)). Considering that the "or isDraft" is logically superfluous, the distinction effectively comes down to "new and published" vs. "old or draft". The respective hooks don't necessarily make sense. There are really four cases, each of which could get its hook. Currently, many plugins seem to handle this by considering both hooks at the same time and making distinctions by themselves.
- What's the "author" column in "entries" for? There's already an "authorid"...
- It is normal that when having submitted a comment, the form to enter another is not displayed on the immediately resulting page and thus the "Reply" links are ineffective? Or have I broken something?

[garvinhicking]

Hi!

Of course we can add hooks into the core. However since every hook costs performance (which is especially important because of comment spam and that increasing performance), can you please describe what usage scenario you have with those hooks? What do you need them for?

- The text "A draft of this entry has been saved" is a little bit misleading as the entry is never duplicated. What about "This entry has been saved and flagged as a draft"?

Hm. I personally prefer the first message, because this message is known to s9y users, and it's not really that misleading? Other opinions?

- In functions_entries.inc.php, a distinction between backend_publish and backend_save is made, stating the difference as "Send publish tags if either a new article has been inserted from scratch, or if the entry was previously stored as draft and is now published". What follows, however, is , put shortly (!isDraft and (isNew or isDraft)). Considering that the "or isDraft" is logically superfluous, the distinction effectively comes down to "new and published" vs. "old or draft". The respective hooks don't necessarily make sense. There are really four cases, each of which could get its hook. Currently, many plugins seem to handle this by considering both hooks at the same time and making distinctions by themselves.

Why do you need a diversion? Within the Hook 'new and publish' or 'old or draft' you can check if the entry is a draft/old - I don't see an advantage of having four hooks? This also would make us need to update all plugins to use those hooks. Not very BC-friendly.

- What's the "author" column in "entries" for? There's already an "authorid"...

It's still in there for backwards compatibility of old plugins. BC is very important to Serendipity, so that's why it was not removed.

- It is normal that when having submitted a comment, the form to enter another is not displayed on the immediately resulting page and thus the "Reply" links are ineffective? Or have I broken something?

Yes, the form is not displayed again to prevent "spamming" or "trolling"

Best regards,
Garvin

[urs.enke]

Hi, thanks for faithfully replying to everbody! I am trying to move several blogs' articles into another blog 'in real-time'. Aggregating feeds is too inefficient, as is using a frequent cron job to duplicate things, so I am writing a plugin that mirrors any database changes (article creation, change, deletion). To do this with comments, I need to add hooks

• - after having submitted a comment
  - when approving a comment
  - when changing a comment
  - when deleting a comment

(Good for me that there's no disapproval. )

As for my elaboration over backend_publish and backend_save, the logical expression is redundant and its description is wrong. You may, of course, ignore the latent criticism of their nature, as there are plugins that use these existing hooks separately and, as I mentioned, it is possible to do any distinction on one's own.

If the "(Reply)" links are inneffective by design immediately after having submitted a comment, they should perhaps not be printed at all.

[garvinhicking]

Hi!

Have you thought about using MySQL replication setup instead? That would be the most efficient, transparent solution?

I do see the need of changing/adding hooks in that case, but I believe that a different replication system would be much easier and wouldn't involve your need to write a plugin for that.

I'll need to check the code thorougly on what you mean about the logical expression on the backend_publish/save hooks, but I thought those should really only be executed on publishing an entry, or saving an entry if no publishing is done. So it should only go the 'backend_publish' route in case an article is newly published (or re-published after setting it to 'Draft' again), and 'backend_save' should be executed in all other cases?

If the "(Reply)" links are inneffective by design immediately after having submitted a comment, they should perhaps not be printed at all.

That's true However the problem of this is that it would involve patching each template's comments.tpl file for that, because it's not a thing that can at this point be changed in the core.

Regards,
Garvin

[urs.enke]

Have you thought about using MySQL replication setup instead?

As it's about merging several blogs, IDs have to be changed, so a mere replication wouldn't suffice.

So it should only go the 'backend_publish' route in case an article is newly published (or re-published after setting it to 'Draft' again), and 'backend_save' should be executed in all other cases?

Besides new/old, this would be one sensible distinction, assuming that the "publish" case includes the update of an already published article. The other case, though, should be called "draft", as the article is "saved" in all cases.

Again, one hook would have sufficed at this place, leaving the other to cover one of the four comment events without additional overhead. But yes, redefining or even renaming them now might not be worth the fuss.

Edit: A little bug report for your personal notes: entries' comment counts are not updated on the deletion of comments.

[garvinhicking]

Hi!

About your miogration thing: If you merge blogs, the IDs of comments shouldn't matter at all, so you could just insert the entries using the auto-increment assigned key using the existing savecomment hook?

As for approving and deleting, I've just committed two new hooks to the SVN:

http://svn.berlios.de/viewcvs/serendipi ... ts.inc.php

Besides new/old, this would be one sensible distinction, assuming that the "publish" case includes the update of an already published article. The other case, though, should be called "draft", as the article is "saved" in all cases.

But when an published entry is edited, also backend_save is executed (not backend_publish), so naming it 'backend_draft' would be more misleading?

Renaming hooks isn't really possible, becaues it would break existing plugins.

Again, one hook would have sufficed at this place, leaving the other to cover one of the four comment events without additional overhead. But yes, redefining or even renaming them now might not be worth the fuss.

One's always cleverer in the end From today's view I would also made it differently. At the stage where it was introduced, the disticiton was much more clear and was only later fleshed out to carry out more tasks (thus not requiring to add new hooks)

Edit: A little bug report for your personal notes: entries' comment counts are not updated on the deletion of comments.

Are you sure? The code contains this:

Code: Select all

serendipity_db_query("UPDATE {$serendipity['dbPrefix']}entries SET $type = $type-1 WHERE id = ". $entry_id ." AND $type > 0 $admin");

and that works at least here on my setup?

Regards,
Garvin

[urs.enke]

If you merge blogs, the IDs of comments shouldn't matter at all, so you could just insert the entries using the auto-increment assigned key using the existing savecomment hook?

They matter as soon as there are comments on comments: when I know that a new comment is a reply to number 10 in the source blog, I need to know as which number this parent comment has once been added in the aggregate blog. For that I need a translation table, for that the original comments' mysql_insert_ids and for that a hook after insertion took place.

As for approving and deleting, I've just committed two new hooks to the SVN

Great, so at least those won't have to be re-inserted in each subsequent upgrade of Serendipity. Thanks! Only the capitalization is not really consistent with the save hook, but then again hook nomenclature seems uncurably inconsistent, anyway, citing BC.

Apart from that second save hook, wouldn't it only be consequent to also have a hook for updating a comment, in comments.inc.php?

when an published entry is edited, also backend_save is executed (not backend_publish), so naming it 'backend_draft' would be more misleading?

I thought we were talking about how it could be, that's why I expressly mentioned the condition that backend_publish would have to include any action that results in a published article. Anyway, as changing the hooks would be tedious/dangerous, this is all very theoretical. What remains are the erreneous comment and the redundant boolean expression.

I'll have to see why the line you mentioned did not decrease my comment count. Let me have another try at an alleged bug: If a comment is deleted, but has children, merely its content is replaced. So far, so good. But a second attempt at deletion then works, the comment in the code even stating that "it can be safely removed". It could still have children, though, nothing has changed versus the initial case...

Edit: I just see that the parent ID is propagated, so apparently the behaviour is indeed intended. Strange...

[garvinhicking]

Hi!

They matter as soon as there are comments on comments: when I know that a new comment is a reply to number 10 in the source blog, I need to know as which number this parent comment has once been added in the aggregate blog. For that I need a translation table, for that the original comments' mysql_insert_ids and for that a hook after insertion took place.

Okay. You got me.

How about this: Since the 'approveComment' feature now has an event hook, can you use that one to migrate your comment? I figure you only need approved comments in the mirrored blogs, so at that place you'd have the entry/comment ID available, and we wouldn't need to add a new hook to after the frontend_saveComment one.

Great, so at least those won't have to be re-inserted in each subsequent upgrade of Serendipity. Thanks! Only the capitalization is not really consistent with the save hook, but then again hook nomenclature seems uncurably inconsistent, anyway, citing BC.

Yeah, you got me there. Unique nomenclature for the hooks is my fundamental flaw.

Apart from that second save hook, wouldn't it only be consequent to also have a hook for updating a comment, in comments.inc.php?

Indeed, I also added a hook there.

I thought we were talking about how it could be, that's why I expressly mentioned the condition that backend_publish would have to include any action that results in a published article. Anyway, as changing the hooks would be tedious/dangerous, this is all very theoretical. What remains are the erreneous comment and the redundant boolean expression.

Ah, okay. I was talking about how it stands - because changing where backend_publish is called with which data is not an option, unless the current behaviour would exhibit bugs with existing plugins. Sorry for not getting you completely there.

I'll have to see why the line you mentioned did not decrease my comment count.

Sure, I'd be interested in that of course!

Let me have another try at an alleged bug: If a comment is deleted, but has children, merely its content is replaced. So far, so good. But a second attempt at deletion then works, the comment in the code even stating that "it can be safely removed". It could still have children, though, nothing has changed versus the initial case...

But if somebody deletes that comment again, I'm sure he has reasons why he really finally wants to delete the comment. We should give him that opportunity, no?

Best regards,
Garvin

[urs.enke]

Hello again!

Since the 'approveComment' feature now has an event hook, can you use that one to migrate your comment? I figure you only need approved comments in the mirrored blogs, so at that place you'd have the entry/comment ID available, and we wouldn't need to add a new hook to after the frontend_saveComment one.

Hmm, this should work. Good idea!

wouldn't it only be consequent to also have a hook for updating a comment

Indeed, I also added a hook there.

Thanks again, my wishlist for Christmas is shrinking fast!

I'll have to see why the line you mentioned did not decrease my comment count.

Sure, I'd be interested in that of course!

In my case, the query decrementing the comments counter does not work because of an apostrophe at its end, which is added in the following line in functions_comments.inc.php:

Code: Select all

$admin = " AND authorid = " . (int)$_SESSION['serendipityAuthorid'] ."'";

I don't understand how this can work in your case, as even the latest SVN contains this line.

[garvinhicking]

Hi!

Code: Select all

$admin = " AND authorid = " . (int)$_SESSION['serendipityAuthorid'] ."'";

Ah! Good catch. I had the permission, so my $admin was always empty (because I had the permission maintainEntriesOther).

Regards,
Garvin