Laurent Laville [2006-03-08 14:20 UTC] My first comment, is only related to source code and PHP dependency.
A quick parse of source code with PHP_CompatInfo gave PHP 5.0.0 min but this result is not significant.
So i've also tried with script :
<?php
require_once 'PEAR/Common.php';
$c = new PEAR_Common();
$file = '[...]PEAR\PackageUpdate.php';
$res = $c->analyzeSourceCode($file);
?>
And found unexpected public tokens on lines :
538: public function setDontAskAgain($dontAsk)
555: public function setDontAskUntilNextRelease($nextrelease)
580: public function setMinimumRleaseType($minType)
602: public function setMinimumState($minState)
629: public function setPreference($pref, $value) {
Need to fix it or this package is only for PEAR 2 and PHP 5.x ;-)
Even Greg told us that PEAR 1.5.0 will have PHP 4.3.0 min.
Laurent Laville
Laurent Laville [2006-03-08 18:35 UTC] Here are my latest comments, while looking deeper into source code.
1. I think its dangerous to use on windows a preference file named "pear.ini" while you've named it ".ppurc" for other platforms.
remember "pear.ini" is default PEAR config file name.
2. getPackageInfo() is suppose to return void (see phpdoc), but in real situation it could also return false on error (line 345).
3. why don't you used raiseError rather than new PEAR_Error inside pushError() method ?
It could made it easy with raiseError especially on lines 343-344 and 741-742
to transmit full pear_error instance
if (PEAR::isError($result)) {
$this->pushError($result);
4. Personnaly i prefer to see usage of constant that identify an error and a global mapping for error codes => error messages rather messages all other source code.
5. last you made a typo error in your example (into proposal page and source code line 57).
if ($ppu->update() {
missing right close parenthesis.
Hope it will help and you understood me.
Laurent
Justin Patrin [2006-03-14 01:24 UTC] I would suggest using PEAR_ErrorStack instead of implementing your own error stack in the class.
I'm also not sure why you're not returning a PEAR_Error. I don't see any loops in the class itself so it doesn't make sense for it to simply aggregate its errors. The package should return PEAR::raiseError on error and let the calling package decide if it wants to aggregate or not.
As has been said before, instanceOf is PHP5 only. If this is a PHP4 package (as it seems to be) please use is_a.
|
