similar to http://drupal.org/node/314244 etc...

the tests running not perfect

Undefined property: stdClass::$signature

no documentation changed etc.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

this code cannot work :(

wonders why not test fails

next version, not expections here

Anonymous’s picture

Title: remove op from hook_comment(). » remove $op from hook_comment().
Status: Active » Needs work

nice work, all tests pass.

+function search_comment_update($a1) {

is it possible to use a better name for $a1?

/**
 * Implementation of hook_comment().
 */
function trigger_comment($a1, $op) {

the doc block needs updating. trigger_comment is now a helper function. might even be worth making it _trigger_comment.

dawehner’s picture

Status: Needs work » Needs review

thx for the review

i named all arguments for validate update insert to form_values and all other $ops to $comment

i renamed the trigger_comment to _trigger_comment and added some comments and removed the Impl. of hook_comment()

<?php
+/**
+ * Helper function for implementations of hook_comment_op().
+ * @param $a1
+ *  Argument, which will be passed to the action.
+ * @param $op
+ *  What kind of action is being performed.
+ */
+function _trigger_comment($a1, $op) {
?>
dawehner’s picture

sry for missing the patch

Anonymous’s picture

Status: Needs review » Needs work

ok, nearly there.

+  search_touch_node(is_array($a1) ? $form_values['nid'] : $form_values->nid);

we know the passed in $form_values is an array, we don't need the is_array checks.

+/**
+ * Helper function for implementations of hook_comment_op().
+ * @param $a1
+ *  Argument, which will be passed to the action.

nitpicking, but we need a blank line in the docblock between "Helper function ..." and the @param part. also, two spaces before "Argument, which will be passed to the action.". more importantly, $a1 can only be one of two things, so best to describe that - form values or comment object.

otherwise, looks good to go.

dawehner’s picture

thx for review again!

i changed also the the text

Reindex the node when comments are added or changed.<code> to a more specific comment for example: <code>Reindex the node when comments are deleted.

working at comment.api.php right at the moment

dawehner’s picture

- rewrite the documentation on comment.api.php
- added some more examples
and the stuff at #6

dawehner’s picture

Status: Needs work » Needs review
Anonymous’s picture

Status: Needs review » Needs work

i get an exception when running search tests:

Trying to get property of non-object    Notice    search.module     690    search_comment_publish()

seems the doco here http://api.drupal.org/api/function/hook_comment/7 is wrong: $op publish passes an array, not the comment object.

otherwise, all looks good.

Anonymous’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.65 KB

ok, seems lazy not to fix that. patch attached, all tests pass.

i think this is RTBC.

dawehner’s picture

thx

i also think this is RTBC

Dries’s picture

Committed to CVS HEAD. Thanks!

Dries’s picture

Status: Reviewed & tested by the community » Fixed
sun’s picture

Status: Fixed » Needs work

All tests pass? Seems we need some more tests then. HEAD is broken.

sun’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
1.15 KB

Fixed for me.

dawehner’s picture

Priority: Critical » Normal
Status: Needs review » Fixed
FileSize
1.15 KB

mh

:(

here is a patch

dawehner’s picture

Component: comment.module » trigger.module
Status: Fixed » Needs review
Anonymous’s picture

Category: feature » bug
Priority: Normal » Critical
Status: Needs review » Needs work

ouch! we're definitely in need of more tests.

@dereine: are you ok to try to write some tests that fail for the broken code?

dawehner’s picture

it would be possible to write the nearly the same tests which are in trigger.test for comment actions

i do some at the moment

wonders whether it is possible to create a commit hook in cvs which checks whether each function exists and stuff like this

sun’s picture

Status: Needs work » Reviewed & tested by the community

First of all, HEAD should be fixed, as this typo prevents tests for #8: Let users cancel their accounts from passing.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Committed to un-break broken HEAD. Leaving CNW so we can get some tests.

sun’s picture

Issue tags: +Needs tests

.

andypost’s picture

Status: Needs work » Postponed
Issue tags: +triggers
marcvangend’s picture

Status: Postponed » Needs work
catch’s picture

Title: remove $op from hook_comment(). » Trigger tests for remove $op from hook_comment().
Priority: Critical » Normal
Tor Arne Thune’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » task
Issue tags: +Needs backport to D7
catch’s picture

Status: Needs work » Fixed
Issue tags: -Needs tests

Marking fixed, since there is a more up-to-date issue that deals with the actions breakage in comment module, and it needs more than tests sadly. #244093: Node and comment actions are (still) completely broken and have broken tests too.

Status: Fixed » Closed (fixed)
Issue tags: -triggers, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.