Driver::connect() should throw only driver-level exceptions

[morozov]

Q	A
Type	improvement
BC Break	yes

Summary

Currently, most of the Driver::connect() throw a DBALException on failure but some throw a DriverException exception. The lower level abstraction (drivers) should not be aware of the higher-level abstraction (DBAL). This way, instead of being implemented in one place, the higher-level logic gets inconsistently duplicated in multiple places.

Additional changes

1. Driver::connect() is documented as @throws DriverException.
2. Connection::connect() is documented as @throws DBALException.
3. Connection::getDatabasePlatformVersion() will not expect Connection::connect() to throw anything else than a DBALException.

Testing

The code being changed is covered by existing tests (e.g. ExceptionTest::testConnectionException()). It doesn't look reasonable to add the unit tests that would cover the specific thrown exception types. Ideally, this should be tested statically (e.g. PhpStorm supports that but I'm not sure if other tools that can run on CI do).

[greg0ire]

The patch coverage is ~ 50%… uncovered code seems to be related to OCI8, SqlAnywhere and Oracle. Do you think this is expected? Or is the AppVeyor report maybe not working properly?
0% coverage for SQLAnywhere\Driver::connect() looks really fishy, but I have to check which ones are supposed to be run on AppVeyor, and which ones on continuousPHP

[morozov]

The patch coverage is ~ 50%… uncovered code seems to be related to OCI8, SqlAnywhere and Oracle. Do you think this is expected?

Yes (#4075, #4076).

Or is the AppVeyor report maybe not working properly?

AppVeyor only covers SQL Server, not SQLAnywhere.

0% coverage for SQLAnywhere\Driver::connect() looks really fishy.

It's not tested anywhere and never was. We need to reconsider whether we want to support it. Unlike other commercial platforms like Oracle and DB2, there's no container available for testing. The driver is also not available on PECL but should be downloaded from their website (needs to be googled).

ContinuousPHP cannot send coverage to anything else than Scrutinizer. We took that into account when we migrated from it. I'd love to migrate from ContinuousPHP too but there's no clear path currently.

[greg0ire]

It's not tested anywhere

I'm late, but I see what you did here :)