cvs: pear /MDB2_Schema/MDB2 Schema.php /MDB2_Schema/MDB2/Schema Parser.php

Igor Feghali wrote:
> ifeghali		Wed Aug 16 19:59:05 2006 UTC
> 
>   Modified files:              
>     /pear/MDB2_Schema/MDB2	Schema.php 
>     /pear/MDB2_Schema/MDB2/Schema	Parser.php 
>   Log:
>   Added support for DML statements on initializeTable()

first up .. good job!

now for the nit picking 
ok from a first scan ..

- we need to get rid of the eval() in favor of using
sprintf() or 
something similar.
eval() is evil and heavily discouraged due to security
concerns.

- buildFieldValue() does not appear to be used. either way
you should 
try to not rely on references and instead use return
whereever possible

- according to PEAR CS you may not omit curly brackets
("{}"). there are 
some if's>/foreach's where you do not use them. you
also misplace the 
curly brackets in some places. check the manual. also look
in the source 
for some examples of how i format if's that span over
multiple lines.

- you can remove the break; after a return. but i guess you
can keep 
them in if you prefer. however here the formatting is not
correct again.

- it appears you do not execute a delete if there is no
where statement. 
there is no reason for this security measure, or it should
be optional 
at best

- it would be good if we could try to leverage the function
module. this 
means you would load the function module and if there is a
method for 
the given function call, then use that. we could also see if
we could 
leverage the matchPattern() method somehow for LIKE/ILIKE
operators

regards,
Lukas

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

Lukas Kahwe Smith wrote:

> - it would be good if we could try to leverage the
function module. this 
> means you would load the function module and if there
is a method for 
> the given function call, then use that. we could also
see if we could 
> leverage the matchPattern() method somehow for
LIKE/ILIKE operators

I should expand on that. getOperator() should probably be
moved to 
MDB2_Driver_Function_Common.

regards,
Lukas

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

Hi,

i was bored from writing on my thesis paper .. so i did some
of the todo 
items already

> - we need to get rid of the eval() in favor of using
sprintf() or 
> something similar.
> eval() is evil and heavily discouraged due to security
concerns.

fixed in cvs

> - buildFieldValue() does not appear to be used. either
way you should 
> try to not rely on references and instead use return
whereever possible

i see now where you are using this.
i heared that array_walk() is not all that fast .. we should
try to 
confirm that. if its not significantly faster, i think we
should not use 
it, as it makes the code harder to read

> - according to PEAR CS you may not omit curly brackets
("{}"). there are 
> some if's>/foreach's where you do not use them.
you also misplace the 
> curly brackets in some places. check the manual. also
look in the source 
> for some examples of how i format if's that span over
multiple lines.

fixed in cvs

> - you can remove the break; after a return. but i guess
you can keep 
> them in if you prefer. however here the formatting is
not correct again.

fixed in cvs

> - it appears you do not execute a delete if there is no
where statement. 
> there is no reason for this security measure, or it
should be optional 
> at best

fixed in cvs

> - it would be good if we could try to leverage the
function module. this 
> means you would load the function module and if there
is a method for 
> the given function call, then use that. we could also
see if we could 

fixed in cvs

> leverage the matchPattern() method somehow for
LIKE/ILIKE operators

still open ..

regards,
Lukas

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php