Correct behaviour to match PHP 8.4 PDO returned types for connect and __construct for the internal pdo object

[srjlewis]

I have done some changes to keep the behaviour the same as the PDO calls with the correct returned types for the pdo object.

Thanks to @frederikbosch as I took some of the ideas from #228 (comment), hope you don't mind

[harikt]

I thought we don't need the connect method itself as this is implemented internally in the PDO.

[srjlewis]

I was looking at the final release of the new PDO

PDO::__construct() always returns PDO and PDO::connect() returns Pdo\{driver} or PDO depending on available drivers

So if you use ExtendedPdo::getPdo() you would get the correct typed object.

[harikt]

That behaviour of PDO is expected. But in our case we are not calling the parent construct.

[aik099]

I'm getting this error when trying to use this library on PHP 8.4 ( https://github.com/console-helpers/db-migration/actions/runs/12323306770/job/34398667606 ):

PHP Fatal error:  Cannot make static method PDO::connect() non static in class Aura\Sql\AbstractExtendedPdo in .../vendor/aura/sql/src/AbstractExtendedPdo.php on line 161

I'm hoping this PR will help with that.

[srjlewis]

@aik099 what version of PHP are you using and what does your composer file look like?

[aik099]

@aik099 what version of PHP are you using and what does your composer file look like?

@srjlewis , I'm using PHP 8.4.

The composer.json file content is ( from https://github.com/console-helpers/db-migration/blob/master/composer.json ):

{
	"name": "console-helpers/db-migration",
	"description": "Database migrations made simple",
	"keywords": ["database", "migration", "sqlite"],
	"license": "BSD-3-Clause",
	"authors": [
		{
			"name": "Alexander Obuhovich",
			"email": "[email protected]"
		}
	],
	"require": {
		"php": ">=5.6",
		"aura/sql": "^2.5 || ^3.0 || ^4.0 || ^5.0"
	},
	"require-dev": {
		"aik099/coding-standard": "dev-master",
		"yoast/phpunit-polyfills": "^2.0",
		"phpspec/prophecy": "^1.10",
		"console-helpers/prophecy-phpunit": "^3.0"
	},
	"autoload": {
		"psr-4": {
			"ConsoleHelpers\\DatabaseMigration\\": "src/DatabaseMigration/"
		}
	},
	"autoload-dev": {
		"psr-4": {
			"Tests\\ConsoleHelpers\\DatabaseMigration\\": "tests/DatabaseMigration/"
		}
	},
	"extra": {
		"branch-alias": {
			"dev-master": "0.1.x-dev"
		}
	}
}

[srjlewis]

@aik099 you would need to change your composer to

"require": {
	"php": ">=5.6",
	"aura/sql": "^2.5 || ^3.0 || ^4.0 || ^5.0||dev-6.x"
}

this will use 6.x branch that is compatable with PHP 8.4.

I awaitig #230 being merged which would allow swapping between aura/sql versions, this is only targeting 5.x branch but can be backported to previous version.

but yes this PR would help aswell to some degree in your case.

All this is due to PHP 8.4 introducing PDO::connect() this is the issue regarding the changes #228

I have both of these PR's in my personal repo and can easily switch between versions.

@harikt please could you review these PR's and tag the 6.x branch for release, it would be helpful for PHP 8.4 users.

[aik099]

@srjlewis , thank you for an explanation. I now understand why it's failing. I'll wait for Aura/Sql 6.x release. We're not switching to PHP 8.4 this year.

[harikt]

@srjlewis yes, I have been checking this. But I am not sure if we need this change or not. @koriym , @frederikbosch can you guys look into this and give some suggestion if we need this specific change ?

1. I think connect method can be removed from ExtendedPDO

   Aura.Sql/src/ExtendedPdo.php

   Line 88 in 0d02dd3

   public static function connect(

2. In this we are passing to options driverSpecific, immediate etc which is not PDO specific and can cause more issues in the future with PDO specific changes.

[srjlewis]

@harikt,

1. We need the ExtendedPdo::connect() method, if we remove it you would be able to call ExtendedPdo::connect() and get a PDO object instead of a ExtendedPdo, so we wrap the connect method so we return a ExtendedPdo object consistently

2. Yes I agree there are other ways to pass the flags, but I did like the idea of how @frederikbosch was added to the options array in Support for PHP 8.4 #228 (comment), also as PHP internals only use INT's as there values, and I am using strings there would be no confilict, and when passed to a PDO object they are just ignored.

[harikt]

can't we actually use [] instead of null, this will also remove the usage of $options ??= []; right ?

[koriym]

Both points of view are reasonable. On one hand, separating Aura.Sql options from PDO options clearly makes future maintenance and understanding the codebase easier. On the other, as @srjlewis notes, the current approach likely won’t cause immediate technical issues since PDO ignores unknown string-based options. It’s a matter of prioritizing long-term clarity versus practical immediacy.

[srjlewis]

@koriym @harikt thanks for your input and concerns

Just to explain other considerations of why I took this approch.

1. I did not want to change the constructor signature and introduce a BC break.
2. There was already a mechanism in place for passing options.
3. If we did introduce a second set of options in the constructor, this would be really confusing for users.

To address the main concerns.

1. PHP switch to using string keys (very unlikely due to the C implementation) and pick the same keys (even more unlikely due to the fact that PDO connects straight away and uses Pdo::connect() for driver specific decisions), but address the issue I have namespaced with a prefix on the static constaints to mitigate this issue.
2. PHP starts to check and raise errors/exceptions on invaild options that are passed, I would think this would be the most likely scenario, I have addressed this issue buy unsetting the custom option upon use and are NEVER sent to PDO on instantiation, so would not create any internal problems for PDO

[pmjones]

PHP adding a static connect() method as a factory has created quite the mess. And the introduction of PDO subclasses changes the foundations on which Aura.Sql was built so many years ago.

As @koriym says, "It’s a matter of prioritizing long-term clarity versus practical immediacy." That is, the big question is, does Aura.Sql need a more thorough rebuilding from the ground up on the new 8.4 changes?

Let's say for 6.x the answer is "no" and that some shorter-term patching-over is the plan. If that's the case, then here are some observations and commentary, all up for discussion:

• PdoInterface is not up-to-date with the 8.4 PDO; I would think it needs at least the static connect() method, as well as any other changes introduced by the 8.4 PDO.

• Whereas ExtendedPdo::connect() returns an ExtendedPdo instance, DecoratedPdo::connect() returns a PDO instance proper (or PDO driver class instance) -- seems to me it should instantiate PDO, then return a DecoratedPdo with that instance ... ?

• As far as a replacement method name for connect(), that's a tough one. I do wish PHP had used a more common name for the factory method, whether new() or create() or make(), but we're stuck now. establlishConnection() seems longish to me. Would _connect() as a replacement be too ugly? If so, maybe lazyConnect() would do. And must it remain public? Might be fine to make it protected.

• I don't especially like the added constants and the behavior that goes with them. Are they really necessary? I would expect DecoratedPdo to be an immediate connection (since the PDO instance has to exist already) and ExtendedPdo to defer the underlying instance creation to the lazy-connection method.

• Finally, I think the lazy-connection method should not choose between PDO::connect() and new PDO(). It should do only one, or the other. If a consumer wants a specific kind of PDO connection, they can use DecoratedPdo with a connection they created themselves. The question then is, should ExtendedPdo use PDO::connect() for a driver-specific instance when available, or new PDO() to always and only get a PDO instance proper? Previously it's been a PDO instance proper, and continuing with that makes sense.

Seems like even a patching-over is a big deal.

Your thoughts, all?

[srjlewis]

Hi @pmjones

Sorry I did miss the PdoInterface, my bad.

As for DecoratedPdo, some direction would be great, I did think about it, but was not sure on direction.

as for establlishConnection(), there is no offical releases with that name in yet so could be easily changed, and I think everyone has a different naming idea and I am not too fussed what it's called, just as long as everyone is happy with it, but personaly I would keep it public due to the fact it is the call users can use to force a connection.

The current 6.x branch only uses one method of connecting within establlishConnection(), which I dont mind, this current PR was submitted separately, as it is not required or needed for the 6.x branch to work with PHP 8.4, I only submitted it for correctness, so I am not fussed if it is used or not, but is only for matching behavior.
Personaly I would not use this PR, I would change line.

Aura.Sql/src/ExtendedPdo.php

Line 114 in 820ea99

$this->pdo = PDO::connect($dsn, $username, $password, $options);

to

$this->pdo = new PDO($dsn, $username, $password, $options)

and not use driver spacific driver classes as this would allow use of the 6.x branch with all supported versions of PHP.

I would also understand if version 6.x was rebuilt with PHP >= 8.4 in mind, but I would leave that to you guys as you know what direction you want go.

I would like to see a agreed upon version of this lib for PHP 8.4 sooner than later, as users are starting to use PHP 8.4 and the 5.x branch crashes on PHP 8.4, my dev and staging enviroments are already PHP 8.4 and my production servers are 8.3, so the usage is already there.

I do feel like I am on a one man mission to get this lib to be 8.4 ready, and it is not huge changes to do so, but I do feel it has been a up hill struggle to do so.

So any input or help would be great to get this over the line.

[harikt]

Hey thanks for your valuable feedback @pmjones .

I have tried to incorporate what you were mentioning in the comments. If all agree we can move with that. Some doc changes may be needed.

#233