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

PDO according to 8.4 changes and feedbacks from @pmjones #233

Merged
merged 4 commits into from
Jan 22, 2025
Merged

Conversation

harikt
Copy link
Member

@harikt harikt commented Dec 21, 2024

Based on comments by @pmjones

Sorry about the formatting messed.

@pmjones
Copy link
Member

pmjones commented Dec 30, 2024

@harikt I have not reviewed yet (sorry) but I wonder if it would make sense to use Lazy (Ghost) Objects for the lazy-connection aspects of this?

https://www.php.net/manual/en/language.oop5.lazy-objects.php

@harikt
Copy link
Member Author

harikt commented Dec 31, 2024

Hi @pmjones ,

According to my understanding ExtendedPdo is still using the lazy way to connect unless we are calling the static method. Are you suggesting the lazy for connect if so I don't think so. The user may need to connect that may be the reason they are calling.

@pmjones
Copy link
Member

pmjones commented Jan 5, 2025

I just tried this:

<?php
class LazyPdo extends PDO
{
    protected PDO $pdo;

    public function __construct(...$args)
    {
        $r = new ReflectionClass($this);
        $r->resetAsLazyGhost(
            $this,
            [$this, 'lazyConnect'],
        );
    }

    public function lazyConnect(...$args)
    {
        $this->pdo = new PDO(...$args);
    }

    public function __call(string $func, array $args)
    {
        return $this->pdo->$func(...$args);
    }
}

$lazyPdo = new LazyPdo('sqlite::memory:');
var_dump($lazyPdo);

But when running, PHP raises the error Cannot make instance of internal class lazy: LazyPdo inherits internal class PDO -- so it looks like classes provided by PHP itself cannot be made ghosts.

Still thinking about this, though; I feel like there must be a way to use the PHP laziness functionality, even if it means exposing the lazy-connection method.

@pmjones
Copy link
Member

pmjones commented Jan 7, 2025

@harikt I begin to wonder if keeping the lazy-loading aspect makes sense any more. I ended up dropping it from Atlas.Pdo because you only create the PDO instance when you need it, e.g. when you pull it from a DI container.

Your thoughts? @srjlewis, others?

@srjlewis
Copy link
Contributor

srjlewis commented Jan 7, 2025

@pmjones I used to use the lazy loading for read/write splitting on master/slave DB clusters, and it works great, but as time moves on I now use maxscale and mariaDB Galera Cluster and remove that type of logic from the application level.

I think there a still are aguments for lazy loading that people can make, but from todays use of design patterens, there are 101 others ways to get the same results, i.e. using a connection location class to lazy load when needed.

So overall I dont think lazy loading is as useful as it once was with DB connections.

But if people needed to, they can extend the class, and it is not huge changes to do so, I think we still need to be mindful that uses can and will extend the PDO wrapper, so I think any change should keep this in mind.

I looks like PHP lazy objects will never work in this use case, as we extend PDO, as @pmjones points out

It is possible to create a lazy instance of any user defined class or the stdClass class (other internal classes are not supported)

@pmjones
Copy link
Member

pmjones commented Jan 7, 2025

Thasnk @srjlewis. @harikt if you're ok with dropping lazy connections, I'm ok with it too.

@harikt
Copy link
Member Author

harikt commented Jan 8, 2025

@pmjones do you mean removing the lazyConnect entirely on newer versions ? I agree to the points regarding lazy. But I would prefer keeping the lazyConnect method as is for now.

@harikt
Copy link
Member Author

harikt commented Jan 8, 2025

If all is well, we can do a release of 6.x in coming weeks.

@Morthy
Copy link

Morthy commented Jan 21, 2025

Really looking forward to seeing a 6.0 release with PHP 8.4 support if everything is fine with this!

@harikt harikt merged commit 8b2ca95 into 6.x Jan 22, 2025
2 of 3 checks passed
@harikt harikt deleted the issue-231 branch January 22, 2025 06:00
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

Successfully merging this pull request may close these issues.

4 participants