Re: Net_Vpopmaild

Joe Stump wrote: > 1.) It seems everything is public. I'd think $log, for instance, could > be private. I don't see a reason why anything would need to log > outside of this package. Additionally, it might make more sense to > implement an accept() method so that I can pass my own non-file Log > object to Net_Vpopmail for logging. Ok, that's an interesting idea. I figured you could just overwrite the constructor and instantiate Log with your own parameters. But accept() seems more flexible. > 2.) All if/else, etc. must have { } e.g. one line if statements don't > conform to PEAR CS. It must be: > > if (true) { > return true; > } Ah, I still have the habit of not bracing single line conditions. I'll fix them. > 4.) PHP5 packages cannot use PEAR_Error and must use exceptions. > Create Net_Vpopmail_Exception extending PEAR_Exception and throw that > instead of PEAR_Error. > > 5.) I'm fairly sure Net_Socket and the Mail package you use are PHP4. > PHP5 PEAR packages cannot rely on PHP4 packages per the RFC's. > RE: #4, is that for raising errors only? And is there a PHP5 version of Net_Socket underway? Or would I need to make Net_Vpopmaild PHP4 compliant to release it as stable? > 6.) In setLimits() I'd make those arrays static so they aren't created > in memory for each time the function is called. > Ah, that's a good idea. > 7.) Instead of validEmailAddress() maybe use Validate::email() ? Not > sure if that's RFC compliant though. Looks like the 'use_rfc822' option can make it RFC compliant. > 8.) Nitpicky, but I don't see any .phpt or unit tests. > Yeah, I was waiting for feedback before I delve into tests, examples, etc. > For a first time package, though, this looks good and definitely > serves an awesome purpose. Thanks! Thanks for the detailed feedback. Regards, Bill