Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

Fixes MultiPoint::fromWKT() so it works with alternate no nested parenthesis wkt #153

Merged
merged 5 commits into from
Mar 3, 2022
Merged

Conversation

opengisdev
Copy link
Contributor

@opengisdev opengisdev commented Feb 16, 2021

Sorry for not discussing this problem before doing the PR :(

WKT officially supports two syntaxes for MultiPoint, one with nested parenthesis and one without:

MULTIPOINT ((10 40), (40 30), (20 20), (30 10))
MULTIPOINT (10 40, 40 30, 20 20, 30 10)

https://en.wikipedia.org/wiki/Well-known_text_representation_of_geometry

laravel-postgis only supports the nested parenthesis syntax.
Postgis st_astext unfortunately uses the version without nested parenthesis:

select st_astext(st_geomfromtext('MULTIPOINT((1 1),(2 1),(2 2))'))
would return
MULTIPOINT(1 1,2 1,2 2)

This PR fixes the problem by checking if no nested parenthesis syntax is used, and if so, adding the parenthesis and go on with the rest of the function. This could probably be done in a more elegant way but I'm not that good at regex :( There is a test proving this problem in the PR.

The PR also updates the regex to support floating point values, it replaces \d by [+-]?([0-9]+([.][0-9]*)?|[.][0-9]+) as explained here: https://stackoverflow.com/questions/12643009/regular-expression-for-floating-point-numbers but yet again, I'm far from an expert with regular expressions. There is also a test proving the problem.

Thanks for maintaining this great package!

…server st_astext), the testFromWKTWithoutInnerParentesis in tests/Geometries/MultiPointTest shows the error, this could probably be done with the preg_match_all but I'm not that good at regex
@JoanBonnin
Copy link

JoanBonnin commented Aug 31, 2021

Is there any plan to merge this any soon? Also, I'm having some troubles with this specific class, since it's not loading InvalidArgumentException class, so we should also add the use InvalidArgumentException line.

@mstaack

@mstaack
Copy link
Owner

mstaack commented Jan 12, 2022

sure go ahead and add the import statement, i will then merge

@mstaack mstaack merged commit 9a30600 into mstaack:master Mar 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants