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

 
 

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

patch #2804: zend patch for fudforum

Submitted by:  Kai Hofmann <powerstat>
Submitted on:  Tue 16 Mar 2004 09:03:44 AM UTC  
 
Category: phpgw-FUDforumPriority: 7 - High
Item Group: NoneStatus: None
Privacy: PublicAssigned to: Kai Hofmann <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>
Project 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>In charge of this item.

 

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

Do you think this task is very important?
If so, you can click here to add your encouragement to it.
This task has 0 encouragements so far.

Only logged-in users can vote.

 

Please enter the title of George Orwell's famous dystopian book (it's a date):

 

 

Follow 6 latest changes.

Date Changed By Updated Field Previous Value => Replaced By
Sun 21 Mar 2004 02:36:33 PM UTCskwashdAssigned toskwashd=>powerstat
Sun 21 Mar 2004 02:35:07 PM UTCskwashdAttached File-=>Added fud2.patch, #2963
Tue 16 Mar 2004 12:56:11 PM UTCcboettgerAssigned tocboettger=>skwashd
  Originator Email=>-unavailable-
  Carbon-Copy-=>Added powerstat
Tue 16 Mar 2004 09:03:45 AM UTCpowerstatAttached File-=>Added 20040316.patch, #2924

Back to the top


Powered by Savane 3.1-cleanup