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.

CommentFileSizeAuthor
#21 trigger-960056-21-d6.patch1.07 KBcedarm
#19 trigger-d6.patch1.06 KBcatch
#14 trigger-960056-14.patch2.74 KBcedarm
#13 style_fix_whitespace-960056-12.patch582 bytespmitchell
#8 drupal-960056-8-D6.patch1.07 KBmikeytown2
#4 960056-trigger.patch3.19 KBcatch
#1 960056-trigger.patch1.08 KBcatch
#1 trigger-d6.patch1.06 KBcatch

Comments

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new trigger-d6.patch1.06 KB
new 960056-trigger.patch1.08 KB

Patches.

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +Needs backport to D6

Status: Needs review » Needs work

The last submitted patch, 960056-trigger.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 960056-trigger.patch3.19 KB

Added 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...

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

looks like we should reset from trigger_assign_form_submit

catch’s picture

catch
he/him
Primary language English
commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Just came across this on another D6 site, bumping to 8.x to be fixed there first.

catch’s picture

catch
he/him
Primary language English
commented

Also 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.

mikeytown2’s picture

mikeytown2 commented
StatusFileSize
new drupal-960056-8-D6.patch1.07 KB

re-roll #1 for git

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.x-dev » 7.x-dev
Issue tags: -Needs backport to D7

#764558: Remove Trigger module from core has been committed, so moving back to D7.

Niklas Fiekas’s picture

Niklas Fiekas commented
Status: Needs review » Needs work
Issue tags: +Novice 
+++ modules/trigger/trigger.module	2 Nov 2010 16:44:36 -0000
@@ -177,9 +177,13 @@ function trigger_trigger_info() {
+    ))->fetchAllAssoc( 'aid', PDO::FETCH_ASSOC);

When we're touching it, let's remove the space there: fetchAllAssoc(_'aid', ...

pmitchell’s picture

pmitchell commented
Assigned: Unassigned » pmitchell

Claiming the above (remove space after fetchAllAssoc(...)) as part of drupalcon core sprint

pmitchell’s picture

pmitchell commented
Status: Needs work » Needs review
StatusFileSize
new style_fix_whitespace-960056-12.patch582 bytes

Fixes the above whitespace formatting issue in #11 (extra space in function call)

cedarm’s picture

cedarm commented
StatusFileSize
new trigger-960056-14.patch2.74 KB

Patch from #4 applies cleanly to 7.x. Added whitespace fix and re-rolled.

techgirlgeek’s picture

techgirlgeek commented
Status: Needs review » Reviewed & tested by the community

Tested 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.

xjm’s picture

xjm
she/her
Primary language English
commented

Thanks @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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Patch (to be ported)

Awesome work folks.

Committed and pushed to 7.x. Thanks! Moving to 6.x for backport.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Version: 7.x-dev » 6.x-dev

ahem.

catch’s picture

catch
he/him
Primary language English
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new trigger-d6.patch1.06 KB

Re-uploading the D6 patch from #1.

Status: Needs review » Needs work

The last submitted patch, trigger-d6.patch, failed testing.

cedarm’s picture

cedarm commented
Status: Needs work » Needs review
StatusFileSize
new trigger-960056-21-d6.patch1.07 KB

Just a path issue - reroll w/ git.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs manual testing

Thanks 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.

micnap’s picture

micnap commented

I 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

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.