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

Error when using Sqlite #304

Open
andylibrian opened this issue Oct 19, 2015 · 6 comments
Open

Error when using Sqlite #304

andylibrian opened this issue Oct 19, 2015 · 6 comments

Comments

@andylibrian
Copy link
Contributor

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
Copy link
Member

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
Copy link

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

@andylibrian
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Member

dbu commented Oct 20, 2015

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
Copy link
Member

dbu commented Jan 11, 2016

#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?

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

5 participants