Re: [PEPr] Call for votes on Encryption::Crypt_RSA

Alexander Valyalkin wrote:

New vesrion of Crypt_RSA package is available at http://chat.finalcombat.com/valyala/big_int/Crypt_RSA-1.0.0RC4.tgz I did most of Philippe's proposes: - class/directory structure now is PEAR-compliant - removed debug functions from bottom of BCMath wrapper file - removed all calls of exit() function. It mostly replaced by PEAR::raiseError() calls - $wrapper_name now is optional parameter. System tries to load the best abailable

      wrapper, if its name is omitted.

I didn't implement &factory() method. I think, it is completely unnesessary in Crypt_RSA* classes. Why don't use constructors instead? Compare: $rsa = &new Crypt_RSA($params, $wrapperName); // constructor usage $rsa = &Crypt_RSA::factory($params, $wrapperName); // factory() usage Can anybody show me any profit of second line? Philippe Jausions wrote:

That allows you to return a PEAR_Error object if no wrapper class or extension can be found.

Ok, it seems as good reason. But again, compare that methods: // constructor usage $rsa = &new Crypt_RSA; if ($rsa->isError()) { $err = $rsa->getLastError(); echo $err->getMessage(); } // factory() usage $rsa = &Crypt_RSA::factory(); if (PEAR::isError($rsa)) { echo $err->getMessage(); } Is that ritorical question: what purpose of constructors, when factory() method usage? Again, if I'll implement factory() method, I have to duplicate it in three classes: Crypt_RSA, Crypt_RSA_Key, Crypt_RSA_KeyPair. Now all of these classes uses one codebase, placed in Crypt_RSA_MathLoader class, to load math wrapper. Any suggestions?

The problem with the constructor alone is that you are returning an half broken object. Your code $rsa->isError() works but then you are introducing a separate error handling method. Consistent error handling solution thoughout all PEAR packages is important. Of course, *the* real solution with constructors is using exceptions, but this alone doesn't justify to move your package to PHP5 only. I also understand the repeated factory() problem, that is why I suggested that you use PEAR::getStaticProperty() to store a wrapper. One solution would be to load the wrapper class and intialize a math wrapper object using PEAR::getStaticProperty() when the Crypt/RSA.php is loaded. Then for every call to one of the method of the class do a "if ($this->isError()) {return PEAR::raiseError(blah blah blah);}" to make sure that the wrapper has been initialized properly. If someone wanted to force a specific wrapper (for benchmarking for instance) you could provide a Crypt_RSA::useWrapper() method as previously suggested. This way they can switch math wrapper at will... The downside of all this is of course you have to do the if ($this->isError()) everywhere. That could make it difficult for someone to extend your class (although, right now, I don't see why one would do such thing...) -Philippe