Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#19 | comment_load_multiple.patch | 6.68 KB | catch |
#14 | comment_load_multiple.patch | 7.18 KB | catch |
#6 | comment_load_multiple.patch | 7.28 KB | catch |
#4 | comment_load_multiple.patch | 8.77 KB | catch |
#1 | comment_load_multiple.patch | 6.29 KB | catch |
Comments
Comment #1
catchFixed 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.
Comment #2
sunWhat about that comment_load() stub function?
Comment #3
catchThat'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.
Comment #4
catchHere'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.
Comment #6
catchFixed the test - needed to make sure either $cids or $conditions were populated in the query. Also added docs for the new hook_comment_load().
Comment #8
andypostbot failed, retest
Comment #10
catchComment #11
andypostretest again
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedforeach (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.
Comment #14
catchRe-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.
Comment #15
catchHaving 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
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedlooks strong to me.
making comments fieldable sure sounds like lipstick on a pig to me. comment's code is so tangled.
Comment #17
catchWell 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.
Comment #19
catchRe-rolled.
Comment #20
catchBack to RTBC.
Comment #21
webchickThis patch is consistent with other _load_multiple() patches, and coding style looks good. Committed to HEAD.
Needs upgrade docs.
Comment #22
catchThanks!
Added to upgrade docs.