Floats in locales that use decimal comma

[thomascorthals]

See issue #262 that was opened 6.5 years ago. I was looking to finally fix this and it turns out it isn't all that hard, but would still be a lot of work. Changes aren't limited to the request builders, other functions that deal with floats (e.g. rangeQuery()) would also need to be updated. There also would have to be additional unit tests for each affected function.

However, in PHP 8.0 all of this is no longer necessary: https://wiki.php.net/rfc/locale_independent_float_to_string. Floats are always cast to string with a decimal point. I've verified that all integration tests succeed for locales that use a decimal comma in PHP 8.0, whereas they fail spectacularly in PHP 7.4.

I have updated the rangeQuery() function locally to make it locale-independent and added a unit test as proof of concept. It succeeds in PHP 7.4 and 8.0. These updates won't cause trouble down the road (or with the workaround in #262), but will become unnecessary when we drop support for PHP 7.4 after 2022-11-28.

That's almost two years out. And there will probably be users stuck on PHP 7.4 for some time after that. Do you think it's still worth the effort to fix this?

[wickedOne]

as there's a workaround available, i'm not sure we should pursue a fix we have to remove in a while...

could you share example code to give a little insight to what you're suggested solution is?

perhahps then it would be possible to come up with something which is less work or at the very least decide whether it's worth the effort.

[wickedOne]

@thomascorthals thanks for adding the poc!

hmz, tough one.
i'm tempted to say to do an ini set in the execute method of the client but that might probably cause a shitstorm of bc problems knowing the amount of events we fire in one request....

personally (but that's just me), knowing php core is about to fix this, i think the best option is to document this issue and workaround properly and leave it at that

[thomascorthals]

See below for the full explanation of how I landed on that implementation. Your comment crossed mine. 😉

[thomascorthals]

i'm tempted to say to do an ini set in the execute method of the client but that might probably cause a shitstorm of bc problems knowing the amount of events we fire in one request....

That also doesn't fix the issue for strings that are "prebuilt" (rangeQuery(), geofilt() …) before they are passed on to a request builder.

[thomascorthals]

I've opened PR #911 as a proof-of-concept and marked it as a draft.

The solution is simple enough: json_encode turns the float into a string representation that always uses a dot as a decimal separator and has an arbitrary number of decimals. (Both sprintf with an %F specifier and number_format can do the dot, but not the arbitrary number of decimals.)

However, the very next function I encountered already poses a problem.

solarium/src/Core/Query/Helper.php

Line 224 in 3340357

public function geofilt(string $field, string $pointX, string $pointY, string $distance, bool $dereferenced = false): string

It says it takes a string, but without strict typing it also happily takes a float. In fact, that's how we unit test it and how we tell to use it in the docs.

solarium/tests/Core/Query/HelperTest.php

Lines 245 to 251 in 3340357

	public function testGeofilt()
	{
	$this->assertSame(
	'{!geofilt pt=45.15,-93.85 sfield=store d=5}',
	$this->helper->geofilt('store', 45.15, -93.85, 5)
	);
	}

solarium/examples/2.1.6-helper-functions.php

Lines 16 to 17 in 3340357

	// add a filterquery to find products in a range of 5km, using the helper to generate the 'geofilt' filter
	$query->createFilterQuery('region')->setQuery($helper->geofilt('store',45.15, -93.85, 5));

We'd have to check the type and only json_encode on actual floats lest we get "45.15" if it's passed as a string. Changing the typehint and declaring strict_types=1 would break BC for everyone who does pass it as a string.

I didn't try and find out how many functions are susceptible to this.

Another major issue is with testing these changes. A correct test requires a locale that uses a decimal comma. If the 'nl_NL.UTF-8' locale isn't available for instance, setlocale(LC_NUMERIC, 'nl_NL.UTF-8') will result in the 'C' locale still being used and the unit tests and integration tests will happily pass. That means every unit test for different locales needs to check for the locale actually being set and must be marked as skipped if not if we don't want to provide a false sense of correctness to contributors. When I use nektos/act locally, their Docker container nektos/act-environments-ubuntu:18.04 doesn't have any of the locales I'm trying to set. When I push to GitHub, the test is skipped because it doesn't have any of these locales either. Definitely a valid concern as it seems all Docker containers "out there" come without preinstalled locales.

$ locale -a
C
C.utf8
POSIX

You'll also have to remember to set the locale back to 'C' in the tearDown() of every TestCase that has this kind of tests. If you do it at the end of the individual tests, a failure in one of those will leave it at the decimal comma locale and cause myriad failures in the following tests.

You could do it in setUp()/tearDown() and not have separate locale specific test methods. But then entire TestCases would have to be skipped if the locale can't be set. Or not skipped and you'd never be aware that you aren't testing this at all. (Maybe it could be done like this for the integration tests …)

You could group all locale related tests in a separate TestCase specifically for this stuff. But then it's not obvious for contributors that these tests might have to be extended as well because they're not in the place you'd expect them to be. Code coverage won't tell you that you've missed something because the code lines will have been covered in the regular test methods. That's why I believe that each locale specific test should follow its locale agnostic counterpart immediately in the same TestCase.

[wickedOne]

We'd have to check the type and only json_encode on actual floats lest we get "45.15" if it's passed as a string. Changing the typehint and declaring strict_types=1 would break BC for everyone who does pass it as a string.

agreed. might be interesting for a 7.0 release to set strict_types=1 but then again we should get rid of the ConfigurableInterface package wide as there's no control over types when you set things with a config array (options resolver might be a consideration tho).

You'll also have to remember to set the locale back to 'C' in the tearDown() of every TestCase that has this kind of tests. If you do it at the end of the individual tests, a failure in one of those will leave it at the decimal comma locale and cause myriad failures in the following tests.

personally i don't think it should be part of the test itself. we might want to look into whether it's possible to use a github action which changes the php.ini for each test run, but i'm not sure testing all possible locales is the way to go

this sounds like a lot of work for something with a workaround and a definite fix ahead so i'll stick to

personally (but that's just me), knowing php core is about to fix this, i think the best option is to document this issue and workaround properly and leave it at that

[thomascorthals]

we might want to look into whether it's possible to use a github action which changes the php.ini for each test run

That'll work when it's fixed across the code base. But I was thinking about maybe introducing it gradually. Request builder first, helper functions after, plugins last. It wouldn't really make a difference for the users until we proclaim "locale support is here" and give us the opportunity for back-and-forth in the meantime.

i'm not sure testing all possible locales is the way to go

Just one should suffice as long as it doesn't use a decimal dot. The three locale thing I did in the POC was just to demonstrate how you absolutely can't rely on setlocale() for testing this across environments, both in contributors' local setup and in a workflow on GitHub.

personally (but that's just me), knowing php core is about to fix this, i think the best option is to document this issue and workaround properly and leave it at that

I agree. The amount of work that will go into this could be better spent on improvements that aren't obsolete in 2 years.

[thomascorthals]

I was a bit thrown off because it always does this when I run tests locally:

[Run Tests/PHP 7.4, Solr 8 server]   | ==> Setup Extensions
[Run Tests/PHP 7.4, Solr 8 server]   | ✓ dom Enabled
[Run Tests/PHP 7.4, Solr 8 server]   | ✓ curl Enabled
[Run Tests/PHP 7.4, Solr 8 server]   | ✓ libxml Enabled
[Run Tests/PHP 7.4, Solr 8 server]   | ✓ mbstring Enabled
[Run Tests/PHP 7.4, Solr 8 server]   | ✓ zip Enabled
[Run Tests/PHP 7.4, Solr 8 server]   | ✗ intl Could not install intl on PHP 7.4.2
[Run Tests/PHP 7.4, Solr 8 server]   | ✓ iconv Enabled
[Run Tests/PHP 7.4, Solr 8 server]   | ✓ json Enabled
[Run Tests/PHP 7.4, Solr 8 server]   | ✓ simplexml Enabled

Actually, the intl extension isn't needed at all for setlocale(). I've adjusted the POC accordingly.

This doesn't change the fact that the Docker containers out there for testing don't have any locales preinstalled.

$ locale -a
C
C.utf8
POSIX

[thomascorthals]

I've updated my comment above as well to remove the inaccurate information.