Support forums : You can sense the bug

trigger_event_first_true()

Not like "it smells funny when I click that". This is for bugs that don't show an error but something just doesn't seem right or ends up doing something you think is wrong. It doesn't show you an error but you know something ain't right.

Moderator: Dracones

trigger_event_first_true()

Postby EdB » Tue Aug 24, 2010 6:29 pm

This is kinda odd. I wanted to take advantage of AppendHitLog so I had to learn how trigger_event_first_true() did it's thing. It kills all other plugins after the first true, based apparently on plugin priority, meaning if 2 plugins use a hook that responds per trigger_event_first_true() then one of them is going to be broken.

I tested this by writing a tiny little plugin that only uses that hook and only echo "hello world". Sure enough it echoed. I then hacked basic_antispam and took out the "if it is not a referer then return false" and my plugin failed to echo. I then set my plugin's priority to lower than basic_antispam's and I got my echo back.

I dunno what the solution is because I'm not so sure I understand the need for trigger_event_first_true(). I'm hip to how a plugin might need to change should_be_logged() to false but that's only one situation that uses the first trigger thing.

And actually for what I want to do - a stats summary emailer - I'd be much better off with a hook inside dbprune() before it actually does any pruning but I can't think of any other reason to add a hook there. And a uni-plugin hook is a bad. Unless an obvious general case can be made. Which I can't. For a stats mailer all I want to do is once per day on the first hit of the new day wrap up the data on a daily basis and throw it in it's own table. Top 10 referers, search terms, pages hit - stuff like that. Based on pruning I could say "before you prune let me rip through the data and store summaries per day, then have at it".

But anyway there's something weird about trigger_event_first_true() and how it knocks other plugins out of doing their thing.

Edit oh and I had to tell basic antispam to check referers or something like that. had to check a box that is off by default in order to make it use appendhitlog
EdB
Dracone
User avatar
 
Posts: 2072
Joined: Sun Nov 22, 2009 7:20 am
Location: Maricopa Arizona

Re: trigger_event_first_true()

Postby Tblue » Tue Aug 24, 2010 7:28 pm

Well, Plugins::trigger_event_first_true() looks for the first plugin whose method you want to call returns true. It doesn't check the other plugins after one plugin returned true, that's by design. The lower the priority of the plugin, the earlier it gets called.
Tblue
Dracone
 
Posts: 340
Joined: Sat Nov 21, 2009 1:35 pm
Location: Berlin, Germany

Re: trigger_event_first_true()

Postby EdB » Tue Aug 24, 2010 7:45 pm

Understood, but why? Cuz see by design it means a plugin will fail to do it's job if it also using a hook called that way. And has higher priority number. So 2 plugins that are properly coded, one will always fail.

I can see not logging a hit twice, but I can't see not bothering to check all remaining plugins just because *a* plugin already used a hook.
EdB
Dracone
User avatar
 
Posts: 2072
Joined: Sun Nov 22, 2009 7:20 am
Location: Maricopa Arizona

Re: trigger_event_first_true()

Postby Yabs » Wed Aug 25, 2010 8:29 am

There's already a hook inside dbprune(), unfortunately it's called after the hitlog has been pruned but before sessions are pruned. It'd be easy enough to move it up a tad and rename it to reflect that.

¥
I may have opened the door but you entered of your own free will

Image Techno Babble II
Image Tacky Pad 3
Yabs
Dracone
User avatar
 
Posts: 896
Joined: Sat Nov 21, 2009 9:59 am

Re: trigger_event_first_true()

Postby EdB » Wed Aug 25, 2010 8:57 am

I hadn't noticed it but only took a cursory glance.

On this issue, some cases of "do it once" sorta make sense (trackback address, doctoring a message) but still leave open the idea that only one plugin gets to use certain hooks. In this specific case since there is the this->already_been_logged() thing I don't see why we would restrict to only one plugin. In other words basic antispam would set the been_logged() appropriately and let another plugin use the same hook.

But the generic "problem" of somebody getting bumped still seems wrong even if there is good reason to only let one get through. Like how many times would you need to display the trackback address? I briefly started thinking of something along the lines of an array of responses that all gotta be true to show trackback addy but then saw something shiny and moved on.

(setting the plugin priority to 1 was the shiny thing ;) )
EdB
Dracone
User avatar
 
Posts: 2072
Joined: Sun Nov 22, 2009 7:20 am
Location: Maricopa Arizona

Re: trigger_event_first_true()

Postby Tblue » Wed Aug 25, 2010 5:35 pm

Ah, you think the idea of having Plugins::trigger_event_first_true() is weird. Well, I guess in some cases it makes sense (e. g. where a plugin is allowed to cancel some action and we do not care whether some other plugin thinks different), but there are other cases where we probably should only do something when all plugins give the same answer (or e. g. if all plugins that do not return NULL return false or something like that...).

Not sure whether it is okay that one single plugin can prevent logging... If it is, then using Plugins::trigger_event_first_true() makes sense, I think.
Tblue
Dracone
 
Posts: 340
Joined: Sat Nov 21, 2009 1:35 pm
Location: Berlin, Germany

Re: trigger_event_first_true()

Postby EdB » Wed Aug 25, 2010 5:54 pm

That's the deal: is the very idea of "me first and me only" a good one or not, and more accurately is it a good one in each location we use it. Some are better than others but I don't have the files open that show which hooks use it.

To me the worst case was the AppendHitLog hook. Very bad call given that there is already an alternative way to see if we still need to log the hit or not. (note to self: make sure my plugin disables itself if both boxes for 'log this type of hit' are turned off.)

The one that shows the trackback addy is where I thought of an array of returns. If ALL conditions desired by the install owner are satisfied then show it.

The one that actually does something is a totally different animal. I think it was our new hook for SendMessage that allows a plugin to completely take over sending the email. Although maybe "this->message_sent()" would be in order?

Alternatively something on installation that says to the plugin installer "Your new plugin uses hook "Foo" which is in use by the plugin "Bar", but that hook only responds to one plugin. So there's a conflict and it ain't our problem that you got plugins fighting it out over a hook." At a minimum if we can't find a way to do it with an array of returns or let all plugins use the hook we can tell folks about the conflict.

Hey another alt to the AppendHitLog one in my head right now is a new hook for "WorkHitLogNoLogging" right before the current hook. That way the current "only one gets it" works without issue and my thing wouldn't even bother with AppendHitLog because I don't want to append it. I want to work with it. Get a day's worth of hits and distill it down to a summary of cool stuff to put in an email, store that summary, then email it out on a specified frequency.

ANYWAY it's an issue that should be recognized such that resolution in general and specific can be found. No sense really in having one plugin knock out others and not, at a minimum, let folks know ya know?
EdB
Dracone
User avatar
 
Posts: 2072
Joined: Sun Nov 22, 2009 7:20 am
Location: Maricopa Arizona


Return to You can sense the bug

Who is online

Users browsing this forum: No registered users and 1 guest

cron