Hey, so here are some thoughts. I'm commenting mostly on paypal, since I'm familiar with them. I think most of this applies to your other drivers/gateways as well. From what I remember with working with Paypal (we do a lot of subscriptions), it's a pain to work with the API and it's far from obvious. I think the objective should be to make it usable to the developer.
First off, one question. We are doing subscriptions, and the way it's setup is, that Paypal notifies us. This notification is independent of the general request and the customer. They aim to do it in 'realtime', but it can sometimes take up to five minutes for Paypal to notify us that the 'order' has been processed while the customer has already seen a 'Thank you for your money' on Paypal's website. Now, the Q is -- how do I receive these notifications with your code? I couldn't figure it out, because what I have to do is wait for a request from paypal and resubmit all parameters for verification. But maybe I've overlooked that one. So apologies in advance. On to my feedback, I hope you don't mind. (Just let me stress again, that even though I got some things that I'd like to see improved/changed, I really appreciate the work done so far very much.) 1) generally, don't use private, as it makes extending your code 'harder'. I couldn't override/extend the paypal gateway and then I couldn't access the httpclient. Use protected instead. 2) Generally public class members (variables and methods) shouldn't be prefixed with '_', only protected and private should. 3) Provide a way for people to override the Zend_Http_Client, e.g.: public function __construct(Zend_Http_Client $client = null) { if ($client !== null) { $this->client = $client; } else { // create instance here } } Maybe also add a setter, setHttpClient(Zend_Http_Client $client). 4) use set and get more appropriately, e.g. creditcardInfo(), should be something like, setCreditcardInfo() (or maybe, setCreditcard()?). 5) Methods such as 'addressVerify' could be more simple/convenient for the developer, e.g. isVerifiedAddress($street, $etc) -- it would return a boolean. I don't see why 'addressVerify' in its current form returns an array. A boolean would be more straight to the point (kind of like you did with isSuccess()). 6) In general/IMHO, there's too much in one class. I understand that it's all related to paypal, but authorization, transaction management, etc. could be phased out into subclasses. e.g.: Zend_Payment_Gateway_Paypal_Transaction (Zend/Payment/Gateway/Paypal/Transaction.php) $paypal = Zend_Payment::factory('paypal'); $transaction = $paypal->transaction(); $transaction->get($id); $transaction->doRefund($id, 'full'); $transaction->search('2009-04-27'); Or: $order = $transaction->create(...); $order->setCustomer(Zend_Payment_Gateway_Paypal_Customer $customer); ... Zend_Payment_Gateway_Paypal_Customer (Zend/Payment/Gateway/Paypal/Customer.php) $customer = $paypal->customer($id); // an identifier of the customer $customer->getBalance(); $customer->getAddressbook(); $customer->addAddress(); $customer->isVerified(); (These are just examples, as of how the interface could be more intuitive.) 7) header() calls in your library should be avoided at all costs for multiple reasons, e.g. it makes it harder (for yourself) in terms of testability. And what if I wanted to display the link instead to my customer? (E.g. "click here to continue to paypal"). Instead those methods should be renamed to, 'getExpressCheckoutLink', 'getBillingAgreementLink' (or similar). With the getter, the developer can either pipe it to header() himself, or do the redirect using the framework code, or display the link in a view so people click on it themselves, etc.. 8) Simplify control-flow, e.g. in your current form, massPay() should look like this instead: public function massPay(array $_receivers=array() ) { if (count($_receivers) == 0) { return null; } $this->addParam('METHOD', 'MassPay'); foreach($_receivers as $i => $receiver) { $this->addParam('L_EMAIL'.$i, $receiver['email']); $this->addParam('L_AMT'.$i, urlencode($receiver['amount'])); $this->addParam('L_UNIQUEID'.$i, urlencode($receiver['uniqueID'])); $this->addParam('L_NOTE'.$i, urlencode($receiver['note'])); } return $this->_buildRequest(); } Save an is_array() by forcing the type to array in the method's signature, and (my personal favorite) exit the method early if there's nothing to process. 9) phpdoc and general CS could use some love. :) 10) Since you're using Zend_Http_Client in your code, you should trap its exceptions, and re-cast, e.g.: (in Zend_Payment_Gateway_Paypal::_buildRequest()) try { $this->client->request(); } catch (Zend_Http_Client_Exception $e) { $msg = "Message: {$e->getMessage()}, Code: {$e->getCode()}"; throw new Zend_Payment_Exception($msg, Zend_Payment::ERR_HTTP); } 11) I'm not sure, if I'm the only one using exception codes, but I'd also like to see that across the board. E.g, defining them as class constants in Zend_Payment, ERR_HTTP, ERR_INPUT, ERR_GATEWAY (just examples). An alternative is using different exception classes, Zend_Payment_Http_Exeption, Zend_Payment_Input_Exception -- but IMHO, that's overheadish. Just some pointers, hope it helps. Till