patchphpGroupWare - Patches: patch #2804, zend patch for fudforum

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #2804: zend patch for fudforum

Submitter:  Kai Hofmann <powerstat>
Submitted:  Tue 16 Mar 2004 09:03:44 AM UTC
   
 
Category:  phpgw-FUDforum Priority:  7 - High
Item Group:  None Status:  None
Privacy:  Public Assigned to:  powerstat
Open/Closed:  Open

Sun 21 Mar 2004 02:36:33 PM UTC, comment #2: 

Here is the new version - half the size of the original patch

Dave Hall <skwashd>
Group administrator
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

Anonymous
Tue 16 Mar 2004 09:03:44 AM UTC, original submission:  

Problems found with zend code analyzer fixed and partly only marked.

Kai Hofmann <powerstat>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #7165:  fud2.patch added by skwashd (13KiB - text/x-patch - This is what I am happy to consider applying - if it is properly tested)
file #7128:  20040316.patch added by powerstat (26KiB - application/octet-stream - already dos2unixed)

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

 

Follow 6 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2004-03-21 skwashd Assigned toskwashd powerstat
2004-03-21 skwashd Attached File- Added fud2.patch, #2963
2004-03-16 cboettger Assigned tocboettger skwashd
    Originator Email -email is unavailable-
    Carbon-Copy- Added powerstat
2004-03-16 powerstat Attached File- Added 20040316.patch, #2924

Back to the top

Powered by Savane 3.13-4448.
Corresponding source code