Add support for Postgres positional parameters to Parser

[sweoggy]

Q	A
Type	improvement
Fixed issues

Summary

This PR enables using Postgres placeholders $# with doctrine/dbal. For example the following query work after this patch:

$connection->executeStatement(
    'INSERT INTO dummy_table (some_id) VALUES ($1)',
    [1],
    [],
);

Without this patch a driver exception is thrown: Could not find parameter 1 in the SQL statement.

[morozov]

@sweoggy, thanks for the patch, it looks good.

Besides the unit tests, could you add an integration test that demonstrate the usage of these parameters with PostgreSQL? It doesn't look like DBAL has ever supported them.

Also, the parser is a back port of the corresponding parser from PDO. It looks like at the time of the back port, it didn't support such parameters or I missed something. In any event, I want to keep its implementation close to PDO. If the support for these parameters was introduced in an newer PHP version, please update the link in the PHPDoc.

[morozov]

Would it make sense to extract the new pattern into a separate constant?

[sweoggy]

I don't think so. Two arguments:

• This is Postgres' way of specifying positional parameters. So the naming makes a lot of sense.
• Adding an additional pattern would incur a performance penalty and complicate the code (having to add one more handler, which in turn just calls $visitor->acceptPositionalParameter($sql);)

[sweoggy]

@sweoggy, thanks for the patch, it looks good.

Besides the unit tests, could you add an integration test that demonstrate the usage of these parameters with PostgreSQL? It doesn't look like DBAL has ever supported them.

Also, the parser is a back port of the corresponding parser from PDO. It looks like at the time of the back port, it didn't support such parameters or I missed something. In any event, I want to keep its implementation close to PDO. If the support for these parameters was introduced in an newer PHP version, please update the link in the PHPDoc.

I believe this is a native pgsql driver feature, https://www.postgresql.org/docs/current/sql-expressions.html#SQL-EXPRESSIONS-PARAMETERS-POSITIONAL

I just pushed a fix for re-occurring positional parameters and added an integration test

[morozov]

I just realized it now that the SQL parser is used elsewhere than the wrapper statement and the oci8 driver – specifically, the pgsql driver. I think it's fine for now but its purpose becomes less and less clear, so we may want to break it down into a few smaller ones to address #3744.

Since @derrabus is the original author of the pgsql driver, I'd like him to review this patch as well.

As a second thought: if those are the placeholders natively recognized by the underlying driver, then why does it require a SQL parser in the DBAL implementation of the driver for them to work? So far, the parser has been used only in order to support the placeholders that the given component doesn't support natively.

[morozov]

It looks like this test passes on pdo_pgsql as well (not sure how it's implemented in PDO). Please move it into a dedicated test class and enable for pdo_pgsql.

Since it's a functional test, it would be better to name it in a way that it reflects the functionality being tested (something about PostgreSQL native positional parameters). The parser is an implementation detail, which isn't always relevant (specifically, in the case of pdo_pgsql).

[sweoggy]

I moved it to a separate class. Tried to enable it for pdo_pgsql as well, but it actually doesn't work, it doesn't seem to pass the values to the query and instead tries to insert null values.

Not sure if this is a limitation of pdo_pgsql or further changes would be required to Doctrine to support positional parameters for this driver. Error can be seen here https://github.com/debricked/dbal/actions/runs/12137000213/job/33839318280

[sweoggy]

Seems to be a limitation of PDO https://www.php.net/pdo.prepare

[derrabus]

As a second thought: if those are the placeholders natively recognized by the underlying driver, then why does it require a SQL parser in the DBAL implementation of the driver for them to work? So far, the parser has been used only in order to support the placeholders that the given component doesn't support natively.

The parser is currently used to translate the PDO-style positional parameters (?) to the positional parameters that Postgres understands ( $1, $2, …). I assume that our "feature" gets in the way if someone actually wants to use the Postgres-style positional parameters.

[sweoggy]

I just realized it now that the SQL parser is used elsewhere than the wrapper statement and the oci8 driver – specifically, the pgsql driver. I think it's fine for now but its purpose becomes less and less clear, so we may want to break it down into a few smaller ones to address #3744.

Since @derrabus is the original author of the pgsql driver, I'd like him to review this patch as well.

As a second thought: if those are the placeholders natively recognized by the underlying driver, then why does it require a SQL parser in the DBAL implementation of the driver for them to work? So far, the parser has been used only in order to support the placeholders that the given component doesn't support natively.

Regarding the second thought. This change is needed because Doctrine\DBAL\Driver\PgSQL\ConvertParameters::getParameterMap() is called on line 58 for Doctrine\DBAL\Driver\PgSQL\Connection whenever you are using a prepared statement.

Without this change, the Parser does not call Doctrine\DBAL\Driver\PgSQL\ConvertParameters::acceptPositionalParameter(), which means that the parameter map will be empty and there will be nothing to map the passed parameters to, which results in the could not find parameter 1 in the SQL statement exception.

[morozov]

The parser is currently used to translate the PDO-style positional parameters (?) to the positional parameters that Postgres understands ( $1, $2, …). I assume that our "feature" gets in the way if someone actually wants to use the Postgres-style positional parameters.

That's my understanding as well. I believe that instead of making the parser support what the driver supports natively, a better approach would be to make the parser not get in the way of someone using the native parameters. The driver should not consider the parameter map derived from parsing the query the source of truth. It should only augment the native placeholders with the PDO-style ones.