[PEPr] Comment on PHP::opensrf-php
[email protected] ("Till Klampaeckel")
| Newsgroups | php.pear.dev |
|---|---|
| Message-ID | <[email protected]> |
Additional comments on your latest changes: 1) The helper methods should go into an object then. 2) Try to make objects injectable – e.g. that instance of HTTP_Request2. It's good that you're using other PEAR components but the way you use it means that it cannot be configured by the user of your library. They may need a proxy to connect, they may want to use cURL, they may want to set a timeout, etc.. 3) Avoid constants in the global scope and provide configuration to the classes via injection: https://github.com/pranjal710/osrf/blob/master/lib/Config.php – configuration should be given to the classes via parameters. People typically install PEAR code in some system wide directory and they shouldn't be required to edit files in there. 4) Avoid func_get_args() — define parameters in the method's signature and use them accordingly. 5) Avoid including inside a class – if necessary do it before the class body. But it would be much nicer to use autoload. 6) Various coding style things — the most obvious is stacking your classes into a common namespace (maybe "OSRF"). PEAR uses the File-ClassName-convention. This means: lib/OsrfSession.php would be lib/OSRF/Session.php. For all other coding style issues, please run phpcs (PHP_CodeSniffer) on your codebase. 7) It would be nice to see some test cases. 8) Code execution in the global scope: https://github.com/pranjal710/osrf/blob/master/lib/Fieldmapper.php#L51-L69 — It's also not very 'defensive'. When one of those calls fails, it'll fail with E_WARNING, E_NOTICE and possible E_FATAL. Libraries need to be robust and return a clear status to the user. -- http://pear.php.net/pepr/pepr-proposal-show.php?id=681