[DBAL-3079] Reworked the usage of PDO in PDOConnection from inheritance to composition by morozov · Pull Request #3080 · doctrine/dbal

morozov · 2018-04-02T02:57:58Z

PDOConnection no longer extends PDO.
The PDO object can be obtained via PDOStatement::getWrappedConnection(). It's required to be able to call PDO-specific methods like setAttribute().
As long as the signature of PDOStatement::query() doesn't have to be compatible with PDO::query(), it's updated to PDOStatement::query(string $sql) : ResultStatement. This is the primary goal of this pull request.
Other query-related methods of Connection have been updated with typed arguments for consistency: prepare(), executeQuery(), executeUpdate(), exec(),
Driver\Statement::rowCount() was updated with : int since it's invoked from some methods above.
The tests wich invoked PDO methods on PDOStatement have been reworked to use PDOStatement::getWrappedConnection().
DriverConnectionMock has been removed since it required an update but wasn't use anywhere.

morozov · 2018-04-02T02:58:30Z

lib/Doctrine/DBAL/Driver/Connection.php

+     * @throws DBALException
     */
-    public function query();
+    public function query(string $sql);


I didn't add the return type since the wrapper connection will have to declare that it returns a driver connection. This will likely change in the future.

@morozov I didn't understand this comment: if we change this in future, wouldn't it be better to let this become a loud BC break via a change of the signature?

morozov · 2018-04-02T02:59:28Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

     * {@inheritdoc}
     */
-    public function prepare($prepareString, $driverOptions = [])
+    public function prepare($prepareString)


No longer needed to follow the PDO signature. Will add to the changelog later.

morozov · 2018-04-02T03:01:40Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

+     */
+    public function beginTransaction()
+    {
+        return $this->conn->beginTransaction();


Not wrapping this into a driver exception. Could be resolved in a different ticket.

Ocramius · 2018-04-02T10:02:34Z

lib/Doctrine/DBAL/Driver/Connection.php

+     * @throws DBALException
     */
-    public function query();
+    public function query(string $sql);


@morozov I didn't understand this comment: if we change this in future, wouldn't it be better to let this become a loud BC break via a change of the signature?

Ocramius · 2018-04-02T10:03:20Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

+    /**
+     * @var PDO
+     */
+    private $conn;


Suggest renaming this to connection or even pdoConnection to avoid abbreviation and confusion overall

morozov · 2018-04-12T04:44:00Z

@Ocramius want to take a look again?

Ocramius

Besides nits, this looks really good 👍

Ocramius · 2018-04-13T13:47:50Z

lib/Doctrine/DBAL/Connection.php

-     * @return \Doctrine\DBAL\Driver\Statement The executed statement.
+     * @param string                 $query  The SQL query to execute.
+     * @param mixed[]                $params The parameters to bind to the query, if any.
+     * @param mixed[]                $types  The types the previous parameters are in.


Types should be (int|string)[]

When iterating over (int|string)[], PhpStorm will recognize elements as mixed, but if it's int[]|string[] (not exactly correct), it will recognize them as int|string. This was previously brought up in #3025 (comment).

I'm still in favor of the proper annotation instead of making PhpStorm happy, especially if it's about scalar types (@Majkl578).

Please let's not introduce types like (int|string)[]. Although unfortunate, it doesn't work in PhpStorm, our CS and most of other tools either.

Hmm I see you actually did the change... :/
I am strongly against and it should be reverted given the current state of PHP ecosystem.

We need to achieve some agreement before reverting it. The state of the PHP ecosystem is PHP 7.1 = ~37%. Yet, we don't support the older versions.

The state of PHP ecosystem is: PHPStan supports this, any other software doesn't.

Ocramius · 2018-04-13T13:48:12Z

lib/Doctrine/DBAL/Connection.php

-     * @param \Doctrine\DBAL\Cache\QueryCacheProfile $qcp    The query cache profile.
+     * @param string            $query  The SQL query to execute.
+     * @param mixed[]           $params The parameters to bind to the query, if any.
+     * @param mixed[]           $types  The types the previous parameters are in.


Types should be (int|string)[]

Ocramius · 2018-04-13T13:49:06Z

lib/Doctrine/DBAL/Connection.php

-     *           raw PDOStatement instances.
+     * @param DriverStatement $stmt   The statement to bind the values to.
+     * @param mixed[]         $params The map/list of named/positional parameters.
+     * @param mixed[]         $types  The parameter types.


Types should be (int|string)[]

Ocramius · 2018-04-13T13:52:55Z

lib/Doctrine/DBAL/Driver/PDOPgSql/Driver.php

    {
        try {
-            $pdo = new PDOConnection(
+            $conn = new PDOConnection(


Can we rename $conn here? Let's avoid abbreviations

Ocramius · 2018-04-13T13:53:14Z

lib/Doctrine/DBAL/Driver/PDOSqlite/Driver.php


        try {
-            $pdo = new PDOConnection(
+            $conn = new PDOConnection(


Same: can we avoid the abbreviation?

Ocramius · 2018-04-13T13:58:48Z

lib/Doctrine/DBAL/Portability/Connection.php


 use Doctrine\DBAL\Cache\QueryCacheProfile;
 use Doctrine\DBAL\ColumnCase;
+use Doctrine\DBAL\Driver\PDOConnection;


Unrelated to this patch, but do we still need the portability connection in 3.x?

What's wrong with it? The column case conversion is very helpful when dealing with Oracle and IBM DB2.

On the other hand, I agree that PORTABILITY_RTRIM is just a crutch for badly designed code which happens to work on MySQL. And as for PORTABILITY_EMPTY_TO_NULL — no idea what it could be used for.

…e to composition

…eturn values

morozov · 2018-04-13T17:43:39Z

lib/Doctrine/DBAL/Connection.php

+     * @param QueryCacheProfile   $qcp    The query cache profile.
     *
-     * @throws \Doctrine\DBAL\Cache\CacheException
+     * @throws DBALException


CacheException is a subclass of DBALException which can happen underneath. Therefore, replaced.

Ocramius

Ocramius · 2018-04-13T22:50:28Z

Thanks @morozov!

stof · 2019-04-24T11:12:07Z

Could we have a way for DBAL's PDOStatement to expose the underlying PDOStatement ? PDO has a getColumnMeta method, which is useful to get info about the type of returned columns (I have a specific use case for that, where I have a controller running SQL queries stored in dedicated files and returning the data in CSV, and so the controller cannot assume the names of column headers as it changes depending on the query).

With DBAL 2.x, I can access this info thanks to the inheritance. But I don't see any way to achieve this in current state of 3.x now that composition is used. The PDOConnection allows accessing the wrapped connection, but the PDOStatement does not have the equivalent API.

morozov · 2019-04-24T15:11:21Z

@stof please file a separate ticket. For a use case like yours, it'd make sense to expose the underlying statement not only for PDO drivers.

stof · 2019-04-29T10:36:00Z

@morozov here it is: #3534

morozov commented Apr 2, 2018

View reviewed changes

Ocramius changed the title ~~[DBAL-3079] Reworked the usage of PDO inPDOConnection from inheritance to composition~~ [DBAL-3079] Reworked the usage of PDO in PDOConnection from inheritance to composition Apr 2, 2018

Ocramius suggested changes Apr 2, 2018

View reviewed changes

Ocramius assigned morozov Apr 2, 2018

Ocramius added Improvement Drivers Connections BC Break Prepared Statements PDO labels Apr 2, 2018

Ocramius added this to the 3.0.0 milestone Apr 2, 2018

morozov force-pushed the develop branch 3 times, most recently from fd275f8 to c78e0e5 Compare April 9, 2018 15:30

morozov force-pushed the issues/3079 branch 8 times, most recently from a06e9c0 to 9c8441a Compare April 12, 2018 03:35

Ocramius suggested changes Apr 13, 2018

View reviewed changes

Majkl578 self-requested a review April 13, 2018 14:16

morozov added 3 commits April 13, 2018 10:37

[DBAL-3079] Reworked the usage of PDO inPDOConnection from inheritanc…

e7ee98c

…e to composition

[DBAL-3079] Added type hints to query-related method parameters and r…

2aa5554

…eturn values

[DBAL-3079] Updated upgrade documentation

48bd0ca

morozov force-pushed the issues/3079 branch from 9c8441a to 48bd0ca Compare April 13, 2018 17:38

morozov commented Apr 13, 2018

View reviewed changes

Ocramius approved these changes Apr 13, 2018

View reviewed changes

Ocramius merged commit fe6bd76 into doctrine:develop Apr 13, 2018

morozov deleted the issues/3079 branch April 13, 2018 22:56

Majkl578 mentioned this pull request Apr 20, 2018

Revert complex type specification in Connection #3113

Merged

morozov mentioned this pull request May 25, 2018

Connection::query() should accept string $sql as first parameter #3079

Closed

morozov mentioned this pull request Apr 14, 2019

Implemented support for persistent connections in PDO and mysqli drivers #3515

Merged

morozov mentioned this pull request May 23, 2019

Use variadic instead of func_get_args #3455

Closed

This was referenced Dec 30, 2019

Research the possibility to backport PDO-related changes from master to 2.10 #3798

Closed

Backport PDO-related changes from master to 3.0.x #3803

Merged

morozov modified the milestones: 4.0.0, 3.0.0 Oct 3, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2022

Uh oh!

Conversation

morozov commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov commented Apr 12, 2018

Uh oh!

Ocramius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ocramius left a comment

Choose a reason for hiding this comment

Uh oh!

Ocramius commented Apr 13, 2018

Uh oh!

stof commented Apr 24, 2019

Uh oh!

morozov commented Apr 24, 2019

Uh oh!

stof commented Apr 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

morozov commented Apr 2, 2018 •

edited

Loading