comment_insert() and comment_update() plus their associated hooks still take arrays. Now they don't.

Patch also removes comment_invoke_comment().

CommentFileSizeAuthor
#15 php5.3youpickypickything.patch665 bytescatch
#12 comment_objects.patch15.13 KBcatch
#11 comment_objects_no_search.patch14.19 KBcatch
#11 comment_objects.patch15.13 KBcatch
#7 comment_objects_0.patch14.03 KBcatch
#3 comment_objects.patch14.97 KBcatch
comment_objects.patch13.46 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

This also blocks #504666: Make comments fieldable.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
FileSize
comment_objects.patch14.97 KB

Didn't completely remove comment_invoke_comment()

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Looks like a nice cleanup to me. Much more readable now.

Not introduced by this patch, but why do we need $comment->comment_format = $comment->format;? Also, would be nice to teach comment_save() about drupal_write_record().

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Wow, nice clean up! :)

Your indentation's off a bit here:

+  foreach ($defaults as $key => $default) {
+     if (!isset($comment->$key)) {
+       $comment->$key = $default;
+     }
+  }

I would really feel a whole lot better if we had some sort of test coverage for the comment API along with this patch. Maybe adding some to the trigger test?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
FileSize
comment_objects_0.patch14.03 KB

search_comment_$op has test coverage. Here's the same patch with the search hunks removed, and you can watch it fail, just to demonstrate there's coverage.

Not touching trigger module with a long barge pole under any circumstances.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
FileSize
comment_objects.patch15.13 KB
comment_objects_no_search.patch14.19 KB

These should apply to HEAD, fixed indentation as well.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
comment_objects.patch15.13 KB

Not sure why the other patch didn't get tested, here it is again.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

I think this is RTBC again.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Fixed » Needs review
FileSize
php5.3youpickypickything.patch665 bytes

Introduced a notice in PHP 5.3 per Berdir.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

I've run the comment tests locally on PHP 5.3 with that change and they pass now.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed to HEAD.

Status: Fixed » Closed (fixed)

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