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

Fix SRID not being optional for expression in GeometryCast #124

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

nickknissen
Copy link
Contributor

@nickknissen nickknissen commented Jul 2, 2024

The current implementation of GeometryCast requires SRID to present in the expression when setting an model attribute with DB:raw

The given case:

$place = Place::factory()->create(['point' => DB::raw("ST_GeomFromText('POINT(12.38057 55.73406)')")]);
$place->isDirty()

This will throw the following error because the regex looks for a SRID in the expressionValue

  Undefined array key 1

  at src\GeometryCast.php:86
     82▕         $expressionValue = $expression->getValue($grammar);
     83▕ 
     84▕         preg_match('/ST_GeomFromText\(\'(.+)\', .+(, .+)?\)/', (string) $expressionValue, $match);
     85▕ 
  ➜ 86▕         return $match[1];
     87▕     }
     88▕ 
     89▕     private function extractSridFromExpression(ExpressionContract $expression, Connection $connection): int
     90▕     {

The ST_GeomFromText specifications allows SRID not to be specified as seen in mysql and PostGis documentation:

I have adjusted GeometryCast regex treat SRID as an optional parameter and return 0 when SRID is not defined.

@nickknissen nickknissen marked this pull request as draft July 2, 2024 13:17
@MatanYadaev
Copy link
Owner

MatanYadaev commented Jul 2, 2024

@nickknissen Hi, thanks for the PR, I'll review it later.
What issue does it solve? Can you add tests to indicate the issue is solved?

@nickknissen nickknissen force-pushed the fix-handle-optional-srid branch 2 times, most recently from 6b8e95e to b76c6c2 Compare July 3, 2024 05:17
@nickknissen
Copy link
Contributor Author

@MatanYadaev
Currently ST_GeomFromText('POINT(9.581389 55.724167)') is not supported as an expression value. SRID is required.

I will add some tests.

@nickknissen nickknissen force-pushed the fix-handle-optional-srid branch 2 times, most recently from 9d41384 to e557924 Compare July 29, 2024 12:50
@nickknissen
Copy link
Contributor Author

@MatanYadaev I have updated the description with some more information about the error and added tests

@nickknissen nickknissen marked this pull request as ready for review July 29, 2024 13:01
tests/TestCase.php Outdated Show resolved Hide resolved
src/GeometryCast.php Outdated Show resolved Hide resolved
@nickknissen nickknissen force-pushed the fix-handle-optional-srid branch 3 times, most recently from 6ac3c7d to 0b8bdc5 Compare July 30, 2024 05:30
@MatanYadaev
Copy link
Owner

Can you please resolve conflicts and fix CI?

@nickknissen
Copy link
Contributor Author

Can you please resolve conflicts and fix CI?

It should be up to date now.

@MatanYadaev MatanYadaev merged commit d11470b into MatanYadaev:master Aug 7, 2024
44 checks passed
@MatanYadaev
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants