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
