Pdo subclassing by Danack · Pull Request #11740 · php/php-src

Danack · 2023-07-18T13:31:49Z

Implementation of https://wiki.php.net/rfc/pdo_driver_specific_subclasses

Needs thinking about ini settings.

Added ability to register driver specific classes.

Tests don't currently work due to mssql server not running on my computer.

RECREATE will drop (if needed) before creating

…on the PdoPgsql class.

Made extension loading work and implemented exceptions.

…define name.

Girgias · 2023-07-18T17:06:19Z

+    public static function connect(
+        string $dsn,
+        ?string $username = null,
+        ?string $password = null,


Might fix some of the tests

Suggested change

?string $password = null,

#[\SensitiveParameter]

?string $password = null,

Do you mean php-src/ext/pdo/tests/sensitive_parameter.phpt? If so, I think that might not be my fault, as I haven't touched that test.

But yes, that probably should be there, and a test for it.

Danack · 2023-07-18T18:58:13Z

@Girgias I'm just waiting for CIs to run....They appear to have built, and I think invoking extern ZEND_API also fixed the issue with building the pdo_* extensions as shared.

One thing that is odd is that the PDO class itself now has all the parameters auto-generated....which apparently they weren't before. Which is a bit confusing.

Girgias · 2023-07-18T19:22:32Z

+ZEND_BEGIN_ARG_WITH_RETURN_OBJ_TYPE_MASK_EX(arginfo_class_PDO_connect, 0, 1, PDO|PDOSqlite, 0)
+	ZEND_ARG_TYPE_INFO(0, dsn, IS_STRING, 0)
+	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, username, IS_STRING, 1, "null")
+	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, password, IS_STRING, 1, "null")
+	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, options, IS_ARRAY, 1, "null")
+ZEND_END_ARG_INFO()
+


Yeah I don't know why in a previous iteration the constants were not declared any more in the arg info.

andypost · 2023-07-18T21:06:58Z

+$extension_location = null;
+$locations = [
+    '/usr/lib/aarch64-linux-gnu/mod_spatialite.so',
+    "/usr/lib/x86_64-linux-gnu/mod_spatialite.so"


On Alpinelinux it living in /usr/lib/mod_spatialite.so

ok, thanks.

andypost · 2023-07-18T21:09:26Z

+Loading non-existent file.
+Unable to load extension '/this/does/not_exist'
+Loading invalid file.
+Unable to load extension '%s: cannot open shared object file: No such file or directory'


On Alpinelinux it fails with

========DIFF======== Loading non-existent file. Unable to load extension '/this/does/not_exist' Loading invalid file. 004- Unable to load extension '%s: cannot open shared object file: No such file or directory' 004+ Unable to load extension 'Error loading shared library /mnt/testing/php83/src/php-src-791ca5d1dbc5cbb4a76dbb9942dd78860827264f/ext/pdo_sqlite/tests/subclasses/pdosqlite_load_extension_failure.php.so: No such file or directory' Fin. ========DONE========

Ok thanks. I relaxed the test criteria so it should pass, but it's probably not super great that the error message apparently varies based on SQLite version.

kocsismate · 2023-07-18T21:19:50Z

+/** @generate-class-entries */
+
+/** @not-serializable */
+class PdoMysql extends PDO


I'm wondering if we should use the same casing as the original class: e.g. PDOMySql etc.?

see https://chat.stackoverflow.com/transcript/message/56464097#56464097
and https://chat.stackoverflow.com/transcript/message/56500210#56500210

Exact spelling of class names is an aesthetic choice, so it's fine for everyone to have their own opinion, and normal for people to be able to conjure up arguments that just so happy to support their own gut instinct. Which I will now do.

I personally chose the one in the RFC as Tim convinced me that "ucfirst(strtolower(...)) for acronyms" as PCGOneseq128XSLRR64 is almost as horrible as PdOsQlItE.

I'm also pretty sure that having the class name (not just the namespace) contain Pdo is going to make the class easier to google as PdoMysql will be treated as a single word by search engines, whereas Pdo\Mysql is treated as two words, and will be more likely to bring up all of the existing pages that talk about PDO and MySQL, rather than the new manual page for the new class.

kocsismate · 2023-07-18T21:24:05Z

+
+/** @generate-class-entries */
+
+/** @not-serializable */


Nit: You could add a @strict-properties tag so that it already forbids dynamic properties

Danack · 2023-07-19T14:30:05Z

So, anyone seen a release manager, and is this able to get into PHP 8.3?

As I mentioned the other day I am feeling "not great" from being in constant pain. I can do a tiny bit more to get this across the line, but if people have nits or want to change stuff....can you just do that after merging so I don't have to spend any more time thinking about this? Or if it's not going in 8.3 those changes can wait 6 months.

Either way, I've really close to the limit of what I can contribute for this RFC for the time being.

Girgias · 2023-07-19T15:52:22Z

So I learned this yesterday, by going through our release process file for unrelated reasons but:

The feature freeze for php-src means that we will not accept any new features after the date of the feature freeze. For any RFCs to be included in the new version, they should be discussed and have the voting polls closed no later than the feature freeze date. However, this does not mean the new feature must have a complete implementation by this date.

Following the feature freeze, the focus of work for the new version will be on fixing bugs, writing tests, and completing/polishing all accepted features.

c.f. https://github.com/php/php-src/blob/master/docs/release-process.md#feature-freeze so RMs should be willing to accept this post FF... which now I'm wondering why we were rushing PRs to get them merged before beta1

Danack · 2023-07-19T19:00:13Z

which now I'm wondering why we were rushing PRs to get them merged before beta1

Probably because of trauma related to drama.

Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>

bukka · 2023-08-31T10:49:48Z

Just a note here that it should be fine to accept such changes during beta but we need to draw some line when this can be accepted and for me it is the first RC which signals ABI freeze and has been already tagged. So I don't think this can be accepted to 8.3. It will have to wait till 8.4 so you have plenty of time to finish the implementation.

Girgias · 2023-09-02T12:43:25Z

Just a note here that it should be fine to accept such changes during beta but we need to draw some line when this can be accepted and for me it is the first RC which signals ABI freeze and has been already tagged. So I don't think this can be accepted to 8.3. It will have to wait till 8.4 so you have plenty of time to finish the implementation.

Yeah, I haven't looked at this again but I don't understand the phpdbg issue

tontonsb · 2023-09-07T06:42:23Z

+        ?string $username = null,
+        ?string $password = null,
+        ?array $options = null
+    ): PDO|PDOSqlite {}


|PDOSqlite might be a leftover from initial attempts, I think they all extend PDO now (?)

Danack · 2023-09-10T14:55:29Z

@bukka fyi, I'm actually not that well due to a chronic pain problem. I'm not going to be able to work on this probably this year.

If the PHP project (or PHP Foundation) has a 'contributor has been bus-factored' process, it would be good to activate that. If there isn't a 'bus-factored' policy....maybe it's time for one.

bukka · 2023-09-10T19:20:41Z

@Danack Sorry to hear that. Hope you feel better soon.

I will raise it during our next meeting and we should hopefully get it all sorted out.

Girgias · 2023-12-27T00:38:33Z

Superseded by #12804

Danack and others added 30 commits July 18, 2023 13:02

Adding PDOSQLite in the wrong place.

551ab85

Version with PDOSqlite apparently working. Which seems unlikely.

6fb4c7f

Typo on class name.

770658e

Initial version of PDOSQLite::loadExtension.

ecb641a

Needs thinking about ini settings.

PDOSQLite::blobOpen is working.

f79a562

Improve test to search for sqlite extension location.

28e2a5c

Moved code to a more correct place.

4134358

Added ability to register driver specific classes.

First draft of pdo_dblib extension class.

5df9630

Tests don't currently work due to mssql server not running on my computer.

Added PDOMySql class and a driver specific method.

a3f690a

Initial version of PDOPgSql sub-class

af73177

Added missed file.

2e78541

Copied methods across from PDO class.

fbe86b7

PDO_DBLIB tests are passing.

497e192

All subclasses created, ODBC works.

6d137ff

pdo_firebird subclass is working.

c579fd8

Changed config files name to be not ignored.

b5c92d3

Fix PDOOci tests

8a5bcdd

Fix PDOFirebird test names

da341c2

Use RECREATE for PDOFirebird tests

f184b17

RECREATE will drop (if needed) before creating

Add PDOMysql testing setup helper

d20705f

Copy methods from being the magic implementations, to being declared …

4fc8305

…on the PdoPgsql class.

Standardized class names.

0ebe730

Corrected constants to not have prefix.

0ddc2d2

Made extension loading work and implemented exceptions.

Added note on safety.

b820c99

WS.

0a785cc

Tidying up.

e0652bf

Standardised directory name.

82b7a83

Fix tpyo.

157cd85

Copy function detection in m4 file across from sqlite3, and separate …

abcd936

…define name.

Standardized case.

c6ea6f5

Revert previous change.

d2db467

Girgias reviewed Jul 18, 2023

View reviewed changes

Attempting to fix pdo_firebird symbol problem.

95a2226

Girgias reviewed Jul 18, 2023

View reviewed changes

Comment thread ext/pdo/php_pdo.h Resolved

Danack added 4 commits July 18, 2023 18:47

Back to extern.

46dd89a

Externed some things that is working for pdo_oci...

fd3a4ae

Oh, both.

f21948e

Oh, both.

67e0deb

Remove class constants which apparently are now generated automatically.

67fa7b1

Girgias reviewed Jul 18, 2023

View reviewed changes

andypost reviewed Jul 18, 2023

View reviewed changes

kocsismate reviewed Jul 18, 2023

View reviewed changes

Danack added 2 commits July 18, 2023 23:17

Added alpine location.

c0ff7ea

Relax test criteria.

6d42f7e

jrfnl reviewed Aug 6, 2023

View reviewed changes

Comment thread ext/pdo_firebird/tests/subclasses/pdofirebird_002.phpt Resolved

Update ext/pdo_firebird/tests/subclasses/pdofirebird_002.phpt

e7a1137

Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>

Girgias mentioned this pull request Sep 4, 2023

PDO and START TRANSACTION WITH CONSISTENT SNAPSHOT in MariaDB php/doc-en#2729

Closed

tontonsb reviewed Sep 7, 2023

View reviewed changes

Girgias closed this Dec 27, 2023

	?string $password = null,
	#[\SensitiveParameter]
	?string $password = null,


		/** @generate-class-entries */

		/** @not-serializable */

Conversation

Danack commented Jul 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Danack commented Jul 18, 2023

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!

Danack commented Jul 19, 2023

Uh oh!

Girgias commented Jul 19, 2023

Uh oh!

Danack commented Jul 19, 2023

Uh oh!

Uh oh!

bukka commented Aug 31, 2023

Uh oh!

Girgias commented Sep 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Danack commented Sep 10, 2023

Uh oh!

bukka commented Sep 10, 2023

Uh oh!

Girgias commented Dec 27, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants