Implement PDO driver specific sub-classes #12804

kocsismate · 2023-11-27T22:05:27Z

Rebased version of #8707
RFC: https://wiki.php.net/rfc/pdo_driver_specific_subclasses

kocsismate · 2023-11-27T22:05:50Z

@Danack I'll try to take care of your RFC :) For now, I only managed to rebase your PR on master, so I didn't change it much yet.

SakiTakamachi · 2023-11-29T10:27:18Z

Since it will be unbundled, it seems possible to exclude OCI.

devnexen

LCTM for the pgsql part.

SakiTakamachi

I made some comments.
Most are minor nits.

SakiTakamachi · 2024-01-07T16:39:08Z

ext/pdo/pdo_dbh.c

@@ -1334,6 +1410,8 @@ static HashTable *dbh_get_gc(zend_object *object, zval **gc_data, int *gc_count)
 }

 static zend_object_handlers pdo_dbh_object_handlers;
+static zend_object_handlers pdosqlite_dbh_object_handlers;


Is this just for SQLite?

Ah no, I'm not sure how it ended up being here... But it's unused, nothing uses this struct.

SakiTakamachi · 2024-01-07T16:39:31Z

ext/pdo/pdo_dbh.c

+	memcpy(&pdosqlite_dbh_object_handlers, &std_object_handlers, sizeof(zend_object_handlers));
+	pdosqlite_dbh_object_handlers.offset = XtOffsetOf(pdo_dbh_object_t, std);
+	pdosqlite_dbh_object_handlers.free_obj = pdo_dbh_free_storage;
+	pdosqlite_dbh_object_handlers.get_method = dbh_method_get;
+	pdosqlite_dbh_object_handlers.compare = zend_objects_not_comparable;
+	pdosqlite_dbh_object_handlers.get_gc = dbh_get_gc;


SakiTakamachi · 2024-01-07T16:48:57Z

ext/pdo_dblib/tests/pdodblib_001.phpt

+$db->query("CREATE TABLE #pdo_dblib_001(name varchar(32)); ");
+$db->query("INSERT INTO #pdo_dblib_001 values('PHP'), ('PHP6');");


nits

Suggested change

$db->query("CREATE TABLE #pdo_dblib_001(name varchar(32)); ");

$db->query("INSERT INTO #pdo_dblib_001 values('PHP'), ('PHP6');");

$db->query("CREATE TABLE #pdo_dblib_001(name VARCHAR(32)); ");

$db->query("INSERT INTO #pdo_dblib_001 VALUES('PHP'), ('PHP6');");

SakiTakamachi · 2024-01-07T16:49:40Z

ext/pdo_dblib/tests/pdodblib_002.phpt

+$db->query("CREATE TABLE #pdo_dblib_002(name varchar(32))");
+$db->query("INSERT INTO #pdo_dblib_002 values('PHP'), ('PHP6')");


SakiTakamachi · 2024-01-07T16:53:56Z

ext/pdo_firebird/tests/pdofirebird_001.phpt

+
+$db = getDbConnection();
+
+$db->query('CREATE TABLE pdofirebird_001 (idx int NOT NULL PRIMARY KEY, name VARCHAR(20))');


nits

Suggested change

$db->query('CREATE TABLE pdofirebird_001 (idx int NOT NULL PRIMARY KEY, name VARCHAR(20))');

$db->query('CREATE TABLE pdofirebird_001 (idx INT NOT NULL PRIMARY KEY, name VARCHAR(20))');

SakiTakamachi · 2024-01-07T16:56:49Z

ext/pdo_mysql/tests/pdomysql_002.phpt

+    echo "Wrong class type. Should be PdoMysql but is " . get_class($db) . "\n";
+}
+
+$db->exec('CREATE TABLE pdomysql_002(id int NOT NULL PRIMARY KEY, name VARCHAR(10))');


nits

Suggested change

$db->exec('CREATE TABLE pdomysql_002(id int NOT NULL PRIMARY KEY, name VARCHAR(10))');

$db->exec('CREATE TABLE pdomysql_002(id INT NOT NULL PRIMARY KEY, name VARCHAR(10))');

SakiTakamachi · 2024-01-07T16:58:23Z

ext/pdo_odbc/tests/pdoodbc_002.phpt

+    echo "Wrong class type. Should be PdoOdbc but is " . get_class($db) . "\n";
+}
+
+$db->exec('CREATE TABLE pdoodbc_002(id int NOT NULL PRIMARY KEY, name VARCHAR(10))');


nits

Suggested change

$db->exec('CREATE TABLE pdoodbc_002(id int NOT NULL PRIMARY KEY, name VARCHAR(10))');

$db->exec('CREATE TABLE pdoodbc_002(id INT NOT NULL PRIMARY KEY, name VARCHAR(10))');

SakiTakamachi · 2024-01-07T17:01:00Z

ext/pdo_pgsql/tests/pdopgsql_002.phpt

+    echo "Wrong class type. Should be PdoPgsql but is " . get_class($db) . "\n";
+}
+
+$db->exec('CREATE TABLE pdopgsql_002(id int NOT NULL PRIMARY KEY, name VARCHAR(10))');


nits

Suggested change

$db->exec('CREATE TABLE pdopgsql_002(id int NOT NULL PRIMARY KEY, name VARCHAR(10))');

$db->exec('CREATE TABLE pdopgsql_002(id INT NOT NULL PRIMARY KEY, name VARCHAR(10))');

SakiTakamachi · 2024-01-07T17:08:15Z

ext/pdo_sqlite/tests/subclasses/pdo_sqlite_createcollation.phpt

+$db = new PdoSqlite('sqlite::memory:');
+$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
+
+$db->query('CREATE TABLE IF NOT EXISTS test_pdo_sqlite_createcollation (id INT AUTO INCREMENT, name TEXT)');


Does it need IF NOT EXISTS? Maybe forgot to fix this.

Yes, I forgot it previously.

SakiTakamachi · 2024-01-07T17:14:52Z

ext/pdo_sqlite/tests/subclasses/pdo_sqlite_createcollation_trampoline.phpt

+
+$db = new PdoSqlite('sqlite::memory:');
+
+$db->query('CREATE TABLE test_pdo_sqlite_createcollation_trampoline (s varchar(4))');


nits

Suggested change

$db->query('CREATE TABLE test_pdo_sqlite_createcollation_trampoline (s varchar(4))');

$db->query('CREATE TABLE test_pdo_sqlite_createcollation_trampoline (s VARCHAR(4))');

nielsdos

Some minor comments of things I noticed in quick glance.

I dislike the 1:1 code copies from driver files, e.g. sqlite_driver.c contains from a quick glance the exact same implementation for createFunction as PHP_METHOD(PdoSqlite, createFunction), this is risky as they can get out of sync and are bad for maintenance.

nielsdos · 2024-01-07T19:58:42Z

ext/pdo_mysql/pdo_mysql.c

+/* proto string PDO::mysqlGetWarningCount()
+ * Returns the number of SQL warnings during the execution of the last statement
+ */
+PHP_METHOD(PdoMysql, getWarningCount)


Don't you need a ZPP check here to parse no parameters?

Uh, ZPP is definitely needed here... Unfortunately I couldn't spot this bug from the original implementation

nielsdos · 2024-01-07T19:58:58Z

ext/pdo_odbc/pdo_odbc.c

@@ -27,6 +27,8 @@
 #include "php_pdo_odbc_int.h"
 #include "pdo_odbc_arginfo.h"

+zend_class_entry *pdo_odbc_ce;


Why is this a global?

Coincidentally, I had just noticed that these class entries had been accidentally declared global, so I have already added the static modifier locally. :) Anyway, thanks for the reminder!

nielsdos · 2024-01-07T19:59:42Z

ext/pdo/pdo.c

@@ -41,6 +41,9 @@ zend_class_entry *pdo_exception_ce;
 /* the registry of PDO drivers */
 HashTable pdo_driver_hash;

+/* the registry of PDO driver specific class entries */
+HashTable pdo_driver_specific_ce_hash;


Make static perhaps?

This one is exposed as it's needed by pdo_dbh.c.

nielsdos · 2024-01-07T20:00:47Z

ext/pdo_pgsql/pdo_pgsql.c

 #include "pdo/php_pdo_driver.h"
 #include "php_pdo_pgsql.h"
 #include "php_pdo_pgsql_int.h"
+#include "pdo_pgsql_arginfo.h"
+
+zend_class_entry *PdoPgsql_ce;


Same "why is this a global" question here. Actually each driver seems to have their own global CE, so the question could be repeated for each of them :P

SakiTakamachi · 2024-01-08T00:59:41Z

I dislike the 1:1 code copies from driver files, e.g. sqlite_driver.c contains from a quick glance the exact same implementation for createFunction as PHP_METHOD(PdoSqlite, createFunction), this is risky as they can get out of sync and are bad for maintenance.

According to the RFC section "When to deprecate old function on PDO", these functions in the PDO Core should be removed at some point.

There is no mention of how it will be maintained in the interim, so that is up for debate.

nielsdos · 2024-01-08T08:37:45Z

I dislike the 1:1 code copies from driver files, e.g. sqlite_driver.c contains from a quick glance the exact same implementation for createFunction as PHP_METHOD(PdoSqlite, createFunction), this is risky as they can get out of sync and are bad for maintenance.

According to the RFC section "When to deprecate old function on PDO", these functions in the PDO Core should be removed at some point.

There is no mention of how it will be maintained in the interim, so that is up for debate.

That doesn't stop us from using an implementation alias or moving the code to a common place. We should still follow best practices during the time the code is not yet deprecated.

SakiTakamachi · 2024-01-08T11:32:28Z

That doesn't stop us from using an implementation alias or moving the code to a common place. We should still follow best practices during the time the code is not yet deprecated.

I agree that we shouldn't have duplicate implementations.

Besides SQLite, pgsql is the only driver with a unique method in the existing implementation, and pgsql seems to have a good idea about this problem.

Personally, I think it's enough to make SQLite work like pgsql.

Maybe I misunderstood your point. I think this PR's way of implementing pgsql's unique methods makes sense in terms of duplication, how do you think of this?

kocsismate · 2024-01-08T15:54:41Z

I dislike the 1:1 code copies from driver files, e.g. sqlite_driver.c contains from a quick glance the exact same implementation for createFunction as PHP_METHOD(PdoSqlite, createFunction), this is risky as they can get out of sync and are bad for maintenance.

Yes, I agree with you. I forgot to unify the implementations indeed. Now, I double checked the duplications, and fixed them for pdo_sqlite.

Additionally, I tried to address Saki's review comments.

SakiTakamachi · 2024-01-08T16:06:49Z

Thank you, LGTM!

kocsismate · 2024-01-08T16:09:24Z

I found a few more things which can be deduplicated, so I pushed yet another commit. Sorry for the noise!

nielsdos · 2024-01-08T16:29:14Z

Personally, I think it's enough to make SQLite work like pgsql.

Maybe I misunderstood your point. I think this PR's way of implementing pgsql's unique methods makes sense in terms of duplication, how do you think of this?

Yep, I was specifically talking about sqlite, which Máté has cleaned up nicely now :-)
Thanks for fixing that.

I do still see from a quick glance there are still global zend_class_entry vars which could either be static or locals (e.g. PdoDblib_ce)

kocsismate · 2024-01-09T07:22:24Z

I do still see from a quick glance there are still global zend_class_entry vars which could either be static or locals (e.g. PdoDblib_ce)

Ah yes, indeed! Fixed now :)

nielsdos · 2024-01-09T18:51:20Z

I do still see from a quick glance there are still global zend_class_entry vars which could either be static or locals (e.g. PdoDblib_ce)

Ah yes, indeed! Fixed now :)

Cool, thanks! I didn't take a deep look, only a glance, nothing else caught my eye :-)

kocsismate · 2024-01-11T15:48:30Z

@Girgias Are you also fine with the merge?

Girgias

Yes :)

iluuu1994 · 2024-01-13T13:10:55Z

ext/pdo_firebird/tests/pdofirebird_001.phpt

+pdo_firebird
+--SKIPIF--
+<?php require(__DIR__ . '/skipif.inc'); ?>
+--FILE--


@kocsismate See #12699, these should be marked with XLEAK. For reference: https://github.com/php/php-src/actions/runs/7509344302/job/20446349499

@iluuu1994
ah! I knew about the firebird leak, but I overlooked it in the review.

@kocsismate
This and 002 also require XLEAK.

iluuu1994 · 2024-01-13T13:13:04Z

This also broke https://github.com/php/php-src/actions/runs/7509344302/job/20446344121. @kocsismate Can you have a look?

SakiTakamachi · 2024-01-13T15:05:59Z

This also broke https://github.com/php/php-src/actions/runs/7509344302/job/20446344121. @kocsismate Can you have a look?

I took a look and found that this constant is only defined when using mysqlnd, so it should use #ifdef PDO_USE_MYSQLND.
(libmysql is only used nightly test so I couldn't found that in normal testing)

kocsismate · 2024-01-14T15:31:25Z

@SakiTakamachi Thanks for looking into it! @iluuu1994 Thanks for letting me know about the failure! My recent commit should fix the problem: 9c7b391

iluuu1994 · 2024-01-14T17:18:09Z

Cool, @kocsismate thanks for taking care of it and thanks @SakiTakamachi for investigating.

janedbal · 2024-02-21T16:56:01Z

Would it be possible to add also some getWarnings() (beside the new getWarningCount) method to PdoMysql similary like mysqli has it?

SakiTakamachi · 2024-02-21T17:40:00Z

@janedbal
Hi, it is preferable to open a new issue rather than posting new suggestions in this thread.

And perhaps your suggestion would be better discussed on the mailing list.

kocsismate added the RFC label Nov 27, 2023

kocsismate requested a review from devnexen as a code owner November 27, 2023 22:05

github-actions bot added Category: Build System Extension: pdo (core) Extension: pdo_dblib Extension: pdo_firebird Extension: pdo_mysql Extension: pdo_oci Extension: pdo_odbc Extension: pdo_pgsql Extension: pdo_sqlite labels Nov 27, 2023

kocsismate marked this pull request as draft November 27, 2023 22:07

kocsismate force-pushed the pdo_sub_classing branch from 40769bc to b55473a Compare November 30, 2023 21:05

github-actions bot removed the Extension: pdo_oci label Nov 30, 2023

kocsismate force-pushed the pdo_sub_classing branch 14 times, most recently from e740462 to 08e50ef Compare December 6, 2023 22:05

devnexen approved these changes Jan 7, 2024

View reviewed changes

SakiTakamachi reviewed Jan 7, 2024

View reviewed changes

nielsdos reviewed Jan 7, 2024

View reviewed changes

Address new review comments

0de1b88

Remove even more redundancies

147a10b

Mark all PDO subclass entries as static

631024b

Girgias approved these changes Jan 11, 2024

View reviewed changes

kocsismate closed this in d6a0b3a Jan 11, 2024

iluuu1994 reviewed Jan 13, 2024

View reviewed changes

kocsismate deleted the pdo_sub_classing branch January 14, 2024 07:07

Girgias mentioned this pull request Feb 21, 2024

Add getWarnings() method to PdoMysql class #13462

Open

jrfnl mentioned this pull request Sep 7, 2024

PHP 8.4 | NewClasses: add support for PDO driver specific subclasses (RFC) PHPCompatibility/PHPCompatibility#1755

Merged

stancl mentioned this pull request Dec 24, 2024

Compile with Pdo\Sqlite::loadExtension() by default? #17251

Closed

		$db->query("CREATE TABLE #pdo_dblib_001(name varchar(32)); ");
		$db->query("INSERT INTO #pdo_dblib_001 values('PHP'), ('PHP6');");

		$db->query("CREATE TABLE #pdo_dblib_002(name varchar(32))");
		$db->query("INSERT INTO #pdo_dblib_002 values('PHP'), ('PHP6')");


		$db = getDbConnection();

		$db->query('CREATE TABLE pdofirebird_001 (idx int NOT NULL PRIMARY KEY, name VARCHAR(20))');

	$db->exec('CREATE TABLE pdomysql_002(id int NOT NULL PRIMARY KEY, name VARCHAR(10))');
	$db->exec('CREATE TABLE pdomysql_002(id INT NOT NULL PRIMARY KEY, name VARCHAR(10))');

	$db->exec('CREATE TABLE pdoodbc_002(id int NOT NULL PRIMARY KEY, name VARCHAR(10))');
	$db->exec('CREATE TABLE pdoodbc_002(id INT NOT NULL PRIMARY KEY, name VARCHAR(10))');

	$db->exec('CREATE TABLE pdopgsql_002(id int NOT NULL PRIMARY KEY, name VARCHAR(10))');
	$db->exec('CREATE TABLE pdopgsql_002(id INT NOT NULL PRIMARY KEY, name VARCHAR(10))');


		$db = new PdoSqlite('sqlite::memory:');

		$db->query('CREATE TABLE test_pdo_sqlite_createcollation_trampoline (s varchar(4))');

Implement PDO driver specific sub-classes #12804

Implement PDO driver specific sub-classes #12804

Conversation

kocsismate commented Nov 27, 2023

kocsismate commented Nov 27, 2023 • edited Loading

SakiTakamachi commented Nov 29, 2023

devnexen left a comment

Choose a reason for hiding this comment

SakiTakamachi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate Jan 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SakiTakamachi commented Jan 8, 2024

nielsdos commented Jan 8, 2024

SakiTakamachi commented Jan 8, 2024

kocsismate commented Jan 8, 2024

SakiTakamachi commented Jan 8, 2024

kocsismate commented Jan 8, 2024

nielsdos commented Jan 8, 2024

kocsismate commented Jan 9, 2024 • edited Loading

nielsdos commented Jan 9, 2024

kocsismate commented Jan 11, 2024

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iluuu1994 commented Jan 13, 2024

SakiTakamachi commented Jan 13, 2024

kocsismate commented Jan 14, 2024

iluuu1994 commented Jan 14, 2024

janedbal commented Feb 21, 2024

SakiTakamachi commented Feb 21, 2024

kocsismate commented Nov 27, 2023 •

edited

Loading

kocsismate Jan 7, 2024 •

edited

Loading

kocsismate commented Jan 9, 2024 •

edited

Loading