Conversation
| self::assertSame( | ||
| [ | ||
| 'CREATE TABLE spatial_table (id INT NOT NULL, ' | ||
| . 'location geometry(geometry) NOT NULL, ' | ||
| . 'point_location geometry(point,4326) NOT NULL, ' | ||
| . 'polygon_area geometry(polygon) NOT NULL)', | ||
| ], | ||
| $this->platform->getCreateTableSQL($table), | ||
| ); |
There was a problem hiding this comment.
Please focus on functional tests. The kind of test that you're building here is pretty much worthless because it does not tell us if the SQL actually works when it hits a database.
There was a problem hiding this comment.
Thanks for the feedback! I already had in mind to add functional tests, since they’re definitely the right way to validate the SQL.
Because these tests require PostgreSQL with the PostGIS extension, would you suggest:
- creating a dedicated GitHub workflow (e.g.
postgresql-postgis.yml) that uses thepostgis/postgisimage, or - replacing the current PostgreSQL workflow with that image so that all PostgreSQL jobs run with PostGIS enabled?
I’d be happy to go with whichever approach best fits the project’s CI strategy.
There was a problem hiding this comment.
We need to make sure DBAL still works with Postgres databases that don't have these extensions installed. So it's probably best for now if we add an additional check for Postgres that uses the latest postgis image and leave the other checks as they are.
There was a problem hiding this comment.
@derrabus I’ve added the functional tests as suggested and pushed them. However, I noticed that none of the functional tests actually executed in the PR pipeline — is there something I need to do to make sure they run?
If you already had a chance to take a look, I’d really appreciate some feedback on whether this is heading in the right direction. That way I can adjust before proceeding with support for a second platform (e.g. MySQL).
There was a problem hiding this comment.
As far as I can tell, your new workflows were run: https://github.com/doctrine/dbal/actions/runs/17288021054/job/49069200071?pr=7096
There was a problem hiding this comment.
Yes, I noticed an error in continuous-integration.yml only afterwards. After fixing it, the functional tests were executed correctly.
I have one question regarding the functional tests: in MySQL there is a test case Doctrine\DBAL\Tests\Functional\Schema\MySQLSchemaManagerTest::testColumnIntrospection that attempts to define any type as a column to compare it against the same definition. At the moment, I’ve introduced two types for PostgreSQL/PostGIS, geometry and geography. Since geography is not available in MySQL, the current test setup would fail.
Is this behavior intentional, or would it be acceptable to adjust the tests so that they explicitly check which types are supported on MySQL?
543f582 to
a35520d
Compare
810f526 to
86b4b15
Compare
|
Adding spatial index management would be interesting for this PR I think :). For instance: Sources: |
Thanks for the advice — I agree, index management is definitely an important aspect when working with geometry data. I haven’t investigated it in depth yet, but it looks like this might already be supported by specifying the index type. A quick search in the repository shows an |
ba2a7aa to
a33fcd8
Compare
0b81371 to
a3544b1
Compare
|
Wow, that's exactly what I'd like to see integrated into Doctrine soon. Great work! |
98c3be5 to
e9e86f4
Compare
|
Hi @derrabus I’ve addressed all the feedback received so far and updated the PR accordingly. Thanks a lot for your time |
derrabus
left a comment
There was a problem hiding this comment.
Thank your for all the work you've put into this PR. Your changes look very promising.
| /** | ||
| * Normalizes types array from positional or associative to associative format. | ||
| * | ||
| * @param array<int<0,max>, string|ParameterType|Type>|array<string, string|ParameterType|Type> $types | ||
| * @param list<string> $columnNames | ||
| * | ||
| * @return array<string, string|ParameterType|Type> | ||
| */ | ||
| private function normalizeTypes(array $types, array $columnNames): array | ||
| { |
There was a problem hiding this comment.
I introduced normalizeTypes() because I needed a reliable way to support SQL-level type conversion via Type::convertToDatabaseValueSQL(), which is required for spatial types and may also be relevant for other custom types in the future.
With insert() and update(), the $types argument can be passed in two different forms:
- Positional, e.g.
[0 => 'geometry', 1 => 'string'] - Associative, e.g.
['location' => 'geometry', 'name' => 'string']
The new getPlaceholderForColumn() method operates at the column level and needs to know whether a specific column requires SQL-level conversion using convertToDatabaseValueSQL(). For that decision, the type must be known by column name.
When $types is positional, there is no straightforward or safe way to determine the type for a given column name at that point. I added normalizeTypes() to normalize both positional and associative $types into a single, column-name–keyed structure. This allows getPlaceholderForColumn() to consistently determine:
- which type applies to each column
- whether SQL-level conversion should be applied
There was a problem hiding this comment.
So the current type abstraction does not work for these new types? It's quite unusual that we need to change the connection class on the wrapper layer for introducing new types.
There was a problem hiding this comment.
Thanks for the discussion — I did a bit more digging to better understand how type conversion is currently applied.
In DBAL itself, convertToDatabaseValue() is called when binding parameters, so value-level conversion works as expected. However, I couldn’t find any code paths in DBAL where convertToDatabaseValueSQL() (or convertToPHPValueSQL()) is actually invoked. As far as I can tell, these methods are currently required and used by doctrine/orm, not by DBAL directly.
This becomes relevant for spatial types because converting GeoJSON into a native geometry value in PostGIS, MySQL, or MariaDB cannot be done via casting — all of them require an explicit SQL function call such as ST_GeomFromGeoJSON(...). If the SQL-level conversion hook isn’t invoked, any custom SQL logic implemented in convertToDatabaseValueSQL() would effectively be ignored.
From this perspective, DBAL currently:
- supports value-level conversion at bind time (
convertToDatabaseValue()), - but does not apply SQL-level conversion hooks during
insert()/update()flows.
That’s the context in which I explored a possible solution at the connection layer: without a place where convertToDatabaseValueSQL() is actually used, this appeared to be a way to express database-specific SQL transformations while still allowing users to work with a user-friendly input format like GeoJSON.
Happy to adjust the approach if there’s a more idiomatic way to support this in DBAL — I mainly wanted to share these findings to provide some additional context around the motivation.
There was a problem hiding this comment.
So there's no binary format that can be insterted directly into those columns? I've run into similar problems while working on the VECTOR type. And you're right, convertToDatabaseValueSQL() is never called by the DBAL, currently. Maybe we need a better abstraction here.
There was a problem hiding this comment.
Good question — this does indeed seem very similar to the VECTOR case.
While geometry columns can sometimes accept WKB directly, conversion functions like ST_GeomFromWKB() or ST_GeomFromGeoJSON() are generally the more robust and recommended approach across spatial databases, as they make the conversion explicit and behave consistently.
There are also some portability concerns with a pure WKB-based approach:
- EWKB (with embedded SRID) is a PostGIS extension and isn’t supported by MySQL or MariaDB, so SRID handling would still require additional logic.
- Relying on implicit casting of binary values feels more fragile than using explicit SQL functions, especially across different platforms and versions.
Because of that, a WKB-based workaround could work in some cases, but it feels more like a workaround than a solid abstraction.
Given this, I’d appreciate some guidance on direction:
- should we explore a WKB-based value-level approach despite these trade-offs, or
- is it worth discussing a DBAL-level abstraction for types that require SQL-level transformation during insert/update (which would also apply to cases like VECTOR)?
Personally, I would lean towards exploring a proper abstraction if that aligns with DBAL’s design goals, but I’m happy to follow the direction you think makes the most sense.
There was a problem hiding this comment.
is it worth discussing a DBAL-level abstraction for types that require SQL-level transformation during insert/update
I think so, yes.
There was a problem hiding this comment.
Before going deeper into an implementation, I’d like to better understand what kind of abstraction you have in mind. From my side, I was trying to reason about where SQL-level type conversion could live in a way that stays consistent and avoids duplication.
One idea I explored was whether it would make sense for Connection::insert() / update() to internally rely on QueryBuilder, so that SQL-level conversion (via convertToDatabaseValueSQL()) could be handled in a single place rather than re-implemented in multiple code paths.
That said, I’m not attached to this approach — I’d be very interested to hear your thoughts on:
- what shape you imagine this abstraction taking, and
- whether using
QueryBuilderinsideConnectionwould align with DBAL’s architectural direction, or if you’d prefer to keep them clearly independent.
Thanks a lot for your review and the detailed feedback! |
e9e86f4 to
b4e6ec4
Compare
b4e6ec4 to
fefc5d4
Compare
|
The tests fail on MySQL. |
8b23377 to
637feb1
Compare
Apologies for that — I hadn’t rerun the tests on MySQL 5.7 yet. I’ve now applied the necessary fixes so the test suite passes on both MySQL 5.7 and MySQL ≥ 8, and I’ve verified that the static analysis checks pass as well. |
No, it still fails on 5.7. |
b702189 to
84f01c9
Compare
84f01c9 to
a7cddba
Compare
a7cddba to
5d6ff38
Compare
|
Just a gentle follow-up on this topic. I completely understand things get busy — I just wanted to check whether you’ve had a chance to think about the abstraction question around SQL-level type conversion. I’m happy to move forward in whichever direction you think fits best (e.g. exploring a QueryBuilder-based approach, or something different). If you have a preference, that would help me focus the implementation accordingly. |
|
Hi @ddegasperi, there is still an error related to PHPStan in the pipeline, but I am unsure if it is related to your PR. |
Add geometry and geography types for spatial data Introduces new spatial data types for handling geometric and geographic data in database applications. GeometryType handles planar coordinates while GeographyType handles spherical earth coordinates, both using GeoJSON format. This provides foundation for spatial data operations across database platforms that support spatial extensions.
Extends Column and ColumnEditor with geometryType and srid properties to support PostgreSQL's PostGIS spatial types and provides a clean schema API for working with GEOMETRY and GEOGRAPHY columns while maintaining backward compatibility. This builds on the core spatial types implementation to complete the PostgreSQL spatial type support at the schema level.
This commit adds functional testing for PostGIS spatial types (GEOMETRY and GEOGRAPHY) with schema introspection and CI integration. The implementation leverages PostgreSQL's native type system for introspection, making it compatible with any PostgreSQL instance without requiring PostGIS system tables to be accessible during schema operations.
Extends AbstractMySQLPlatform with GEOMETRY type support and enhances MySQLSchemaManager to introspect spatial columns with geometryType and SRID properties. MySQL supports GEOMETRY types (POINT, LINESTRING, POLYGON, etc.) with optional SRID constraints using the conditional comment syntax for MySQL 8.0.3+.
Refactor GeometryType and GeographyType to replace direct GeoJSON string handling with dedicated value objects, preventing exposure of database-specific formats to application code. This commit introduces a Geometry value object that encapsulates a supporting GeoJSON value object responsible for validating and wrapping GeoJSON representations.
Implement spatial index creation and introspection for PostgreSQL using the GIST (Generalized Search Tree) index method — the standard access method for spatial data in PostGIS. This commit introduces a new getIndexMethodSQL() hook in AbstractPlatform for platform-specific index clauses, and overrides it in PostgreSQLPlatform to emit "USING GIST" for spatial indexes. SQL generation examples: MySQL → CREATE SPATIAL INDEX idx ON table (col) PostgreSQL → CREATE INDEX idx ON table USING GIST (col)
5d6ff38 to
ccedf0e
Compare
@seb-jean The reported PHPStan error points to I’ve rebased my branch on the latest |
|
That's exactly what I thought when I went through PR's code. |
Summary
This PR introduces spatial data type support for DBAL, starting with PostgreSQL/PostGIS and MySQL. This implementation is intended to serve as a reference and guide for adding support for other database platforms in the future.
Core Spatial Types
Types::GEOMETRY) - For planar coordinate systemsTypes::GEOGRAPHY) - For spherical earth coordinatesSchema API Integration
ColumnandColumnEditorwithgeometryTypeandsridpropertiesColumn::editor()->setGeometryType('POINT')->setSrid(4326)What's next