Sat 20 Mar 2004 06:36:12 PM UTC, comment #1:
I have had a good look through this.
Comments as follows:
returns
return; vs return false;
the first returns null the other returns false - i have not checked what the call is looking for but if it is testing with is_null then we have a real problem. See http://www.php.net/manual/en/functions.returning-values.php manual text and user last comment
Initialising variables before use
This is required for php classes but not variables in php4. I actually see the there is little difference between the following:
$myarray = array();
...
$myarray['mykey'] = $value;
and:
$myarray[] = $value;
other than readability. Initialising arrays before using them was requirement in php3, but was dropped in php4. In addition to this, which is better:
$myar = array();
...
$myar['key2'] = array();
....
$myar['key2']['value3'] = 1;
or just
$myar['key2']['value3'] = 1;
I would like some documented performance stats on this before considering it. Also why do it in an obscure app, it should be done in the api first if it does lead to a performance boost, but then it would require a human code review.
display_section($appname,$file);
The appname is not undefined - it is set before the file is included. Have you looked at the other */inc/hook_prefs? (or hook_admin?) It is the way it is designed.
<br> vs <br />
<br /> is xhtml, phpgw 0.9.16 should be html 4.01 transitional compliant. See
http://www.w3.org/TR/html4/struct/text.html#edef-BR
http://www.w3.org/TR/xhtml1/#h-4.6
http://www.w3.org/TR/xhtml1/#guidelines
Either way nothing is broken
class.ufud.inc.php
All those values should be caste db_addslash()d before being given to the db, not the quick hack of assigning empty values to them, this would also improve security.
//$GLOBALS[] and other variables
Why is this done? I won't apply a patch which has no explanation of why it is done
From a thorough review of the patch I think it will be more like ~5k rather than 26k. I will post a new version tomorrow
Cheers
|