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

Port https://github.com/civicrm/civicrm-core/pull/20040 #20098

Merged
merged 3 commits into from
Apr 19, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

eileenmcnaughton and others added 2 commits April 19, 2021 13:21
We've said this is wrong before but it has survived until now because it seemed scarier to change it. However,
now the reverse seems true.

The value in amount should always be machine friendly and there are no known processors
that expect locale specific formatting.

On the other hand the format() function is intended to prepare money for DISPLAY which
is not what is going on here
… to return appropriate amount in that case otherwise just return amount
@civibot
Copy link

civibot bot commented Apr 19, 2021

(Standard links)

@civibot civibot bot added the 5.36 label Apr 19, 2021
@seamuslee001
Copy link
Contributor

@eileenmcnaughton I think your missing the additional commit with the formatting?

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 I think if we port this we can leave the issue open but remove the regression tag (as there will be no known instances but the visibility might be helpful)

@seamuslee001
Copy link
Contributor

@eileenmcnaughton I think you need #20074 as well right?

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 yes - added

@eileenmcnaughton eileenmcnaughton merged commit ddb6be2 into civicrm:5.36 Apr 19, 2021
@eileenmcnaughton eileenmcnaughton deleted the 536 branch April 19, 2021 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants