Location module stores lat/lng incorrectly as integers in the database

[jessedobbelaere]

Bug Report

Q	A
Critical Bug	no
Version	5.x.x

Problem description

When I use the Location module in the latest Fork CMS, instead of storing 51.0624 and 3.74855 as longitude/latitude in the database, it stores 51 and 3. This gives a totally different location.

Steps to reproduce

Enter an address in the Location module. Save it. The backend will do Geolocation using the Google Geo Coding API. It will find the lat/lng coordinates. When Spoon Database stores the record, it casts to integers.

Expected behavior / Proposed solution

Quick n dirty solution: typecast the float to strings before inserting the item in the Model.php. Spoon Database inserts those correctly as 51.0624 and 3.74855 instead of 51 and 3.

Actual solution: in Spoon Library's database.php there's some logic that binds values to the statement:

https://github.com/forkcms/library/blob/master/spoon/database/database.php#L1209-L1213

There's a bug in the getType($value) function: https://github.com/forkcms/library/blob/master/spoon/database/database.php#L713-L718

It considers floats to be of type PDO::PARAM_INT which should not be the case as it casts the float to an int and inserts an integer in the db.

There is no PDO::PARAM for floats and decimals so different sources suggest to use PDO::PARAM_STR. Tl;dr probably a oneliner fix in Spoon Library?

[carakas]

Any idea how it comes it suddenly started acting up?

[jessedobbelaere]

No idea as I haven't used the Location module in a long time... But I see Jeroen refactored it a bit in #2525 in 2018 so maybe it used to be a string, now it's a float being sent to PDO?

Spoon Library should use PDO::PARAM_STR anyways in case of a float, thats a bug :D (https://stackoverflow.com/a/9542340/1409047). Should I just make a PR towards Spoon Library?

[jonasdekeukelaere]

Can confirm, had this in one project, but can't remember when, I did cast it to strings because no time to figure out what the issue was.

[jessedobbelaere]

This should do it ⬆️ https://github.com/forkcms/library/pull/74/files

[ohvitorino]

We should add some tests to cover this situation.

[carakas]

I think in the restructure_core there are already tests for the new location module

[ohvitorino]

I think in the restructure_core there are already tests for the new location module

That's correct. I've just tested it.

[carakas]

fixed in #3001