Separation of concerns concern (!) in IPN Trait
You may not want to fix this @mattwire but I'm noting it here because it's something that stops me chosing to use this trait, and thought it might be helpful to log why.
The IPN class is a request controller that takes whatever the PP throws its way, and is responsible for parsing and processing that data and producing some sort of output suitable to the external PP's needs.
If all PPs sent one event per IPN, then this trait would be fine. However it's very common for an IPN to bundle a few events in one IPN call, which is where things get knotty.
e.g. Most of the properties added by the IPN trait belong to a single event, not to the IPN. e.g. contributionRecurID - and the associated getter/setter methods.
This means that the same instance of this IPN class holds data that belongs to different events. I think it's better separation of concerns to have a separate event processing class to encapsulate all the data to do with a single event.
e.g. the following error becomes easy to make, and it becomes easier the more event-level properties you add to the IPN trait:
class myIPN {
use CRM_Core_Payment_MJWIPNTrait;
public function processTheIPN(array $events) {
foreach ($events as $event) {
if (!empty($event->thirdPartyRefToContributionRecurID)) {
$this->setContributionRecurID($event->thirdPartyRefToContributionRecurID);
}
// ...
$this->carryOnProcessingEvent($event);
}
}
The error here being that if event 1 has a recur ID, but event 2 doesn't, event 2 inherits the recur of event 1.
Of course you can solve this by verbose code that resets all these at the end of the event loop, but it's a foot-gun. I prefer a pattern like:
class myIPN {
use CRM_Core_Payment_MJWIPNTrait;
public function processTheIPN(array $events) {
foreach ($events as $event) {
$eventProcessor = new \Civi\MyProcessor\EventProcessor($event);
$eventProcessor->doYourThing();
}
}
}
then
class Civi\MyProcessor\EventProcessor {
protected ?int $contributionRecurID = NULL;
public function __construct($event) {
if (!empty($event->thirdPartyRefToContributionRecurID)) {
$this->contributionRecurID = $event->thirdPartyRefToContributionRecurID;
}
// ...
$this->carryOnProcessingEvent($event);
}
}
Obviously the constructor might sensibly need extra data (like the payment processor, which does belong to the IPN data, typically), but the point is that this code:
- has good separation of concerns: the event processor class does just that.
- saves us from ourselves; as the object is not reused between events, we don't need to worry about remembering to reset all the vars.
Anyway, I realise nobody has to use mjwshared, and nobody has to use the ipn trait, but I thought I'd pen this rather than silently moving away.