dev/financial#33 Add in Hook postIPNProcess to allow Extensions to do c… #12928

seamuslee001 · 2018-10-12T00:49:29Z

…ustom processing of the IPN Data

Overview

This adds in a hook alterIPNData which would allow extension authors the ability to do custom processing on IPNs as they come in

Before

No Hook on the IPN data

After

Hook exists, meaning custom processing such as sending google analytics information based on the IPN data can occur

ping @eileenmcnaughton @monishdeb @johntwyman

civibot · 2018-10-12T00:49:31Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

eileenmcnaughton · 2018-10-12T07:39:42Z

@seamuslee001 we have some tests for alterPaymentProcessorParams in our suite - can we get a similar one for this

pradpnayak · 2018-10-12T08:55:42Z

Thanks @seamuslee001 for the new hook implementation. I have extension that does need to perform some validation and alter to ipn's $_REQUEST before they are processed. I used config hook as an alternate method but alterIPNData seems to be most generic.

seamuslee001 · 2018-10-14T01:00:15Z

@eileenmcnaughton i have added in a unit test now

monishdeb · 2018-10-15T12:10:51Z

CRM/Utils/Hook.php

+   * @param array $IPNData - Array of IPN Data
+   * @return mixed
+   */
+  public static function alterIPNData($IPNData) {


@seamuslee001 did you missed & here or there is a reason you don't want to pass by reference?

At dev/financial/issues/33, @seamuslee001 writes

We could also ensure that the original data not get modified by not passing by reference so that the hook could only do its own processing without affecting the original data

The intention needs to be clarified.

I think it makes sense to pass by reference - a hook should be able to alter

monishdeb · 2018-10-15T12:16:18Z

@seamuslee001 Except for that minor change I mentioned on the patch, can you please submit a doc PR? against https://github.com/civicrm/civicrm-dev-docs

eileenmcnaughton · 2018-10-16T01:03:43Z

CRM/Core/Payment/PayPalImpl.php

@@ -745,6 +745,7 @@ public function cancelSubscription(&$message = '', $params = array()) {
   */
  static public function handlePaymentNotification() {
    $params = array_merge($_GET, $_REQUEST);
+    CRM_Utils_Hook::alterIPNData($_REQUEST);


I think you need to pass $params here not $_REQUEST

eileenmcnaughton · 2018-10-16T01:04:11Z

tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php

+  /**
+   * Store Custom data passed in from the PayPalIPN in a custom field
+   */
+  public function HookCiviCRMAlterIPNData($data) {


trivial point - lower case h I think

eileenmcnaughton · 2018-10-16T01:08:25Z

extern/authorizeIPN.php

@@ -38,6 +38,7 @@

 require_once '../civicrm.config.php';
 $config = CRM_Core_Config::singleton();
+CRM_Utils_Hook::alterIPNData($_REQUEST);


I don't think we should add hooks into these deprecated paths - there is some risk of them not working from previous nightmares with these bits of code so I'd rather not touch (also people should stop using them)

eileenmcnaughton · 2018-10-16T01:09:03Z

extern/ipn.php

@@ -41,6 +41,7 @@
 /* Cache the real UF, override it with the SOAP environment */

 $config = CRM_Core_Config::singleton();
+CRM_Utils_Hook::alterIPNData($_REQUEST);


also deprecated path

eileenmcnaughton · 2018-10-16T01:09:22Z

extern/pxIPN.php

@@ -19,6 +19,7 @@
 require_once 'CRM/Core/Config.php';

 $config = CRM_Core_Config::singleton();
+CRM_Utils_Hook::alterIPNData($_REQUEST);


def deprecated - hopefully this processor is disabled on install

eileenmcnaughton · 2018-10-16T01:12:59Z

@seamuslee001 @mattwire I think we can call this hook (once only) from

CRM_Core_Payment::handlePaymentMethod (somewhere after logPayment) - all the new style ipns hit that.

It would be a little trickier for unit testing - but not unresolvable I'm sure

seamuslee001 · 2018-10-16T08:52:45Z

@eileenmcnaughton @mattwire i have made the changes as has been requested, I have also managed to get the unit test updated for handlePaymentMethod. I have put the hook after the main processing so that there isn't any chance of issues being caused from the hook preventing normal processing of the IPN

eileenmcnaughton · 2018-10-18T06:20:44Z

@seamuslee001 so just working through this - it's not really an alterIPNData hook is it - it's basically a listener / post hook

seamuslee001 · 2018-10-18T06:24:18Z

Yeh pretty much @eileenmcnaughton

…

On Thu, 18 Oct 2018 at 5:20 pm, Eileen McNaughton ***@***.***> wrote: @seamuslee001 <https://github.com/seamuslee001> so just working through this - it's not really an alterIPNData hook is it - it's basically a listener / post hook — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12928 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGe_FW1VX-gC9O6S2KpG5rZVIWPYyXkTks5umB3AgaJpZM4XYmsg> .

eileenmcnaughton · 2018-10-18T06:27:15Z

@seamuslee001 should we rename it then? If the logNotification method called a hook then it would achieve the same thing but it doesn't from the looks

seamuslee001 · 2018-10-18T06:29:44Z

@eileenmcnaughton maybe but I think we are still potentially altering data or something? Maybe postIPNProcess instead?

…

On Thu, 18 Oct 2018 at 5:27 pm, Eileen McNaughton ***@***.***> wrote: @seamuslee001 <https://github.com/seamuslee001> should we rename it then? If the logNotification method called a hook then it would achieve the same thing but it doesn't from the looks — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12928 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGe_FVA1Yf9DsqLcB_VeqZXC6wpHoYafks5umB9JgaJpZM4XYmsg> .

eileenmcnaughton · 2018-10-18T06:32:12Z

@seamuslee001 yeah I think that makes sense - we aren't altering the ipn data from what I can see - although we may take a data related action in the hook. I added a comment on gitlab - I think we should give people a chance to process that

…ustom processing of the IPN Data Add in unit test of alterIPNData hook and ensure that it is called in all styles of IPN Notifications Shift hook call to be done in handlePaymentMethod function as per Eileen's comment and remove the hook from deprecated pathways and passbyrefeence in hook Change name to be postIPNProcess to be more explicit about the purpose of the hook

seamuslee001 · 2018-10-18T20:41:51Z

@eileenmcnaughton i have now updated the name of the hook (i'll change the PR title as well shortly) however i think we are now pretty much there right?

seamuslee001 · 2018-10-19T20:28:55Z

Jenkins re test this please

mattwire · 2018-10-19T20:47:21Z

@seamuslee001 I know I'm late to that party on this one.... Agree with it being called postIPNProcess as it's not allowing anything to be altered. However, some IPN classes don't return, they call exit() instead (eg. paymentExpressIPN call exit on bad parameters) - there are also a number of extensions that call exit() in their IPNs for various reasons.

So I wonder if it makes sense to actually call the hook before processing the IPN, though we'd need to be sure any hook didn't block (as, for example, Sagepay requires a response to the IPN request within 1 second). On the other hand we are logging the IPN to systemlog anyway.. I'm just seeing an extension in my head that passes the IPN to another system or something (eg. an accounting system) which might be interested in that IPN even if CiviCRM fails to process it.

seamuslee001 · 2018-10-24T23:59:44Z

@mattwire i think that should be the concern of another hook or at the very least an expansion of hook in terms of when it is processed. I think there could be opportunities to improve this but I don't think it should hold this up @eileenmcnaughton this PR has now been waiting for about a week since we made the change to the name, i note that i don't think anyone in the lab has objected to this and i'm not seeing Matt's comment as a blocker to this PR being merged thoughts?

eileenmcnaughton · 2018-10-25T00:12:50Z

@seamuslee001 OK - I'll merge this - it will not be 100% reliable for the reason @mattwire mentions & I think calling a pre-hook when we log to the system log makes sense - but I think this also solves a need & should not add a mtce burden due to the tes

eileenmcnaughton · 2018-10-25T00:14:37Z

@seamuslee001 can you address documentation for this - I just closed the issue

seamuslee001 · 2018-10-25T00:15:21Z

@eileenmcnaughton yep about to work on it now

civibot bot added the master label Oct 12, 2018

monishdeb reviewed Oct 15, 2018

View reviewed changes

eileenmcnaughton reviewed Oct 16, 2018

View reviewed changes

seamuslee001 force-pushed the dev_financial_33 branch 2 times, most recently from 6c6b5e4 to 8fac610 Compare October 16, 2018 08:56

seamuslee001 force-pushed the dev_financial_33 branch from 8fac610 to 2d39b9c Compare October 18, 2018 20:41

seamuslee001 changed the title ~~dev/financial#33 Add in Hook alterIPNData to allow Extensions to do c…~~ dev/financial#33 Add in Hook postIPNProcess to allow Extensions to do c… Oct 18, 2018

seamuslee001 added the sig:extension support Supports being able to integrate extensions with CiviCRM label Oct 19, 2018

eileenmcnaughton merged commit 65fe53a into civicrm:master Oct 25, 2018

eileenmcnaughton deleted the dev_financial_33 branch October 25, 2018 00:12

alifrumin mentioned this pull request May 14, 2020

WIP add preIPNProcess hook so extensions can edit incoming IPN data before processing #17325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/financial#33 Add in Hook postIPNProcess to allow Extensions to do c… #12928

dev/financial#33 Add in Hook postIPNProcess to allow Extensions to do c… #12928

seamuslee001 commented Oct 12, 2018

civibot bot commented Oct 12, 2018

eileenmcnaughton commented Oct 12, 2018

pradpnayak commented Oct 12, 2018

seamuslee001 commented Oct 14, 2018

monishdeb Oct 15, 2018

highfalutin Oct 15, 2018

eileenmcnaughton Oct 16, 2018

monishdeb commented Oct 15, 2018

eileenmcnaughton Oct 16, 2018

eileenmcnaughton Oct 16, 2018

eileenmcnaughton Oct 16, 2018

eileenmcnaughton Oct 16, 2018

eileenmcnaughton Oct 16, 2018

eileenmcnaughton commented Oct 16, 2018

seamuslee001 commented Oct 16, 2018

eileenmcnaughton commented Oct 18, 2018

seamuslee001 commented Oct 18, 2018 via email

eileenmcnaughton commented Oct 18, 2018

seamuslee001 commented Oct 18, 2018 via email

eileenmcnaughton commented Oct 18, 2018

seamuslee001 commented Oct 18, 2018

seamuslee001 commented Oct 19, 2018

mattwire commented Oct 19, 2018

seamuslee001 commented Oct 24, 2018

eileenmcnaughton commented Oct 25, 2018

eileenmcnaughton commented Oct 25, 2018

seamuslee001 commented Oct 25, 2018

dev/financial#33 Add in Hook postIPNProcess to allow Extensions to do c… #12928

dev/financial#33 Add in Hook postIPNProcess to allow Extensions to do c… #12928

Conversation

seamuslee001 commented Oct 12, 2018

Overview

Before

After

civibot bot commented Oct 12, 2018

eileenmcnaughton commented Oct 12, 2018

pradpnayak commented Oct 12, 2018

seamuslee001 commented Oct 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Oct 16, 2018

seamuslee001 commented Oct 16, 2018

eileenmcnaughton commented Oct 18, 2018

seamuslee001 commented Oct 18, 2018 via email

eileenmcnaughton commented Oct 18, 2018

seamuslee001 commented Oct 18, 2018 via email

eileenmcnaughton commented Oct 18, 2018

seamuslee001 commented Oct 18, 2018

seamuslee001 commented Oct 19, 2018

mattwire commented Oct 19, 2018

seamuslee001 commented Oct 24, 2018

eileenmcnaughton commented Oct 25, 2018

eileenmcnaughton commented Oct 25, 2018

seamuslee001 commented Oct 25, 2018