And trigger module implements hook_comment_view(), which means, joy of joy, that we issue a database query per comment just to find out whether any actions have been assigned to hook_comment_view().
Actually found this due to _trigger_get_hook_aids() in D6 when profiling a D6 site, but it's the same issue in D7.
Patches coming up.
Comment | File | Size | Author |
---|---|---|---|
#21 | trigger-960056-21-d6.patch | 1.07 KB | cedarm |
#19 | trigger-d6.patch | 1.06 KB | catch |
#14 | trigger-960056-14.patch | 2.74 KB | cedarm |
#13 | style_fix_whitespace-960056-12.patch | 582 bytes | pmitchell |
#8 | drupal-960056-8-D6.patch | 1.07 KB | mikeytown2 |
Comments
Comment #1
catchPatches.
Comment #2
catchComment #4
catchAdded drupal_static_reset(), only the test hunks are needed to get it to pass, but we technically need the others to keep things fully in sync. It's a bit sad that trigger module doesn't have any crud functions, but not for this issue. In reality the triggers fired when submitting trigger admin forms ought to be zero...
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedlooks like we should reset from trigger_assign_form_submit
Comment #6
catchJust came across this on another D6 site, bumping to 8.x to be fixed there first.
Comment #7
catchAlso this query is unindexed in both D6 and D7 - doe a filesort. It needs an index on hook, op, weight in D6, and on hook, weight in D7. It's in the top 20-odd queries for total time (frequency * time) on the D6 site I'm looking at.
Comment #8
mikeytown2 CreditAttribution: mikeytown2 commentedre-roll #1 for git
Comment #9
catchComment #10
xjm#764558: Remove Trigger module from core has been committed, so moving back to D7.
Comment #11
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedWhen we're touching it, let's remove the space there:
fetchAllAssoc(_'aid', ...
Comment #12
pmitchell CreditAttribution: pmitchell commentedClaiming the above (remove space after fetchAllAssoc(...)) as part of drupalcon core sprint
Comment #13
pmitchell CreditAttribution: pmitchell commentedFixes the above whitespace formatting issue in #11 (extra space in function call)
Comment #14
cedarm CreditAttribution: cedarm commentedPatch from #4 applies cleanly to 7.x. Added whitespace fix and re-rolled.
Comment #15
techgirlgeek CreditAttribution: techgirlgeek commentedTested by creating a trigger of displaying a message when a comment is viewed.
Added a dpm() in the query code to ensure the query was only ran once per type.
Comment #16
xjmThanks @pmitchell, @cedarm, and @techgirlgeek! This looks good to me as well. It looks like moshe's earlier review has also been addressed.
This doesn't really seem like something that needs an automated test.
Comment #17
webchickAwesome work folks.
Committed and pushed to 7.x. Thanks! Moving to 6.x for backport.
Comment #18
webchickahem.
Comment #19
catchRe-uploading the D6 patch from #1.
Comment #21
cedarm CreditAttribution: cedarm commentedJust a path issue - reroll w/ git.
Comment #22
xjmThanks for the reroll @cedarm. :)
Since D6 does not have a test suite for this, it would be good to have some manual testing to confirm the patch works properly with no adverse effects.
Comment #23
micnap CreditAttribution: micnap commentedI tested this manually - same as 15.
What I did:
I added a node and 3 comments on that node.
Created a trigger displaying a message when a comment is viewed.
Added a dpm($action) in the while block after the query in the patch
Results:
According to the query log, the query was only run once for all 3 comments instead of the once per comment without the patch.
Mickey