Connection is not lazy anymore, since platform detection was introduced - exposing the issue via broken test

[weaverryan]

Hi guys!

This is not meant to be merged, at least not yet. The test added in #691 is flawed. It's failing NOT because there is an exception when trying to use a closed connection (in fact, if the connection is closed, it simply re-opens), but instead, the exception is:

Access denied for user 'root'@'localhost' (using password: YES)

The tests should show this. The reason is that we're using connection parameters at the top of this class (https://github.com/doctrine/dbal/blob/master/tests/Doctrine/Tests/DBAL/ConnectionTest.php#L19) that, until now, were never used to actually connect to the database. But now, they are being used to connect to the database, but they're incorrect - they should be pulling from TestUtil.

So, this test needs to be fixed or removed.

Thanks!

[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-1130

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

[weaverryan]

Here's the proof that the test is causing an exception, but only because the password is incorrect - not because there's some sort of "Connection is Closed" exception: https://travis-ci.org/doctrine/dbal/jobs/48084218#L298

[Ocramius]

What happens if the platform cannot be fetched? This code will likely cause a method call on null, according to #783.

[Ocramius]

IMO, this should still throw an exception if the $platform is null (connection is re-opened, platform is re-detected, platform is null due to failed automatic detection)

The exception type may change.

[weaverryan]

Actually, $this->platform will never fail being detected: https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Connection.php#L386.

But more importantly, this test was added to test this change: https://github.com/doctrine/dbal/pull/688/files#diff-7b1e94fb7dd408985f7b7973330dc2edL600 (you can see this referenced from #691 where this change came from).

Basically, the test was to check to make sure that if the connection is closed, we don't end up getting any "property doesn't exist" type errors. I'm not sure that's something that can be tested, and I can't see how this test was ever valid. So, either this test should be removed (because it's totally not testing what it was intended to test) or I'm completely missing the purpose of this test. Both are possible :)

[deeky666]

I guess the only way to properly test this without side effects is by using reflection. @Ocramius what do you think? Any other idea?

[deeky666]

Okay seems there are no other suggestions so I would accept using reflection here to do the assertion. @weaverryan can you do that?

[deeky666]

Agree that the test should be rewritten somehow. The test is very risky and highly depends on implementation detail. I guess either Reflection has to be used to test the internal state of Connection::$_conn or it has to be moved to the functional test suite.
Speaking of that, the credentials defined in this test class are only there so that the driver / driver manager does not throw an exception because of missing params. In the end this test class is not intended to do real database interaction.

[weaverryan]

@deeky666 I agree on all points! Now that we're connecting when the platform is being determined, we do now try to connect in some cases, so we have to be careful (since, as you said, it was never the real intention). I'd really like your thoughts on #783 (other than the removal of this test in that PR - I can rewrite it with reflection).

Thanks!

[weaverryan]

Done! Summary:

1. This test was introduced for Fixed closing a connection #688 in Add test case for #688 #691.
2. My implementation is taken from one of the attempts on Add test case for #688 #691
3. I have properly tested that the test DOES fail if the inner _conn is not nullified

The only issue is that this test should technically test that the property was nullified, and not unset. But, I'm not actually sure if that's testable - even with reflection. Even when I unset the property temporarily, Reflection sees it just fine (since it's in the class declaration).

My opinion (fwiw) is that we merge this, or do-away with the test entirely. But I think we should keep it.

Thanks!

[deeky666]

@weaverryan thanks for your investigation! IMO if not even reflection can detect whether a property was unset, this test is rather useless. As you stated this test does not fail if the property was unset so it is not a failing test case for the original issue.
Another idea I have is to create a mock class for this particular test case which extends the connection class and adds an additional method that directly accesses $this->_conn like this:

class DBAL1003Connection extends \Doctrine\DBAL\Connection
{
    public function getWrappedConnection()
    {
        return $this->_conn;
    }
}

class ConnectionTest extends \Doctrine\Tests\DbalTestCase
{
    $connection = new DBAL1003Connection(); // params to be inserted

    $connection->close();

    $this->assertNull($connection->getWrappedConnection());
}

I think this should raise the notice and reveal the bug.

[deeky666]

Or we simply mock Doctrine\DBAL\Connection and make connect() do nothing. This could have the same effect.

[deeky666]

Yep that reveals the issue:

public function testConnectionIsClosed()
{
    $connection = $this->getMockBuilder('Doctrine\DBAL\Connection')
        ->disableOriginalConstructor()
        ->setMethods(array('connect'))
        ->getMock();

    $connection->close();

    $this->assertNull($connection->getWrappedConnection());
}

[weaverryan]

@deeky666 excellent suggestion - I've updated the test (I'm still using reflection to set _conn internally - then this will fail if _conn is unset or if we forget to set it to null on close()).

I've verified that unset($this->_conn) or forgetting $this->_conn = null both make the test fail now.

Thanks!

[deeky666]

@weaverryan finally merging, thanks again!