Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doPayment params - munge country / country_id into billingCountry #984

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

ufundo
Copy link
Contributor

@ufundo ufundo commented Jul 10, 2024

Overview

Fix deprecation warning in PropertyBag when munging billing country parameter.

(Using Stripe Checkout with CiviCRM core 5.72 but it looks like it would affect other payment processors on 5.71+ (since this commit civicrm/civicrm-core@64abef2).

Before

Logs show

User deprecated function: PropertyBag::setBillingCountry input warnings (may be errors in future): Not ISO 3166-1 alpha-2 code. Given input matched a country name, assuming it was that. Input: "United Kingdom" was munged to: "GB" Caller: Civi\Payment\PropertyBag::offsetSet in CRM_Core_Error::deprecatedWarning() (line 1129 of /var/www/html/vendor/civicrm/civicrm-core/CRM/Core/Error.php)
#0 /var/www/html/web/core/includes/bootstrap.inc(166): _drupal_error_handler_real()
#1 [internal function]: _drupal_error_handler()
#2 /var/www/html/vendor/civicrm/civicrm-core/CRM/Core/Error.php(1129): trigger_error()
#3 /var/www/html/vendor/civicrm/civicrm-core/Civi/Payment/PropertyBag.php(715): CRM_Core_Error::deprecatedWarning()
#4 /var/www/html/vendor/civicrm/civicrm-core/Civi/Payment/PropertyBag.php(260): Civi\Payment\PropertyBag->setBillingCountry()
#5 /var/www/html/vendor/civicrm/civicrm-core/Civi/Payment/PropertyBag.php(385): Civi\Payment\PropertyBag->offsetSet()
#6 /var/www/html/vendor/civicrm/civicrm-core/Civi/Payment/PropertyBag.php(147): Civi\Payment\PropertyBag->mergeLegacyInputParams()
#7 /var/www/html/web/modules/civicrm/custom/com.drastikbydesign.stripe/CRM/Core/Payment/StripeCheckout.php(144): Civi\Payment\PropertyBag::cast()
#8 /var/www/html/web/modules/contrib/webform_civicrm/src/WebformCivicrmConfirmForm.php(51): CRM_Core_Payment_StripeCheckout->doPayment()
#9 /var/www/html/web/modules/contrib/webform_civicrm/src/Plugin/WebformHandler/CivicrmWebformHandler.php(162): Drupal\webform_civicrm\WebformCivicrmConfirmForm->doPayment()
...

After

Hopefully it works 🤞
(Still testing)

@KarinG
Copy link
Collaborator

KarinG commented Jul 10, 2024

I've kicked off the tests - which run on 4x Drupal x CiviCRM versions and include Credit Card and ACHEFT iATS tests so this may be Stripe specific.

@ufundo
Copy link
Contributor Author

ufundo commented Oct 25, 2024

The crash I was seeing turned out to be something else. But I think think something like this patch makes sense to fix the deprecation warnings.

I see that some of the integration tests fail - but I can't seem to see any logs or anything for them. Are they available? Sorry not that familiar with Github Actions

@ufundo ufundo marked this pull request as ready for review October 25, 2024 09:33
@mattwire
Copy link
Collaborator

Is webform providing inconsistent values for country? The issue with passing something like "United Kingdom" is that it changes depending on the language etc. so is unreliable/inconsistent. However, the ISO code ("GB") is always correct.

But it looks like webform is passing in either "United Kingdom" or (id) 1226. Shouldn't we be able to reliably get either the ISO code or the ID directly from the form?

Those two questions above don't have to block this PR, just like to get to the bottom of this.

@ufundo
Copy link
Contributor Author

ufundo commented Oct 25, 2024

Shouldn't we be able to reliably get either the ISO code or the ID directly from the form?

I don't have much context, but it seemed like it was just getting the country id / name from the contact record, as fetched using API3?

@KarinG
Copy link
Collaborator

KarinG commented Oct 25, 2024

I've made a small syntax edit in your fork so that it kicks off the tests against the current test matrix. I think this is ok except I'll want to make sure all existing payment processing tests pass.

@KarinG KarinG merged commit 069071b into colemanw:6.x Oct 25, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants