Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for PHP 8.4 #228

Open
srjlewis opened this issue Oct 3, 2024 · 9 comments
Open

Support for PHP 8.4 #228

srjlewis opened this issue Oct 3, 2024 · 9 comments

Comments

@srjlewis
Copy link
Contributor

srjlewis commented Oct 3, 2024

Hi

I am doing some eairly testing with PHP 8.4

I have found a conflict within the library

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

It is a result of a accepted PHP RFC https://wiki.php.net/rfc/pdo_driver_specific_subclasses which introduces a static connect method PDO::connect() as shown in the above method.

Here is lavavel's fix within there library https://github.com/laravel/framework/pull/52538/files

It looks like ExtendedPdoInterface::connect() would need renaming to somthing along the lines of ExtendedPdoInterface::autoConnect() and throughout the rest of the library.

I would then think ExtendedPdo:autoConnect() would looke somthing like.

class ExtendedPdo extends AbstractExtendedPdo
{
    public function autoConnect(): void
    {
        if ($this->pdo) {
            return;
        }

        // connect
        $this->profiler->start(__FUNCTION__);
        list($dsn, $username, $password, $options, $queries) = $this->args;
        if(version_compare(phpversion(), '8.4.0', '<')) {
            $this->pdo = new PDO($dsn, $username, $password, $options);
        } else {
             $this->pdo = PDO::connect($dsn, $username, $password, $options);
        }
        $this->profiler->finish();

        // connection-time queries
        foreach ($queries as $query) {
            $this->exec($query);
        }
    }
}

This would need to be a new majer version due to the big BC break ExtendedPdoInterface::connect() being public.

I can create a MR for this if needed.

@koriym
Copy link
Member

koriym commented Oct 4, 2024

It seems reasonable and appropriate to me. cc/ @pmjones @harikt @frederikbosch

@harikt
Copy link
Member

harikt commented Oct 4, 2024

Boooom.. @srjlewis Thank you for bringing this up and also nice pointers. We can make this next major version, but I am not sure what should we call this. I don't like the name autoConnect because it is not connecting automatically. Also this is a manual call. Any other alternatives you have in mind ?

Some of the methods came to my mind are createConnection, makeConnection, connectToDb.

@harikt
Copy link
Member

harikt commented Oct 4, 2024

PR #229

@srjlewis
Copy link
Contributor Author

srjlewis commented Oct 4, 2024

@harikt I do agree with the naming, I used autoConnect as an example, and can easily be changed, apart from this issue I have not run into any other issues with PHP 8.4

@frederikbosch
Copy link
Contributor

With driver specific subclasses in PHP, I think we should re-evaluate more than just the new PDO::connect method. Driver-specific classes are available in this library since #138 was merged. To what extend is that superseded in PHP 8.4? Should we not drop that if it is included in PHP itself?

If so, would it then not be better to drop < 8.4 for a new version 6.0? And just use override the connect method in order to support lazy connections? And, then maybe use an option for direct connections?

<?php

class ExtendedPdo
{
    public const CONNECT_IMMEDIATELY = 'immediate';

    protected array $args = [];
    protected bool $driverSpecific = false;

    public function __construct(
        string $dsn,
        ?string $username = null,
        ?string $password = null,
        array $options = [],
        array $queries = [],
        ?ProfilerInterface $profiler = null
    ) {
      // if no error mode is specified, use exceptions
        if (! isset($options[PDO::ATTR_ERRMODE])) {
            $options[PDO::ATTR_ERRMODE] = PDO::ERRMODE_EXCEPTION;
        }

        // retain the arguments for later
        $this->args = [
            $dsn,
            $username,
            $password,
            $options,
            $queries
        ];

        if ($options[self::CONNECT_IMMEDIATELY]) {
             $this->establishConnection();
        }

        // keep the rest of the constructor
    }

    #[\Override]
    public static function connect(string $dsn, string $username = null, string $password = null, array $options = null) {
    {
          $pdo = new self($dsn, $username, $password, $options);
          $pdo->driverSpecific = true;
          return $pdo;
    }

    // call this method from query, perform, execute etc
    private function establishConnection(): void
    {
       if ($this->pdo) {
            return;
       }

       if ($this->driverSpecific) {
            $this->pdo = PDO::connect($dsn, $username, $password, $options);$password, $options);
        } else {
            $this->pdo = new PDO($dsn, $username, 
        }
    }
}

@srjlewis
Copy link
Contributor Author

srjlewis commented Oct 4, 2024

@frederikbosch we could approch the issue has you mention, but I see a few issues in your approach.

  1. it would make the libaray PHP 8.4 only, I was trying to keep compatability with prior versions of php
  2. You have removed the compatability of initilizing the object with a profiler in the connect method

but by all means keep the ideas flowing

@frederikbosch
Copy link
Contributor

frederikbosch commented Oct 4, 2024

  1. Why keep the compatibility if you go to a version 6? People can use v5 for compatibility?
  2. That's because it's an example, not a PR. I would leave that in for sure.

@srjlewis
Copy link
Contributor Author

srjlewis commented Oct 4, 2024

It will help with a smooth migration/update path

We can release v6.0 and users can implement it before the release of PHP 8.4

This would also allow users to run PHP 8.x in production and do testing with PHP 8.4 before its release.

This is what I am doing at the moment, yes I know users can use some composer magic but that makes it more complicated for the users.

I also think it is a good idea to keep compatability with all supported version of PHP and not make users switch between versions of the libaray.

@frederikbosch
Copy link
Contributor

I disagree with that.

  1. Why rush to release before 8.4?
  2. There is no composer magic needed: if your are on PHP < 8.3, you use version 5 of AuraSQL, if you are going to use PHP 8.4, you use version 6 of AuraSQL. I don't see problem with that. That's no magic, that's how version management works.

Moreover, that is why there is semantic versioning: it's a contract how to deal with backward compatibility. When, you change the major version, you are going to break things. Now, with PHP 8.4 compatibility will be broken anyhow since the new connect method will force us to break things. You can better re-evaluate, and put the library in a mode that is going to proof itself again in the future. Hence, I suggest we slow down, and evaluate whether we need the driver specific parsers included in this library now PHP provides them in the core of PDO. And then see what is the best syntactic solution for immediate or manual connections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants