[PEPr] Comment on Encryption::GPG

Daniel Convissor (http://pear.php.net/user/danielc) has commented on the proposal for Encryption::GPG. Comment: This is an important and helpful package. Your approach is generally good, though needs some refinement. Make the directory structure like this: Crypt/GPG/package.xml Crypt/GPG/GPG.php Remove the README file. All of the information in there should be in the GPG.php file's docblocks. Follow the coding standards. Notable problems in your code are: * header comment blocks. * use of brackets for functions and control structures. * function docblock layout. See the Sample File in the coding standards. * just because a method is private doesn't mean it doesn't need a docblock. * spacing around operators (+, ., etc). * method naming convention (for example "process_with_passphrase" should be "_process", "open_subprocess" should be "_openSubprocess", etc). * put docblocks for the properties. * use ' instead of " for strings that don't need evaluation. * wrap lines at 80 columns. * declare the $process property. * use full open tags <?php, not short tags. * nest with spaces, not tabs. Define the properties to the data types they are: $gpg_binary = ''; $pipes = array(); The listKeys() method could produce much more useful information. For example, make the key id the index for the array and make the data a sub-array of that, don't ignore sub-keys, provide uid's, etc. So, instead of returning this: [0] => pub 1024D/8FFE1FFC 2002-12-23 return this: [8FFE1FFC] => Array [type] => pub [size] => 1024D [created] => 2002-12-23 [uid] => Array [danielc@analysisandsolutions.com] => Daniel Convissor (Office) In close_subprocess(): if ($this->process != null && is_resource($this->process)) { can just be: if (is_resource($this->process)) { and set $this->pipes = array(); instead of null. There doesn't seem to be a compelling reason to make this package PHP 5 only, though the package does require 4.3.0 due to proc_open(). The package.xml file says the license is LGPL but the docblock says it's GPL. Use the LGPL @license tag from the Header Comment Block page of the coding standards. I may have more feedback later. This is an important package. Please don't rush it to vote. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=229 -- Sent by PEPr, the automatic proposal system at http://pear.php.net