[PEPr] Comment on Encryption::Crypt_GPG

Daniel Convissor (http://pear.php.net/user/danielc) has commented on the proposal for Encryption::Crypt_GPG. Comment: Excellent job! While "import" is the name of the command line switch for importing keys, please consider a more informative name for the import() method, like importKey(). You have getFingerprint() return a solid string (F15AB7E29D356C64CEB2C00FE8142F028FFE1FFC). But, fingerprints are generally displayed to be human readable (F15A B7E2 9D35 6C64 CEB2 C00F E814 2F02 8FFE 1FFC). I see you're running str_replace() to strip out the spaces. I'd suggest there be a second parameter to the method allowing users to choose which they want and have the spaces left in by default. OH, also, you need to run trim() on the result. It comes back on my system with a trailing LF. Please come up with file methods. Perhaps it can be as simple as adding the file name as an element of $args: public function encryptFile($file, $username, $armor = true) { $args = array('--recipient "' . $username . '"', '--encrypt'); if ($armor) { $args[] = '--armor'; } $args[] = $file; // ... snip ... } When using the test script with my own existing key, things worked fine in the encrypt() call, but the call to decrypt() threw the following error: gpg: failed to translate osfhandle 00000004 Dude, putting a call to deleteSecretKey() inside a publicly distributed test script is dangerous. If you want to keep it there, you MUST place a VERY bold warning above the $key_id = '' on line 85. For example, I just zoomed ahead and set it to my key id and POOF went my secret key. Fortunately, I have backups. :) In several of the docblocks, I see reference to a file named "doc/DETAILS." That directory doesn't exist in the tarball. Perhaps you forgot to include it in the package.xml file. REGARDLESS, that information should really be in the docblocks. For example, the docblock for listKeys() should explain the format of the arrays/objects returned. SO... What IS that array key? I'm used to seeing things like "8FFE1FFC" but that's returning "E8142F028FFE1FFC". Also, make the key a property of the object as well. In listKeys() you use $key->creation_date but in the Crypt_GPG_Key class you defined $create_date. There are a few nit-picky coding standards things... Put the license stuff into page-level docblock instead of a separate block comment above it. Use docblocks, not just block comments, above the include/require statements. Have the short descriptions in the page-level docblocks truly reflect the contents of the given file rather than just using the same description for each file. 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