Fix addVars to allow adding vars to billing-block

[mattwire]

Overview

@eileenmcnaughton I think this fixes the issues with paypal checkout / omnipay that we've encountered with js errors.

Specifically: eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor@b9d5719

CRM_Core_Resources::singleton()->addVars('omnipay', $jsVariables, 'billing-block'); or equivalent \Civi::resources()->addVars('omnipay', $jsVariables, 'billing-block'); does not work when adding via CRM_Core_Payment_XX::buildForm() because it defines CRM instead of extending it when the payment processor is the default on the page. It does not cause problems when the processor adding vars is not the default.

This is because of load order - the billing-block gets "rendered" in PHP before html-header which doesn't work when the form actually loads because the js on the form then tries to extend CRM before it is defined and triggers a chain of js errors.

Lost yet?

Before

Cannot use addVars to add variables to billing-block when payment processor is default on the page.

After

Can use addVars to add variables to billingblock in all situations without issues.

Technical Details

Explained above I think!

Comments

The tests should help show what is expected to be rendered.

[civibot]

(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]

@mattwire style issue

[colemanw]

Code looks good to me.

[totten]

@mattwire - So @eileenmcnaughton gave this testing suggestion for the original problem/motivation. I'm not really able to reproduce the problem/solution.

Steps:

1. Install omnipaymultiprocessor 3.13 (6e59b7116e84b22908111f6872ca8e45949bb970). This has the recent revert.
2. Create a dummy payment processor and PayPal Checkout processor. Setup a Paypal developer sandbox application and copy the clientId and secret.
3. Alternate the configuration of the first contribution page in the demo data-set -- e.g. try with:
   • (a) PayPal Checkout as the only/solo processor
   • (b) Dummy processor as default. PayPal Checkout as secondary.
4. Alternate the versions of civicrm-core, e.g.
   • Use this PR/branch (ie 99c6336)
   • Use the commit preceding this PR/branch (ie e1c19ea)
   • Use master-res-coll (18247) without trying to forward-port of this change
   • Use master-res-coll (18247) with the (unverified) forward-port of this change
5. Open the contribution page (/civicrm/contribute/transact?reset=1&id=1)

Based on the two configurations of the contribution-page, and based on the 4 revisions of code, that gives 8 different test-scenarios. I tested in Firefox 78.0.1 (macOS). In all 8 cases, I observed the same behavior:

• The PayPal buttons rendered at the expected time+place. (This seems notable -- when I didn't have proper credentials, it was unable to render the button. The appearance of button seems to demonstrate the clientId is going somewhere needful.)
• In the browser JS console, I could inspect the value of CRM.vars.omnipay. It seemed plausible.
• When the PayPal buttons rendered, the browser JS console showed a large number of CORS errors. I guess they're all about attempts at remote-logging. I never see any errors about CRM.vars.omnipay, CRM.vars, or CRM.
  

Going a step further, I used the browser console to get a fully-formed curl command (w/session ID, etc) for these two interesting requests:

# The main HTML page (as rendered during the use-case with PayPal as secondary)
curl 'http://dmaster.bknix:8001/civicrm/contribute/transact?reset=1&id=1' -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0) Gecko/20100101 Firefox/78.0' -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8' -H 'Accept-Language: en-US,en;q=0.5' --compressed -H 'Connection: keep-alive' -H "$COOKIE" -H 'Upgrade-Insecure-Requests: 1' -H 'Cache-Control: max-age=0'

# The XHR request for openingg the PayPal subform (as loaded for PayPal as secondary)
curl 'http://dmaster.bknix:8001/civicrm/payment/form?formName=Main&currency=USD&&is_back_office=&id=1&processor_id=5&cid=202&payment_instrument_id=undefined&snippet=json' -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0) Gecko/20100101 Firefox/78.0' -H 'Accept: application/json, text/javascript, */*; q=0.01' -H 'Accept-Language: en-US,en;q=0.5' --compressed -H 'X-Requested-With: XMLHttpRequest' -H 'Connection: keep-alive' -H 'Referer: http://dmaster.bknix:8001/civicrm/contribute/transact?reset=1&id=1' -H "$COOKIE"

The content of these pages was identical in 99c6336 (this PR/branch) and e1c19ea (ie the immediate predecessor).

So tldr: can't see how to reproduce

[mattwire]

@totten Ok, it's a bit tricky to reproduce but hopefully obvious once you realise what is going on. You won't be able to reproduce directly with the PR for omnipay or using Stripe because they're both using workarounds to avoid the problem (as they need to until we have something like this PR in core).

For Stripe this is the bit of code that would change, and that contains the workaround:
https://lab.civicrm.org/extensions/stripe/-/blob/master/CRM/Core/Payment/Stripe.php#L384-386

    \Civi::resources()->addVars(E::SHORT_NAME, $jsVars);
    $form->assign('stripeJSVars', $jsVars);

1. CRM_Core_Payment::buildForm() is run every time the processor loads and needs to assign some javascript variables to CRM.vars.
2. In the code above, it also assigns it to a tpl which re-assigns the variables if not already set: https://lab.civicrm.org/extensions/stripe/-/blob/master/templates/CRM/Core/Payment/Stripe/Card.tpl#L11-22

addVars defaults to no region which means it will either get put in ajax-snippet or html-header. Neither of those are useful for a payment processor loading into the billing-block region.
In the case of html-header if the payment processor is the default it will get it's vars loaded into the html-header on the page but they won't then get updated if you switch to another processor (eg. if you have two stripe processors on the form). Stripe works around that limitation by destroying CRM.vars.stripe when switching to a different payment processor and then re-loading the new vars via the card.tpl template.

With this PR, or a similar one based on the work you are doing for bundles, you can specify billing-block directly as the region for addVars() and the payment processor (eg. Stripe) will load the js vars directly into the billing-block each time. Without this PR it will still load into the billing-block but the code generated by CRM_Core_Region is wrong as, because of the load order of various functions, tries to define CRM.vars instead of extending it and that crashes javascript in the browser because it (Common.js etc.) sees a $.extend(CRM.. before it sees var CRM = .

[totten]

Great, thanks for the description @mattwire.

So to r-run, I created a dummy extension with this code:

function delme_civicrm_config(&$config) {
  _delme_civix_civicrm_config($config);
  static $run = 0;
  if ($run) return;
  $run = 1;

  CRM_Core_Resources::singleton()
    ->addVars('delme', ['time' => CRM_Utils_Time::getTime()], 'billing-block');
  CRM_Core_Region::instance('billing-block')->add([
    'script' => 'cj("#billing-payment-block").prepend("time="+CRM.vars.delme.time);'
  ]);
}

Then I added two dummy payment processors on a contribution page. When switching between the payment processors, it should redraw the billing-block with an injected timestamp:

This worked as expected:

• In both Fix addVars to allow adding vars to billing-block #18141 and Add higher-level support for "bundles" and "collections" of resources #18247 (with forward-port), the block updates whenever you switch payment processors.
• In prior revisions (with neither Fix addVars to allow adding vars to billing-block #18141 nor the forward-port), the block fails to update, and the JS console shows errors.

[eileenmcnaughton]

Ohh that's aa cool way to test - I wish we could add that into CI but then it would be testing js + php together which seems hard

[totten]

@colemanw @mattwire I know this is a bit of a pre-existing issue, but that conditional (if... $region === 'html-header' || ...) is growing a bit magical. Would it be possible to either:

• Have one JS fragment that's more clever ("if CRM exists, then merge data -- else initialize anew.").
• Simplify the rule: the only time to use var CRM=... is in html-header. (If you use it at any other time, whether AJAX or non-AJAX, won't it produce problems?)

         $loader = ($region === 'html-header')
           ? 'var CRM = %s;' : 'CRM.$.extend(true, CRM, %s);';
         $js = sprintf($loader, json_encode($this->getSettings($region)));
         $html .= sprintf("<script type=\"text/javascript\">\n%s\n</script>\n", $js);

[eileenmcnaughton]

@totten I don't think 2 would work - if a processor needs to inject code it won't necessarily have the chance to do so in html-header - because processor forms are often loaded when the processor is selected, or by webform

[mattwire]

@totten I've thought about getting rid of || !isAjaxMode() because I also think it is superfluous. That might be a good goal to do before making other changes per this #18141, #18247.

In drupal 7 the html-header is always rendered if we are on a civicrm-ified page (eg. drupal_webform). But the renderSetting and definition of var CRM is not always called. It does get called if you specify a payment processor on the webform because CRM.config.creditCardTypes gets added which triggers the render settings code.

See: https://github.com/civicrm/civicrm-drupal/blob/7.x-master/civicrm.module#L156

As I understand it, if a payment processor is going to be loaded we will have html-header, it's just that html-header is static across payment processor changes so it can't be re-used by the processor (as you've cleverly reproduced in the timestamp example).

[colemanw]

I agree we should remove the magic region condition here. How about moving the condition to js:

#18262

[mattwire]

I need to verify if this is superseded by #18262

[eileenmcnaughton]

cool - I'll close & if still required you can rebase & re-open