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

Remove deprecated use of format money #20697

Merged
merged 1 commit into from
Jun 26, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Deprecation errors are being logged due to passing the html for min_amount to the money::format funciton. This is a read only field

Removing that call results in a slight loss of formatting but it's broken before AND after on sites with non-US decimarl separators and the field feels obscure enough and is not an input field so my money (guffaw) says no-one will miss the currency symbol.

It's a bit of work to fix it so it DOES format correctly as js is involved & it has never been reported

Before

A deprecation notice is logged due to passing html to Money;;format()

The site default currency is shown but the thousand separators are incorrect.

image

After

The currency symbol has been sacrificed to get rid of the deprecation notice - the thousand separators are still incorrect

image

Technical Details

Comments

This is logging deprecation errors. It doesn't work quite right before or after but
I suspect no-one much cares as it hasn't been raised before now
@civibot
Copy link

civibot bot commented Jun 23, 2021

(Standard links)

@civibot civibot bot added the master label Jun 23, 2021
@eileenmcnaughton
Copy link
Contributor Author

test this please

@demeritcowboy
Copy link
Contributor

Agree it's broken before anyway. And there's some precedent for removing these crmMoney html calls at the expense (guffaw 2) of a missing currency symbol pending a better fix.

@demeritcowboy demeritcowboy merged commit 2b145b8 into civicrm:master Jun 26, 2021
@eileenmcnaughton eileenmcnaughton deleted the prem branch October 17, 2021 23:34
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.

2 participants