MicahCarrick / PHP-PayPal-IPN

PHP 5 class to listen for and handle Instant Payment Notifications (IPN) from PayPal.
BSD 3-Clause "New" or "Revised" License
419 stars 211 forks source link

processIpn should have a $data argument so it can be used in frameworks etc #1

Closed rudiedirkx closed 13 years ago

rudiedirkx commented 13 years ago

Because some frameworks delete $_POST and save all post data to some other location. Or when you want to test IPN data.

A library should never depend on environment variables (like $_POST).

So I propose:

public function processIpn( $ipn ) {

and

$verified = $listener->processIpn($_POST);
// or
$verified = $listener->processIpn(array('a' => 'b', 'c' => 'd', 'e' => 'f'));
// etc

(I also don't see why processIpn would check the request's REQUEST_METHOD...)

Quixotix commented 13 years ago

Good call. I think I'll add an optional parameter to processIpn() for passing in the POST data, which will default to $_POST if not provided (and if $_POST exists).

As for requiring a request method of POST, an IPN is always a POST request. Should somebody navigate to the ipn listener through a GET request or if something really goes wrong and the listener gets indexed in Google, a 405 would be an appropriate response. It also provides a simple way to ensure exception handling and logging is working in the listener.

rudiedirkx commented 13 years ago

Should somebody navigate to the ipn listener through a GET request ...

But what about my test page that uses the Listener? I'm not firing a POST request every time. I just hit refresh and my test IPN is loaded. Checking for POST is also environment and should be done outside the class. Or put it in a method that you can call before you call processIpn. Like so in the actual IPN page:

$ipn->requirePostMethod();
$ipn->requireSomeOtherEnvironmentValueOrFlag();
$ipn->processIpn($_POST);

and so in my test page (that can be anywhere):

// don't check POST or other env stuff. It's just a test.
$ipn->processIpn($myTestIPN);

If you force these env checks, you can never use the class in a test...

rudiedirkx commented 13 years ago

Or my test cases that don't even require an HTTP environment. They should be able to use the IPN class. TDD to the max!

Quixotix commented 13 years ago

You're right, from a design perspective it belongs outside of the IPN class. I don't think it's necessary, but I also like $ipn->requirePostMethod(). I'll think on that a bit.

Most folks are going to be testing this with real post data from the PayPal IPN Simulator in the sandbox since the IPN will always fail if it's not an exact post back from PayPal. You won't be able to test "VERIFIED" conditions with testing tools--but of course you can test for communication problems between you and Paypal.

Part of my goal here was to make this nice and easy to use in response to the years worth of emails I've gotten about an old IPN script I wrote a while back. People have a hard time understanding the IPN is coming

rudiedirkx commented 13 years ago

True that. Easy should be the goal =)