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

[VUFIND-1618] Eliminate setlocale() call from Bootstrapper. #3526

Merged
merged 4 commits into from Mar 21, 2024

Conversation

demiankatz
Copy link
Member

@demiankatz demiankatz commented Mar 18, 2024

Use of setlocale is being discouraged due to changes introduced in PHP 8. The only explicit user of PHP locale functions in VuFind is the CurrencyFormatter service, which relied on a setlocale call in the Bootstrapper to establish appropriate formatting behavior. This PR eliminates the Bootstrapper call and passes locale settings directly to the CurrencyFormatter while maintaining the legacy locale behavior for backward compatibility.

TODO

  • Run full test suite
  • Update changelog when merging (change to Bootstrapper)
  • Resolve VUFIND-1618 when merging

@demiankatz demiankatz added this to the 10.0 milestone Mar 18, 2024
@demiankatz demiankatz added the architecture pull requests that involve significant refactoring / architectural changes label Mar 18, 2024
@demiankatz
Copy link
Member Author

All Mink tests are passing.

Copy link
Contributor

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the changes look fine to me, I'm seeing dollar sign instead of euro sign in fines with locale set to fi_FI (defaultCurrency not set), which is not expected and doesn't happen with the dev branch. I also tried fi_FI.UTF-8 and fi_FI.UTF8 to no avail.

@demiankatz
Copy link
Member Author

@EreMaijala, I think in your case, the dev branch may only be working by accident.

When I try using the fi_FI locale in my test environment, I get dollar signs everywhere. However, in the dev branch, the numbers are formatted the American way, and in this branch, the numbers are formatted the Finnish way.

I did some debugging and discovered that in the dev branch, the setlocale call doesn't seem to be working correctly with fi_FI, and so the default locale value of C is getting used... and that's why the number formatting is wrong in the dev branch. I suspect the root cause of this may be that I haven't installed the fi_FI locale in my environment, but I haven't dug in deeper to confirm.

I wonder if the same thing is happening on your end, but when the locale of "C" is used, it defaults to Euros and Finnish formatting because of the way your operating system is set up... but when you explicitly specify fi_FI, you get the same behavior that I am seeing when I explicitly specify that value. I'm not sure why that would use dollar signs, but maybe PHP's support is incomplete or some additional data needs to be loaded somewhere.

Does that make any sense to you? And most importantly, if you set your locale to "C" in config.ini, does that get you correct behavior in this branch?

In any case, regardless of what you find from that test, I'm not entirely sure what the best solution is to the problem. I suspect there may not be an entirely simple answer.

@EreMaijala
Copy link
Contributor

@demiankatz There's certainly a difference between C and fi_FI, but the currency sign is $ with both:
C: $2.50
fi_FI: 2,50 $

@EreMaijala
Copy link
Contributor

@demiankatz As far as I can see, the problem is that the localeconv() call in CurrencyFormatter uses the locale set with setlocale, but since it's no longer set here, it will return information according to PHP's default locale.

@EreMaijala
Copy link
Contributor

@demiankatz After a bit of digging, this should work instead of localeconv: $this->formatter->getTextAttribute(NumberFormatter::CURRENCY_CODE);

@demiankatz
Copy link
Member Author

Thanks, @EreMaijala, and sorry for not noticing that in the first place. Your proposed fix seems to solve everything.

Copy link
Contributor

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, everything works properly now. :) But shouldn't CurrencyFormatterTest also work without setlocale in setUp method?

@demiankatz
Copy link
Member Author

Thanks, everything works properly now. :) But shouldn't CurrencyFormatterTest also work without setlocale in setUp method?

I think that is still required. The helper is now less dependent on the locale setting, but for backward compatibility, some optional dependency remains. If no locale is provided to the helper, it uses setlocale(LC_MONETARY, 0); to fetch the current system locale. So the test will be region-dependent if we remove the setup call; I don't think we want to do that.

Copy link
Contributor

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, of course. Thanks!

@demiankatz demiankatz merged commit dbfec47 into vufind-org:dev Mar 21, 2024
7 checks passed
@demiankatz demiankatz deleted the vufind-1618 branch March 21, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture pull requests that involve significant refactoring / architectural changes
Projects
None yet
2 participants