jackalope / jackalope-doctrine-dbal

Doctrine DBAL transport implementation for Jackalope
http://jackalope.github.io
Other
143 stars 60 forks source link

Error when using Sqlite #304

Open andylibrian opened 9 years ago

andylibrian commented 9 years ago

I am writing a Laravel package that wraps jackalope doctrine dbal. In my test, I use pdo sqlite driver, and the test failed with following error:

Argument 1 passed to Jackalope\Transport\DoctrineDBAL\Client::registerSqliteFunctions() must be an instance of Doctrine\DBAL\Driver\PDOConnection, instance of PDO given, called in /home/travis/build/ramping/laravel-jackalope-doctrine-dbal/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php on line 2631 and defined

/home/travis/build/ramping/laravel-jackalope-doctrine-dbal/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php:214

/home/travis/build/ramping/laravel-jackalope-doctrine-dbal/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php:2631

/home/travis/build/ramping/laravel-jackalope-doctrine-dbal/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php:266

/home/travis/build/ramping/laravel-jackalope-doctrine-dbal/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php:429

/home/travis/build/ramping/laravel-jackalope-doctrine-dbal/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php:389

/home/travis/build/ramping/laravel-jackalope-doctrine-dbal/vendor/jackalope/jackalope/src/Jackalope/Repository.php:145

Here is the build : https://travis-ci.org/ramping/laravel-jackalope-doctrine-dbal/jobs/86153727

Apparently, it's because of this:

    /**
     * @TODO: move to "SqlitePlatform" and rename to "registerExtraFunctions"?
     *
     * @param PDOConnection $sqliteConnection
     *
     * @return Client
     */
    private function registerSqliteFunctions(PDOConnection $sqliteConnection)
    {

After I read these lines:

        // @TODO: move to "SqlitePlatform" and rename to "registerExtraFunctions"?
        if ($this->conn->getDatabasePlatform() instanceof SqlitePlatform) {
            $this->registerSqliteFunctions($this->conn->getWrappedConnection());
        }

I tried to change it to

private function registerSqliteFunctions(\PDO $sqliteConnection)

Then the test passed. Any idea? Is this something related to that "TODO"?

lsmith77 commented 9 years ago

hmm this is interesting .. might be a BC break in Doctrine 2.5?

@Ocramius do you have an idea why he is not getting an instance of PDOConnection from Doctrine DBAL but a raw PDO object?

Hmm I guess the reason is that Laravel is setting the PDO connection as a parameter maybe?

@andylibrian can you make the change to \PDO in a PR so that we can see if the test suite still passes .. we might indeed simply require a too strict type hint here.

Ocramius commented 9 years ago

Any git changelogs for that type hint? I don't remember this changing since 2.4.x

andylibrian commented 9 years ago

@lsmith77 I just sent the PR and it seems like the test suite still passes. @Ocramius I browsed through the log of this repo and the type hint was already there since the first time.

I felt determined so I toke a look at Doctrine DBAL and found this, what do you guys think about it?

The error from jackalope was because $this->conn->getWrappedConnection() returns PDO instead of DBAL PDOConnection (which the type hint requires):

        // @TODO: move to "SqlitePlatform" and rename to "registerExtraFunctions"?
        if ($this->conn->getDatabasePlatform() instanceof SqlitePlatform) {
            $this->registerSqliteFunctions($this->conn->getWrappedConnection());
        }

in Doctrine\DBAL\Connection::getWrappedConnection()

    /**
     * Gets the wrapped driver connection.
     *
     * @return \Doctrine\DBAL\Driver\Connection
     */
    public function getWrappedConnection()
    {
        $this->connect();

        return $this->_conn;
    }

It says it should return \Doctrine\DBAL\Driver\Connection from $this->_conn, but I saw this in constructor:

    public function __construct(array $params, Driver $driver, Configuration $config = null,
            EventManager $eventManager = null)
    {
        $this->_driver = $driver;
        $this->_params = $params;

        if (isset($params['pdo'])) {
            $this->_conn = $params['pdo'];
            $this->_isConnected = true;
            unset($this->_params['pdo']);
        }

$this->_conn is set from $params['pdo'] which is a PDO instance.

That might be the case? In my Laravel package, I indeed reuse the existing PDO connection (from Laravel Service Provider) and pass it to Doctrine DBAL as 'pdo' param.

cryptocompress commented 9 years ago

Is this something related to that "TODO"?

This TODO is a reminder to clean up those "if PLATFORM then" cascades. Not related to this issue.

dbu commented 9 years ago

so there is some regression in dbal it seems. but we do need a method from PDO actually in that place. commented on the pull request.

@andylibrian great that you build a laravel module, by the way! once it is working, please do a PR to the https://github.com/phpcr/phpcr.github.io/ so that we list the module in the integrations section of the website!

dbu commented 8 years ago

306 has been merged and is included in the latest stable tag. is there anything left to do here? @andylibrian did you get around to create that laravel module?