Patch needs docs for hook_comment_load() and some cleanup - we should should remove hook_comment_view() entirely most likely. This patch is a prerequisite to fieldable comments.

One fail locally which I can't reproduce manually.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Needs work » Needs review
FileSize
6.29 KB

Fixed a join coming after an addField() pointed out by sun in irc. Setting to CNR so the test bot can knock back for that weird test failure - the issue is that when deleting a tree of comments, simpletest can still see the replies - however I can't reproduce this at all manually.

sun’s picture

+      $comment = current(comment_load_multiple(array('cid' => $cid, 'status' => COMMENT_PUBLISHED)));

What about that comment_load() stub function?

catch’s picture

That'd mean adding either a $status parameter to comment_load(), or always setting status within the function. Not sure about that inconsistency vs. node/user/file/taxonomy term/vocabulary loading.

catch’s picture

FileSize
8.77 KB

Here's a version with additional assertion in the test showing that the cid for the deleted comment which is displaying is no longer in the database. Very odd.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Assigned: Unassigned » catch
Status: Needs work » Needs review
FileSize
7.28 KB

Fixed the test - needed to make sure either $cids or $conditions were populated in the query. Also added docs for the new hook_comment_load().

Status: Needs review » Needs work

The last submitted patch failed testing.

andypost’s picture

Status: Needs work » Needs review

bot failed, retest

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
andypost’s picture

retest again

Status: Needs review » Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

Status: Needs work » Needs review

foreach (module_implements('comment_load') as $module) {

I think a module_invoke_all() would be more effective here. The implementations can still alter $comments by reference. module_invoke_all() does a drupal_function_exists() which this code lacks.

Otherwise, looks very nice.

catch’s picture

FileSize
7.18 KB

Re-rolled with module_invoke_all() and a slight cleanup. I'd like to see this go in fairly quickly so work on #504666: Make comments fieldable can begin.

catch’s picture

Having trouble both with the D6/7 upgrade path and devel generate which makes it hard to benchmark, so this is on one node and one comment:

ab -c1 -n500 node/72

HEAD:
Requests per second:    9.46 [#/sec] (mean)
Time per request:       105.719 [ms] (mean)
Time per request:       105.719 [ms] (mean, across all concurrent requests)
Transfer rate:          79.96 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    94  106  11.0    101     153
Waiting:       74   97  10.9     94     148
Total:         95  106  11.0    101     153

Patch:

Requests per second:    9.39 [#/sec] (mean)
Time per request:       106.475 [ms] (mean)
Time per request:       106.475 [ms] (mean, across all concurrent requests)
Transfer rate:          79.57 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    96  106  11.9    100     159
Waiting:       73   99  11.7     94     146
Total:         96  106  11.9    101     159
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

looks strong to me.

making comments fieldable sure sounds like lipstick on a pig to me. comment's code is so tangled.

catch’s picture

Well it is and it isn't. Having to either run an extra module just to get file uploads on comments (like Drupal.org does) adds a lot of cruft, making comments fieldable should be a very small amount of code hopefully, and means we'd be able to use a core filefield module for the issue queue on Drupal.org for example. See #504678: Use objects in the comments API for the other pre-requisite cleanup patch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
6.68 KB

Re-rolled.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

This patch is consistent with other _load_multiple() patches, and coding style looks good. Committed to HEAD.

Needs upgrade docs.

catch’s picture

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

Thanks!

Added to upgrade docs.

Status: Fixed » Closed (fixed)

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