On Mon, Sep 17, 2012 at 02:11:43PM +0200, mario@xenji.com wrote:

From: "Mario (xenji) Mueller"

Signed-off-by: Mario (xenji) Mueller --- web/lib/acctfuncs.inc.php | 5 ++ web/lib/aur.inc.php | 4 ++ web/lib/hookdef.php | 113 ++++++++++++++++++++++++++++++++++++++++++++++ web/lib/hooks/account.php | 4 ++ 4 files changed, 126 insertions(+) create mode 100644 web/lib/hookdef.php create mode 100644 web/lib/hooks/account.php

Before reviewing this, could you please explain why hooks are advantageous here? Hooks are nice if you want to change entry points of a function invocation dynamically, such as in extensions or complex configurations, but I don't see how a hook framework is supposed to be superior to a simple function call here.

diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php index 6f7c98a..0ccd235 100644 --- a/web/lib/acctfuncs.inc.php +++ b/web/lib/acctfuncs.inc.php @@ -188,6 +188,11 @@ function process_account_form($UTYPE,$TYPE,$A,$U="",$T="",$S="",$E="", } else { # account created/modified, tell them so. # + + // trigger hook and pass the fully escaped array to it. + // see lib/hookdef.php for details. + trigger_hook('newAccount', $escaped); + print __("The account, %s%s%s, has been successfully created.", "<b>", htmlspecialchars($U,ENT_QUOTES), "</b>"); print "<p>\n"; diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php index 6dcbb34..50c4bce 100644 --- a/web/lib/aur.inc.php +++ b/web/lib/aur.inc.php @@ -11,6 +11,10 @@ include_once('translator.inc.php'); set_lang();

include_once("config.inc.php"); + +include_once("hookdef.php"); +include_once("hooks/account.php"); + include_once("routing.inc.php"); include_once("version.inc.php"); include_once("acctfuncs.inc.php"); diff --git a/web/lib/hookdef.php b/web/lib/hookdef.php new file mode 100644 index 0000000..19d93d5 --- /dev/null +++ b/web/lib/hookdef.php @@ -0,0 +1,113 @@ + + * @since 2012-09-17 + */ +function trigger_hook($sHookName, $mPayload) +{ + error_log('Started ' . __FUNCTION__); + $aUserDefFunctions = get_defined_functions()['user']; + error_log('Got defined fncs: ' . implode(', ', $aUserDefFunctions)); + + // We just want to process the relevant hooks, no other hooks or fnc. + $oFilter = new \CallbackFilterIterator( + new \ArrayIterator($aUserDefFunctions), + function ($mCurr, $iKey, $oIter) use ($sHookName) + { + // keep this with three eq. signs, it is needed! + if ( + strpos($mCurr, 'hook_') !== false + && strpos(strtolower($mCurr), strtolower($sHookName)) !== false + ) + { + error_log('Hookname OK: '. $sHookName . ' in ' . $mCurr); + return true; + } + return false; + }); + + + $oHookQueue = new \HookQueue(); + + foreach ($oFilter as $sFunction) + { + // $_ => unused declaration, borrowed from scala's convention + list($_, $_, $iWeight, $_) = explode('_', $sFunction); + error_log("Inserting $sFunction into Queue with weight $iWeight"); + $oHookQueue->insert($sFunction, (int)$iWeight); + } + + // We just want to have the function name, not the prio + $oHookQueue->setExtractFlags(\SplPriorityQueue::EXTR_DATA); + if ($oHookQueue->count() > 0) + { + $oHookQueue->top(); + + foreach ($oHookQueue as $sFunction) + { + call_user_func($sFunction, $mPayload); + } + } +} + +/** + * This is an implementation of the prio queue, which fits perfectly for our + * need to sort the hooks in relation to their weight. + * + * Taken from the example on php.net, because I was too lazy to type the text. + * + * @see http://php.net/manual/de/class.splpriorityqueue.php + * @author xenji + * @since 2012-09-17 + */ +class HookQueue extends SplPriorityQueue +{ + public function compare($priority1, $priority2) + { + if ($priority1 === $priority2) return 0; + return $priority1 < $priority2 ? -1 : 1; + } +} diff --git a/web/lib/hooks/account.php b/web/lib/hooks/account.php new file mode 100644 index 0000000..aea57b5 --- /dev/null +++ b/web/lib/hooks/account.php @@ -0,0 +1,4 @@ +

On Tue, Sep 18, 2012 at 11:05:31PM +0200, Mario Mueller wrote:

Simple function call would be enough to solve the exact need at every single point of extension, so there is no need to defend my proposal. I this is the way you want to expand this, there is no need for my patch. It will make contributions even harder as they are now, because everybody needs to understand every piece of the code, wich is - please don't get me wrong - pretty hard without enough of documentation. Hooks make your life easier, as you enable contributions on a very low level with mostly defined APIs.

If I got you right, the hook framework you suggested allows for defining functions with dynamic name binding. These are usable if you need to redefine a function's semantics and/or choose between different implementations -- features that are great if plugins/extensions are supported. However, I'm still not sure how hooks make contributions easier for us. You will still have to look up the hook name (vs. looking up the function name) and you will still have to write the hook code (vs. writing the function body). I agree that both the notification code and documentation need a lot of work, but I don't think a hook framework is needed -- unless we want to introduce a plugin interface.

The need really depends on how you want to mangage the existing code. If you want to touch it as little as possible, hooks are *one* of the ways that you can possibly go.

I hope I pointed out the advantages.

2012/9/18 Lukas Fleischer

...

On Mon, Sep 17, 2012 at 02:11:43PM +0200, mario@xenji.com wrote:

...

From: "Mario (xenji) Mueller"

Signed-off-by: Mario (xenji) Mueller --- web/lib/acctfuncs.inc.php | 5 ++ web/lib/aur.inc.php | 4 ++ web/lib/hookdef.php | 113 ++++++++++++++++++++++++++++++++++++++++++++++ web/lib/hooks/account.php | 4 ++ 4 files changed, 126 insertions(+) create mode 100644 web/lib/hookdef.php create mode 100644 web/lib/hooks/account.php

Before reviewing this, could you please explain why hooks are advantageous here? Hooks are nice if you want to change entry points of a function invocation dynamically, such as in extensions or complex configurations, but I don't see how a hook framework is supposed to be superior to a simple function call here.

...

diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php index 6f7c98a..0ccd235 100644 --- a/web/lib/acctfuncs.inc.php +++ b/web/lib/acctfuncs.inc.php @@ -188,6 +188,11 @@ function

process_account_form($UTYPE,$TYPE,$A,$U="",$T="",$S="",$E="",

...

} else { # account created/modified, tell them so. # + + // trigger hook and pass the fully escaped array to it. + // see lib/hookdef.php for details. + trigger_hook('newAccount', $escaped); + print __("The account, %s%s%s, has been

successfully created.",

...

"<b>",

htmlspecialchars($U,ENT_QUOTES), "</b>");

...

print "<p>\n"; diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php index 6dcbb34..50c4bce 100644 --- a/web/lib/aur.inc.php +++ b/web/lib/aur.inc.php @@ -11,6 +11,10 @@ include_once('translator.inc.php'); set_lang();

include_once("config.inc.php"); + +include_once("hookdef.php"); +include_once("hooks/account.php"); + include_once("routing.inc.php"); include_once("version.inc.php"); include_once("acctfuncs.inc.php"); diff --git a/web/lib/hookdef.php b/web/lib/hookdef.php new file mode 100644 index 0000000..19d93d5 --- /dev/null +++ b/web/lib/hookdef.php @@ -0,0 +1,113 @@ +

...

+ * If you think about making it "better" by creating a OOP version with + * interfaces, please take a look at the rest of the system and ask yourself + * if this really fits in here. + */ + +// LIST OF EXISTING HOOKS +/* + * USE THIS PATTERN: + * + * filename.php hookname (payload1, payload2, ...) + * + * acctfuncs.inc.php newAccount (array($U, $E, $P, $salt, $R, $L, $I, $K)) + */ + +/** + * Triggers the hook by it's name and passes the given payload to the hook. + * + * The trigger does not return anything as they must be used in a fire-and- + * forget manner. + * + * The functions for the hooks must follow a naming convention to be

...

+ * by this function. Keep in mind that the weight goes from 1 (lowest) to + * PHP_INT_MAX (highest). The higher the number the earlier it will be executed. + * + * The pattern is: + * + * <code>hook_$hookname_$weight_uniqueId</code> + * + * @example <code> + * function hook_registerUser_15_SendmailOnReg($sUsername) { do something } + * // or + * function hook_updatePackage_1_SendmailOnUpdt($sPkgName, $sUsername) { + * do something + * } + * </code> + * + * @param string $sHookName The hook name to be triggered + * @param mixed $mPayload A nullable payload to pass to the hook. + * + * @return void + * @author xenji + * @since 2012-09-17 + */ +function trigger_hook($sHookName, $mPayload) +{ + error_log('Started ' . __FUNCTION__); + $aUserDefFunctions = get_defined_functions()['user']; + error_log('Got defined fncs: ' . implode(', ', $aUserDefFunctions)); + + // We just want to process the relevant hooks, no other hooks or fnc. + $oFilter = new \CallbackFilterIterator( + new \ArrayIterator($aUserDefFunctions), + function ($mCurr, $iKey, $oIter) use ($sHookName) + { + // keep this with three eq. signs, it is needed! + if ( + strpos($mCurr, 'hook_') !== false + && strpos(strtolower($mCurr), strtolower($sHookName)) !== false + ) + { + error_log('Hookname OK: '. $sHookName . ' in ' . $mCurr); + return true; + } + return false; + }); + + + $oHookQueue = new \HookQueue(); + + foreach ($oFilter as $sFunction) + { + // $_ => unused declaration, borrowed from scala's convention + list($_, $_, $iWeight, $_) = explode('_', $sFunction); + error_log("Inserting $sFunction into Queue with weight $iWeight"); + $oHookQueue->insert($sFunction, (int)$iWeight); + } + + // We just want to have the function name, not the prio + $oHookQueue->setExtractFlags(\SplPriorityQueue::EXTR_DATA); + if ($oHookQueue->count() > 0) + { + $oHookQueue->top(); + + foreach ($oHookQueue as $sFunction) + { + call_user_func($sFunction, $mPayload); + } + } +} + +/** + * This is an implementation of the prio queue, which fits perfectly for our + * need to sort the hooks in relation to their weight. + * + * Taken from the example on php.net, because I was too lazy to type

stuff. triggered the text.

...

+ * + * @see http://php.net/manual/de/class.splpriorityqueue.php + * @author xenji + * @since 2012-09-17 + */ +class HookQueue extends SplPriorityQueue +{ + public function compare($priority1, $priority2) + { + if ($priority1 === $priority2) return 0; + return $priority1 < $priority2 ? -1 : 1; + } +} diff --git a/web/lib/hooks/account.php b/web/lib/hooks/account.php new file mode 100644 index 0000000..aea57b5 --- /dev/null +++ b/web/lib/hooks/account.php @@ -0,0 +1,4 @@ +

I'll double check that. Von meinem iPhone gesendet Am 17.09.2012 um 17:20 schrieb "canyonknight@gmail.com" :

On Mon, Sep 17, 2012 at 8:11 AM, wrote:

...

From: "Mario (xenji) Mueller"

Signed-off-by: Mario (xenji) Mueller --- web/lib/acctfuncs.inc.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php index d58c759..6f7c98a 100644 --- a/web/lib/acctfuncs.inc.php +++ b/web/lib/acctfuncs.inc.php @@ -327,9 +327,8 @@ function search_results_page($UTYPE,$O=0,$SB="",$U="",$T="", }

$result = db_query($q, $dbh); - $num_rows = mysql_num_rows($result);

Is this really unused? I can't check right now, but I have a feeling this breaks account searching. The account search results page is finicky and needs a lot of work (pagination more like package results page, GET requests rather than POST, etc).

...

- while ($row = mysql_fetch_assoc($result)) { + while (($row = mysql_fetch_assoc($result)) != false) {

I don't know Lukas' preference here. A style for this should probably be established and used.

...

$userinfo[] = $row; }

-- 1.7.12

On Mon, Sep 17, 2012 at 8:06 PM, Lukas Fleischer wrote:

On Mon, Sep 17, 2012 at 11:19:33AM -0400, canyonknight@gmail.com wrote:

...

On Mon, Sep 17, 2012 at 8:11 AM, wrote:

...

From: "Mario (xenji) Mueller"

Signed-off-by: Mario (xenji) Mueller --- web/lib/acctfuncs.inc.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php index d58c759..6f7c98a 100644 --- a/web/lib/acctfuncs.inc.php +++ b/web/lib/acctfuncs.inc.php @@ -327,9 +327,8 @@ function search_results_page($UTYPE,$O=0,$SB="",$U="",$T="", }

$result = db_query($q, $dbh); - $num_rows = mysql_num_rows($result);

Is this really unused? I can't check right now, but I have a feeling this breaks account searching. The account search results page is finicky and needs a lot of work (pagination more like package results page, GET requests rather than POST, etc).

I hope it's unused -- you removed it in the PDO patch (which also supersedes this one) :)

You are correct I removed it in the PDO patch as there isn't a one-to-one equivalent for mysql_num_rows(). The difference is I also made a change at the same time to the $num_row reference in account_search_results.php. So my removal *should* work.

...

...

- while ($row = mysql_fetch_assoc($result)) { + while (($row = mysql_fetch_assoc($result)) != false) {

I don't know Lukas' preference here. A style for this should probably be established and used.

The original version is obviously easier to read... I agree that we need to improve our coding style guidelines and add some more principles/examples.

Sounds good.

...

...

$userinfo[] = $row; }

-- 1.7.12