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

CRM-21365 Fix Currency #11329

Merged
merged 2 commits into from
Nov 28, 2017
Merged

CRM-21365 Fix Currency #11329

merged 2 commits into from
Nov 28, 2017

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 26, 2017

Overview

Improve handling of currency

Comments

This is proposed as an alternative to #11210 and #11325


Change-Id: I63f3f5445b39b9bd6068835ae61c3e488177ce75
…is passed to processor

Change-Id: I59a5cd16499e89fbaa54594071fd396de81dfb82
@mattwire
Copy link
Contributor

@eileenmcnaughton Yes, this is a better "fix". Does the getCurrency function have to be in CRM_Core_Form though? It couldn't go somewhere like CRM_Utils_Money or something?

@eileenmcnaughton
Copy link
Contributor Author

I think it's appropriate to the form class because it's about how the form holds that information.

@colemanw
Copy link
Member

Yes to form class but I agree CRM_Core_Form is not a great place for something this specific. What about putting it in CRM_Core_Payment_ProcessorForm.

@eileenmcnaughton
Copy link
Contributor Author

The forms we care about (event forms, contribution pages) don't inherit from that class. (I did suggest about 3 years ago creating a current parent but that was not taken up then & we now have a bunch of functions in that top level form class that are there so those 2 form-sets can inherit from them.)

Once 5.3 is gone we can look at moving all those functions (there are maybe half a dozen) into a trait class.

@mattwire
Copy link
Contributor

@eileenmcnaughton I'm happy with your explanation for location of that function :-)

@colemanw
Copy link
Member

Sorry I still don't get it. This diff looks to me like it would produce a fatal error. Because you're adding a function to CRM_Core_Form but calling it from CRM_Core_Payment_ProcessorForm which does not extend CRM_Core_Form so wouldn't that crash?

@eileenmcnaughton
Copy link
Contributor Author

In that function we call $form->getCurrency();

$form will be an instance of

  • @param CRM_Contribute_Form_Contribution_Main|CRM_Event_Form_Registration_Register|CRM_Financial_Form_Payment $form

@colemanw
Copy link
Member

Ah, I get it now. Thanks @eileenmcnaughton

@colemanw colemanw merged commit 923c18f into civicrm:master Nov 28, 2017
@colemanw colemanw changed the title Currency CRM-21365 Fix Currency Nov 28, 2017
@eileenmcnaughton eileenmcnaughton deleted the currency branch November 28, 2017 02:21
@eileenmcnaughton
Copy link
Contributor Author

@GinkgoFJG can you confirm this works for your bug (looking for the pr now)

sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants