Hooks events irc log

gistfile1.txt

(20:45:10) nn-: btw, did you read my posts in hooks rfc?
(20:46:02) naderman: yeah
(20:46:21) naderman: didn't see anything I would need to reply to?
(20:46:26) naderman: or did I forget something?
(20:46:42) naderman: there's the stuff we need to add to extension finder
(20:46:45) naderman: did you want me to do that?
(20:47:05) naderman: hmm looking at it again
(20:47:09) naderman: there were a bunch of questions
(20:47:18) naderman: I guess I just didn't have any immediate answers and moved on
(20:47:19) naderman: sorry :P
(20:47:47) ***nn- applies slappage to naderman
(20:48:05) nn-: and then you blasted my wip pull reqeust
(20:48:06) naderman: ckwalsh: hey
(20:48:07) A_Jelly_Donut: apple slappage? <3
(20:48:14) naderman: nn-: yeah but I already said sorry for that
(20:48:26) naderman: nn-: I was just confused by there being a run_hooks function in the WIP PR
(20:48:31) naderman: I expected it to be template stuff only
(20:48:37) naderman: but understood later ;-)
(20:48:49) naderman: A_Jelly_Donut: ?
(20:49:11) nn-: i'm not sure it makes sense to merge just the template stuff, but if you/anyone thinks this makes sense i will probably make it happen
(20:49:23) A_Jelly_Donut: meh, misread what Oleg wrote
(20:49:29) naderman: nn-: yeah I'd keep those two separate
(20:49:45) naderman: well unless you expect them to have any functionality in common?
(20:49:52) naderman: hmm I guess you're right
(20:49:53) nn-: that's tbd
(20:49:57) naderman: nevermind, keep them in one
(20:49:58) naderman: yeah
(20:49:59) nn-: not sure where the hooks will go actually
(20:50:06) nn-: global function seems like not the best of ideas
(20:50:10) naderman: nope
(20:50:24) nn-: some kind of a hook manager?
(20:50:35) naderman: I would do /ext/foo/hook/bar_hook.php
(20:51:03) naderman: which defines some bar hook class with hook methods
(20:51:13) naderman: allows you to ship multiple of those classes with one extensions if necessary
(20:51:17) nn-: i'm talking about the code that executes hooks
(20:51:21) naderman: ohhh
(20:51:22) naderman: sorry
(20:51:27) nn-: do we have a term for that?
(20:51:33) nn-: you're talking about ledges
(20:51:36) nn-: actually no
(20:51:40) nn-: you're talking about hooks
(20:51:42) nn-: ledges are locations
(20:51:55) naderman: ledge -> (hook handler?) -> hook
(20:52:11) naderman: or manager or something
(20:52:54) naderman: actually thinking about this more, I'm not sure ledge makes that much sense
(20:53:13) naderman: ah yes it does
(20:53:20) naderman: well event makes more sense
(20:53:27) naderman: I mean this is event handling afterall
(20:53:51) naderman: $hook_manager->trigger(new PostEvent($post_data, ...));
(20:54:06) naderman: I would really prefer if we had some kind of event object
(20:54:40) naderman: so you can use it both for return values (even multiple values) and to control flow
(20:54:50) naderman: like jquery events
(20:54:51) nn-: hmm
(20:55:12) nn-: obviously we have no exception handling (ha!)
(20:55:14) naderman: like $event->stopPropagation()
(20:55:34) naderman: nn-: how do you mean?
(20:55:38) nn-: my poc v1 used references for parameters to support modification
(20:55:43) naderman: yeah I know
(20:55:49) nn-: i'm currently thinking for hooks to accept and return a hash
(20:55:53) naderman: but I'd rather stick them all into a specialised event object
(20:56:07) naderman: I'd really rather do this the OOP way
(20:56:27) naderman: certainly improves documentation a lot
(20:56:34) naderman: it's very clear what data a particular event provides
(20:56:37) naderman: and which data you can set
(20:56:42) naderman: with an array that's rather magic
(20:58:43) naderman: nn-: what do you think about the event way?
(20:58:59) nn-: the whole idea of hooks poc and hooks v2 is to not over-engineer them
(20:59:23) nn-: so i guess i might be moving in that direction but so far i have not seen a need to abort hook chain
(20:59:42) nn-: right now i have no use case for events
(20:59:47) naderman: how do you handle return values then?
(20:59:54) naderman: well my main point for events is
(20:59:56) nn-: hook accepts and returns an array
(21:00:00) nn-: possibly modifying it
(21:00:10) naderman: yes all I'm asking is that that array be an object
(21:00:16) naderman: so it's not arbitrary what keys that array contains
(21:00:21) naderman: but well defined somewhere
(21:00:32) nn-: right now keys are up to the ledge
(21:00:51) naderman: yes, so without looking at the code of the ledge you can't really tell what you're getting
(21:01:00) naderman: with an object you can look at the api docs of the event
(21:01:00) nn-: precisely
(21:01:03) naderman: and it'll document it
(21:01:15) nn-: problem with that we don't have objects anywhere
(21:01:20) naderman: hmm?
(21:01:26) nn-: sql is arrays
(21:01:31) nn-: db fetches are arrays
(21:01:33) naderman: I don't think you understand
(21:01:37) naderman: yes sure
(21:01:45) naderman: you just stick that array or whatever into the event
(21:01:55) nn-: i understand
(21:02:04) naderman: e.g. $event->getRetrievePostSql();
(21:02:08) naderman: or some such
(21:02:12) nn-: and i'm not saying it's bad, but i am saying right now i am not seeing modifications that need this
(21:02:15) naderman: typically events will have more than one value
(21:02:27) naderman: this is not about modifications _needing_ this
(21:02:34) naderman: it's about making modifications easier to write?
(21:02:51) naderman: because you don't rely on some undefined random array
(21:03:02) naderman: that might change from version to version without being properly documented in the api reference
(21:03:17) nn-: consider a modify_sql_for_XXX hook
(21:03:24) nn-: what do you suppose it might receive?
(21:03:37) nn-: do you get anything by stuffing that into a wrapper object?
(21:03:47) naderman: let's take an example from the hook location request forum
(21:03:59) naderman: cause I think most suggestions there needed multiple values
(21:04:27) naderman: like
(21:04:27) naderman: http://area51.phpbb.com/phpBB/viewtopic.php?f=111&t=33714
(21:04:38) naderman: Input arguments: $page_title, $display_online_list, $item_id, $item
(21:04:44) naderman: would certainly help if that was
(21:04:50) naderman: $event->get_page_title()
(21:04:55) naderman: $event->get_item_id()
(21:04:56) naderman: ...
(21:04:58) naderman: rather than
(21:05:02) naderman: $event['page_title']
(21:05:07) naderman: simply because the latter has no docs
(21:05:09) naderman: the former does
(21:06:03) naderman: nn-: you understand what I mean?
(21:06:26) nn-: ok
(21:06:47) naderman: and then you can also restrict set_...() to values that are actually going to be used later on
(21:06:57) nn-: but
(21:06:58) naderman: so if you want to allow modification of the page_title
(21:07:03) nn-: now you'll need to define objects for each ledge
(21:07:04) naderman: the event can have a set_page_title() method
(21:07:07) naderman: yes
(21:07:10) naderman: it's certainly more code
(21:07:16) naderman: but it's mostly for documentation purposes
(21:07:21) naderman: we would have to write all of that down anyway
(21:07:34) nn-: this is going to be pretty slow i fear
(21:07:45) naderman: you mean execution wise or development wise?
(21:07:51) nn-: not that we tend to care about performance and especially trade it for features
(21:07:55) nn-: execution
(21:07:56) naderman: I wouldn't worry about execution time of this at all
(21:08:03) naderman: creating some object really doesn't cost any time
(21:08:19) naderman: I mean whether you create an object or an array makes a tiny difference at best
(21:08:29) naderman: only thing we could worry about is the extra memory for loading those classes
(21:08:33) naderman: but that won't be hundreds
(21:08:41) naderman: so even that will be little performance difference
(21:09:23) naderman: we could even write the whole thing so that there is a check for the existance of any hooks for that ledge
(21:09:29) naderman: and only create the event object if there are hooks at all
(21:09:36) naderman: so typically you'd write something like
(21:10:42) naderman: if ($hook_handler->has('foobar'))
(21:10:42) naderman: {
(21:10:42) naderman:     $event = new PageHeaderEvent($page_title, $item_id);
(21:10:42) naderman:     $hook_handler->trigger('foobar', $event);
(21:10:42) naderman:     $page_title = $event->get_page_title();
(21:10:42) naderman: }
(21:11:14) naderman: well phpbb_event_page_header for the class name I guess
(21:11:16) nn-: holy loc count batman
(21:11:19) naderman: yeah
(21:11:32) naderman: nn-: you can put it in one line
(21:11:36) naderman: unless you need the return value
(21:11:38) nn-: please no
(21:11:42) naderman: even with the array you will get this
(21:11:45) nn-: that's even worse
(21:11:51) naderman: no you don't understand
(21:12:03) naderman: $hook_handler->trigger('foobar', new PageHeaderEvent($page_title, $item_id););
(21:12:08) nn-: with an array i don't get this if i use an array that is already built
(21:12:20) naderman: how likely is that?
(21:12:21) nn-: $array = hook('foo', $array);
(21:12:21) naderman: o_O
(21:12:27) naderman: yeah that will like never happen
(21:12:36) naderman: again as I said look at all the requests
(21:12:41) naderman: you nearly always need multiple variables
(21:12:47) nn-: i don't look at the requests
(21:12:47) naderman: so you will always have to construct that array as well
(21:12:49) nn-: i look at the mods
(21:12:59) naderman: the requests are from MOD authors based on what they need
(21:13:01) nn-: which may be wrong
(21:13:14) nn-: ^ me looking at mods
(21:13:17) nn-: but that's the current idea
(21:13:19) naderman: again the loc with array/object will be pretty much the same
(21:13:34) naderman: except from the classes we will need to define
(21:13:44) naderman: which again is mostly documentation which we'll have to write otherwise too
(21:13:47) nn-: well, ok
(21:13:48) naderman: just won't be tied to the code
(21:13:53) nn-: we don't need this at rev 1
(21:14:05) nn-: rev 1 is having a ledge, a manager and a hook
(21:14:17) nn-: rev 2 will be adding events on top