feat(#458): ensure Doctrine returns PostgreSQL spatial data in text format (EWKT) instead of binary format (EWKB) #462
Conversation
…ormat (EWKT) instead of binary format (EWKB)
The convertToPHPValueSQL() method signature differs between DBAL versions: - DBAL 2.x: convertToPHPValueSQL($sqlExpr, mixed $platform) - DBAL 3.x+: convertToPHPValueSQL(string $sqlExpr, AbstractPlatform $platform) Removing the AbstractPlatform type hint from the $platform parameter maintains compatibility with both versions while satisfying PHPStan's contravariance requirements.
WalkthroughIntroduces a new abstract base class Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Moderate complexity stems from new abstract base class logic, ORM persistence trait with schema and entity management, multiple test method implementations, and integration across spatial types. Changes follow clear patterns and maintain consistent structure, reducing cognitive load despite spanning multiple files and test concerns. Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-08-24T16:52:32.488ZApplied to files:
🧬 Code graph analysis (1)src/MartinGeorgiev/Doctrine/DBAL/Types/BaseSpatialType.php (1)
🪛 PHPMD (2.15.0)src/MartinGeorgiev/Doctrine/DBAL/Types/BaseSpatialType.php34-34: Avoid unused parameters such as '$platform'. (undefined) (UnusedFormalParameter) 🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseSpatialType.php (2)
33-41: Simplify EWKT generation using ST_AsEWKT (fewer calls, clearer intent)PostGIS can emit EWKT directly; consider replacing the CASE with ST_AsEWKT to avoid repeating ST_SRID/ST_AsText and to keep behavior identical when SRID=0 (no prefix).
Apply:
- public function convertToPHPValueSQL($sqlExpr, AbstractPlatform $platform): string - { - return \sprintf( - "CASE WHEN ST_SRID(%s) = 0 THEN ST_AsText(%s) ELSE 'SRID=' || ST_SRID(%s) || ';' || ST_AsText(%s) END", - $sqlExpr, - $sqlExpr, - $sqlExpr, - $sqlExpr - ); - } + public function convertToPHPValueSQL($sqlExpr, AbstractPlatform $platform): string + { + // ST_AsEWKT includes SRID prefix when present; cast covers GEOGRAPHY too. + return \sprintf('ST_AsEWKT(%s::geometry)', $sqlExpr); + }
33-33: Silence “unused parameter” noise for $platformDBAL requires the signature; add a suppression to keep linters quiet.
- public function convertToPHPValueSQL($sqlExpr, AbstractPlatform $platform): string + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ + public function convertToPHPValueSQL($sqlExpr, AbstractPlatform $platform): string {tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/WktAssertionTrait.php (1)
15-45: Don’t ignore SRID mismatches when both sides have SRIDCurrently SRID is always stripped, which can mask “4326 vs 3857” errors. Compare SRIDs when both exist, then compare normalized WKT bodies.
Apply:
- protected function assertWktEquals(WktSpatialData $expected, WktSpatialData $actual): void - { - $this->assertEquals( - $this->normalizeWkt((string) $expected), - $this->normalizeWkt((string) $actual), - 'WKT spatial data mismatch' - ); - } + protected function assertWktEquals(WktSpatialData $expected, WktSpatialData $actual): void + { + [$expSrid, $expBody] = $this->splitSrid((string) $expected); + [$actSrid, $actBody] = $this->splitSrid((string) $actual); + + if ($expSrid !== null && $actSrid !== null) { + $this->assertSame($expSrid, $actSrid, 'SRID mismatch in EWKT'); + } + + $this->assertEquals( + $this->normalizeWkt($expBody), + $this->normalizeWkt($actBody), + 'WKT spatial data mismatch' + ); + } @@ - private function normalizeWkt(string $wkt): string + private function normalizeWkt(string $wkt): string { - // Remove SRID prefix for comparison if present - if (\str_starts_with($wkt, 'SRID=')) { - $parts = \explode(';', $wkt, 2); - $wkt = $parts[1] ?? $wkt; - } - // Normalize whitespace: PostgreSQL returns WKT without spaces after commas $normalized = \preg_replace('/,\s+/', ',', $wkt); if ($normalized === null) { return $wkt; } // Normalize decimal numbers: PostgreSQL may return -9.0 as -9 - $result = \preg_replace('/(\d)\.0(?=\D|$)/', '$1', $normalized); + $result = \preg_replace('/(\d)\.0(?=\D|$)/', '$1', $normalized); // keep current scope (.0 only) return $result ?? $normalized; } + + /** + * @return array{0:int|null,1:string} [srid|null, wktWithoutSrid] + */ + private function splitSrid(string $wkt): array + { + if (\preg_match('/^SRID=(\d+);(.*)$/s', $wkt, $m) === 1) { + return [(int) $m[1], $m[2]]; + } + return [null, $wkt]; + }tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeographyTypeTest.php (2)
55-62: Silence PHPMD: unused $typeName in overrideParameter is required for signature parity with the trait but unused here.
- protected function assertOrmValueEquals(mixed $expected, mixed $actual, string $typeName): void + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ + protected function assertOrmValueEquals(mixed $expected, mixed $actual, string $_typeName): void { if (!$expected instanceof WktSpatialData || !$actual instanceof WktSpatialData) { throw new \InvalidArgumentException('Expected WktSpatialData value objects.'); } $this->assertWktEquals($expected, $actual); }
83-101: Silence PHPMD: unused $testName from data providersThe label is for readability; it’s fine to ignore in the method.
- public function can_retrieve_null_geography_using_entity_manager_find(): void + public function can_retrieve_null_geography_using_entity_manager_find(): void @@ - public function can_retrieve_geography_values_using_entity_manager_find(string $testName, WktSpatialData $wktSpatialData): void + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ + public function can_retrieve_geography_values_using_entity_manager_find(string $_testName, WktSpatialData $wktSpatialData): void @@ - public function can_retrieve_geography_values_using_dql_select(string $testName, WktSpatialData $wktSpatialData): void + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ + public function can_retrieve_geography_values_using_dql_select(string $_testName, WktSpatialData $wktSpatialData): voidtests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeometryTypeTest.php (2)
55-62: Silence PHPMD: unused $typeName in overrideKeep signature; mark parameter unused or rename.
- protected function assertOrmValueEquals(mixed $expected, mixed $actual, string $typeName): void + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ + protected function assertOrmValueEquals(mixed $expected, mixed $actual, string $_typeName): void { if (!$expected instanceof WktSpatialData || !$actual instanceof WktSpatialData) { throw new \InvalidArgumentException('Expected WktSpatialData value objects.'); } $this->assertWktEquals($expected, $actual); }
90-108: Silence PHPMD: unused $testName from data providersRename to underscore or suppress.
- public function can_retrieve_geometry_values_using_entity_manager_find(string $testName, WktSpatialData $wktSpatialData): void + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ + public function can_retrieve_geometry_values_using_entity_manager_find(string $_testName, WktSpatialData $wktSpatialData): void @@ - public function can_retrieve_geometry_values_using_dql_select(string $testName, WktSpatialData $wktSpatialData): void + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ + public function can_retrieve_geometry_values_using_dql_select(string $_testName, WktSpatialData $wktSpatialData): voidtests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/OrmEntityPersistenceTrait.php (1)
157-164: Silence PHPMD: unused $columnType in prepareTestTableThe parameter is part of the higher-level API but not used here.
- protected function prepareTestTable(string $columnType): array + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ + protected function prepareTestTable(string $_columnType): array { $tableName = $this->getEntityTableName(); $columnName = $this->getEntityColumnName(); $this->createTestTableForEntity($tableName); return [$tableName, $columnName]; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseSpatialType.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Geography.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Geometry.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeographyTypeTest.php(3 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeometryTypeTest.php(3 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/OrmEntityPersistenceTrait.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/WktAssertionTrait.php(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#421
File: docs/AVAILABLE-TYPES.md:31-33
Timestamp: 2025-08-19T13:07:15.184Z
Learning: In martin-georgiev/postgresql-for-doctrine, the point[] array type should be documented as "point[]" not "_point" in the AVAILABLE-TYPES.md table, to be consistent with all other array types like text[], jsonb[], inet[], etc.
📚 Learning: 2025-08-24T16:52:32.488Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#0
File: :0-0
Timestamp: 2025-08-24T16:52:32.488Z
Learning: This repository uses BaseType extension pattern for custom Doctrine DBAL types with PostgreSQL platform assertions, comprehensive unit and integration testing with data providers, and dedicated exception classes for type conversion errors.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/OrmEntityPersistenceTrait.phpsrc/MartinGeorgiev/Doctrine/DBAL/Types/BaseSpatialType.php
📚 Learning: 2025-09-01T18:48:28.508Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#434
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/PostGIS/ST_CrossesTest.php:12-31
Timestamp: 2025-09-01T18:48:28.508Z
Learning: When analyzing unit test files in this codebase, always verify the actual file structure and existing patterns before suggesting changes. The ContainsGeometries fixture exists at ./fixtures/MartinGeorgiev/Doctrine/Entity/ContainsGeometries.php and the PostGIS unit tests use the namespace Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\PostGIS consistently.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeometryTypeTest.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/WktAssertionTrait.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeographyTypeTest.php
📚 Learning: 2025-08-27T18:19:35.789Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#430
File: tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StrictlyRightTest.php:31-36
Timestamp: 2025-08-27T18:19:35.789Z
Learning: WKT string literals can be used directly in Doctrine spatial function tests (e.g., 'POINT(-5 -5)') without explicit ST_GeomFromText wrapping, as PostgreSQL/PostGIS automatically handles the implicit conversion to geometry types when used with spatial operators.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/WktAssertionTrait.php
🧬 Code graph analysis (7)
src/MartinGeorgiev/Doctrine/DBAL/Types/Geography.php (1)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseSpatialType.php (1)
BaseSpatialType(19-43)
src/MartinGeorgiev/Doctrine/DBAL/Types/Geometry.php (1)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseSpatialType.php (1)
BaseSpatialType(19-43)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/OrmEntityPersistenceTrait.php (3)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeographyTypeTest.php (4)
getEntityClass(27-30)getEntityColumnName(32-35)assertOrmValueEquals(55-62)createTestTableForEntity(37-53)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeometryTypeTest.php (4)
getEntityClass(27-30)getEntityColumnName(32-35)assertOrmValueEquals(55-62)createTestTableForEntity(37-53)tests/Integration/MartinGeorgiev/TestCase.php (1)
dropTestTableIfItExists(252-259)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeometryTypeTest.php (4)
src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/WktSpatialData.php (1)
WktSpatialData(22-170)tests/Integration/MartinGeorgiev/TestCase.php (2)
TestCase(49-299)dropTestTableIfItExists(252-259)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/OrmEntityPersistenceTrait.php (6)
getEntityClass(44-44)getEntityColumnName(51-51)createTestTableForEntity(182-182)assertOrmValueEquals(67-70)runOrmFindRoundTrip(76-109)runOrmDqlSelectRoundTrip(115-152)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/WktAssertionTrait.php (1)
assertWktEquals(15-22)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/WktAssertionTrait.php (1)
src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/WktSpatialData.php (1)
WktSpatialData(22-170)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseSpatialType.php (1)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseType.php (1)
BaseType(17-52)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeographyTypeTest.php (5)
src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/WktSpatialData.php (1)
WktSpatialData(22-170)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeometryTypeTest.php (11)
DataProvider(83-88)DataProvider(96-101)DataProvider(103-108)Test(77-81)Test(90-94)getEntityClass(27-30)getEntityColumnName(32-35)createTestTableForEntity(37-53)assertOrmValueEquals(55-62)getTypeName(17-20)getPostgresTypeName(22-25)tests/Integration/MartinGeorgiev/TestCase.php (2)
TestCase(49-299)dropTestTableIfItExists(252-259)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/OrmEntityPersistenceTrait.php (6)
getEntityClass(44-44)getEntityColumnName(51-51)createTestTableForEntity(182-182)assertOrmValueEquals(67-70)runOrmFindRoundTrip(76-109)runOrmDqlSelectRoundTrip(115-152)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/WktAssertionTrait.php (1)
assertWktEquals(15-22)
🪛 PHPMD (2.15.0)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/OrmEntityPersistenceTrait.php
157-157: Avoid unused parameters such as '$columnType'. (undefined)
(UnusedFormalParameter)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeometryTypeTest.php
55-55: Avoid unused parameters such as '$typeName'. (undefined)
(UnusedFormalParameter)
98-98: Avoid unused parameters such as '$testName'. (undefined)
(UnusedFormalParameter)
105-105: Avoid unused parameters such as '$testName'. (undefined)
(UnusedFormalParameter)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseSpatialType.php
33-33: Avoid unused parameters such as '$platform'. (undefined)
(UnusedFormalParameter)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeographyTypeTest.php
55-55: Avoid unused parameters such as '$typeName'. (undefined)
(UnusedFormalParameter)
91-91: Avoid unused parameters such as '$testName'. (undefined)
(UnusedFormalParameter)
98-98: Avoid unused parameters such as '$testName'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: sloth
- GitHub Check: wait-for-tests-worflows-before-upload
🔇 Additional comments (6)
src/MartinGeorgiev/Doctrine/DBAL/Types/Geometry.php (1)
21-21: Good alignment with shared spatial baseSwitching to BaseSpatialType cleanly reuses the EWKT conversion logic for ORM reads. No further changes needed here.
src/MartinGeorgiev/Doctrine/DBAL/Types/Geography.php (1)
21-21: Consistent inheritance with GeometryExtending BaseSpatialType ensures uniform EWKT handling across spatial types. Looks good.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeographyTypeTest.php (1)
37-53: Confirm entity table name matches the created tableOrmEntityPersistenceTrait defaults to strtolower(ContainsGeometries) => “containsgeometries”. If the entity maps to “contains_geometries” (or another name), override getEntityTableName() to match, otherwise ORM find/DQL won’t see the inserted row.
If needed, add:
+ protected function getEntityTableName(): string + { + return 'contains_geometries'; // adjust to the entity’s actual table name + }Based on learnings.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/GeometryTypeTest.php (2)
37-53: Confirm entity table name matches the created tableAs with geography tests, ensure the created table name equals the entity’s mapped table. Override getEntityTableName() if needed so ORM reads the row you insert.
+ protected function getEntityTableName(): string + { + return 'contains_geometries'; // adjust if mapping differs + }Based on learnings.
64-75: DBAL select expression mirrors ORM behaviorThe CASE reproduces EWKT behavior for geometry at the DBAL level. Looks correct.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/OrmEntityPersistenceTrait.php (1)
56-62: Default table-name heuristic may not match entity mappinglowercase(class short name) can diverge from the entity’s mapped table (e.g., contains_geometries). Consider overriding getEntityTableName() in tests using this trait to avoid coupling test DDL to a guess.
|
Isn't this additional CASE performance overhead? Couldnt you just select This way you would avoid this additional CASE WHEN. |
Summary by CodeRabbit
New Features
Tests