Enhancement: expose ping() on pingable-connections.

[till]

This PR introduces a new PingableConnection interface and exposes
a ping method on \Doctrine\DBAL\Connection. For drivers where no
ping is supported, a \Doctrine\DBAL\ConnectionException is thrown.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-666

We use Jira to track the state of pull requests and the versions they got
included in.

[stof]

braces around the instanceOf check are not needed.

However, spaces around the ! are needed according to the coding standards

[till]

if ( ! ($this->_conn instanceof PingableConnection) ) {

or

if (false === ($this->_conn instanceof PingableConnection)) {

?

[beberlei]

first one, but without the ending space, just:

if ( ! ($this->_conn instanceof PingableConnection)) {

[stof]

@beberlei are you using the brances around the instanceof ? I would have said

if ( ! $this->_conn instanceof PingableConnection) {

but maybe I'm doing a mix between the Doctrine CS and the Symfony ones (which are the one I use all the time), where it would be if (!$this->_conn instanceof PingableConnection) {

[stof]

is it something which can be supported by other drivers too ?

[till]

@stof I don't know — I think you can fake it with PDO, e.g.:

try {
    $pdo->query('SELECT 1');
} catch (\Exception $e) {
    $this->connect_or_reconnect_code_here();
}

[deeky666]

I like the idea but I have not seen an implementation like mysqli::ping() in any other driver. At least we should find a way to make that possible in drivers that do not natively support such a method. Another idea would be to integrate that into the Connection interface instead of having a seperate PingableConnection interface? But only if that BC break is acceptable and if we find a way to implement that in other drivers, too.

[deeky666]

New line before return, please.

[till]

@deeky666 semantic versioniong and all, BC breaks for Doctrine 3.0 only, right? ;-)

IMO, this could be refactored later when a release date is closer. What do you guys think of the SELECT 1 idea in my previous comment?

[deeky666]

@till Well, there was a recent change in the driver interface, too ;) Some BC breaks are acceptable but that is up to @beberlei. As not many people are creating their own drivers/connections, this MIGHT be ok.
The SELECT query could be an option...

[stof]

The way to reconnect with PDO is to close the connection and instantiate a new one. So this would have to be implemented in Doctrine\DBAL\Connection itself. a solution could be like this:

    /**
     * Ping the server!
     *
     * @return bool
     */
    public function ping()
    {
        if ( ! $this->_isConnected) {
            return; // Don't connect if not done yet. It will be lazy on first use
        }

        if ($this->_conn instanceof PingableConnection) {
            $this->_conn->ping(); // TODO handle the case of MySQLi failures in ping(). what are they ?
            return;
        }

        try {
            $this->query('SELECT 1');
        } catch (DBALException $e) {
            // As the underlying connection is unset, the next query will connect again thanks to the lazyness
            $this->close();
        }
    }

[deeky666]

@stof Looks like a reasonable implementation. But instead of $this->query('SELECT 1') I would use $this->query($this->_platform->getDummySelectSQL()) to ensure that a platform specific query is fired as I am not sure that will definitely work on all platforms.

[till]

@stof In case @beberlei doesn't object to the implementation, just PR to my branch and I'll include it.

[stof]

@deeky666 indeed, this is a good idea. I forgot that we have such method in the platform

[beberlei]

@stof Yes I was wondering myself that PDO cant possible implement this completly in the Driver.

I would do it a bit differently by checking for the return value of ping on the driver and close if false, then have PDO do the SELECT1 and return false on exception. Also the check for isConnected has to return a boolean, as the API specifies bool as return value.

[deeky666]

@stof At least then this method in the platform would have an actual use case :D

[till]

Hmm, thinking about this, I propose the following:

1. I'll add ping() to \Doctrine\DBAL\Driver\PDOConnection
2. Make \Doctrine\DBAL\Driver\PDOConnection implement PingableConnection
3. Have a 🍺 or 🍻

Thoughts?

[till]

So PDOConnection didn't work. No 🍺 for me.

[stof]

As you implement the method, I would implement PingableConnection on this class too

[beberlei]

no, this is the main connection and not connected (despite naming) with the real connections.

[till]

🙇 Thanks to everyone for the help!

[stof]

@beberlei see the comments here: #414 (comment)

the behavior of this method is inconsistent currently between MySQLi and others, with a different meaning for the false return value (making it unusable). Please give your mind on my proposal of a better implementation

[till]

@stof IMO, people who use mysqli will set the reconnect option and then it's consistent. It's up to the implementation in the code around it to handle the false. I don't want the same hack we used for PDO in the mysql driver.

[stof]

@till the point is that reutrn false for MySQLi or returning false for the other drivers has different meaning. false for other drivers in the current implement does not mean it cannot reconnect. It means it is not connected currently (but the next time it will need to connect, it will try it).
and even when using MySQLi, the behavior for false is inconsistent between 2 cases:

• you never used the connection yet, so it is not yet connection. The implementation returns false but you can continue using it. It is not an error.
• the connection timed out and you haven't set reconnect. The connection is now unusable. And the only choices for the calling code are either killing the process (or at least stopping to use the DB in the process) or calling close to have the same behavior than for other drivers

This means that the calling cannot handle false, because it does not know what false means.

Note that all these issues are related to a bad definition of the contracts in your 2 methods.

• Doctrine\DBAL\Connection says it returns a boolean, without saying what the boolean means at all (and for a good reason as even this code does not know what it means)
• PingableConnect says it return true in case it is successful, without explaining what it considers as being successful

[beberlei]

Should be consistent now, everything else is documented.