Multiple load comments
catch - June 26, 2009 - 00:47
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | task |
| Priority: | normal |
| Assigned: | catch |
| Status: | closed |
Description
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| comment_load_multiple.patch | 6.01 KB | Idle | Failed: 11788 passes, 1 fail, 0 exceptions | View details |
#1
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.
#2
+ $comment = current(comment_load_multiple(array('cid' => $cid, 'status' => COMMENT_PUBLISHED)));What about that comment_load() stub function?
#3
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.
#4
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.
#5
The last submitted patch failed testing.
#6
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().
#7
The last submitted patch failed testing.
#8
bot failed, retest
#9
The last submitted patch failed testing.
#10
#11
retest again
#12
The last submitted patch failed testing.
#13
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.
#14
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.
#15
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
#16
looks strong to me.
making comments fieldable sure sounds like lipstick on a pig to me. comment's code is so tangled.
#17
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.
#18
The last submitted patch failed testing.
#19
Re-rolled.
#20
Back to RTBC.
#21
This patch is consistent with other _load_multiple() patches, and coding style looks good. Committed to HEAD.
Needs upgrade docs.
#22
Thanks!
Added to upgrade docs.
#24
Automatically closed -- issue fixed for 2 weeks with no activity.