DBAL 3.0: suggestions to improve the Driver\Connection interface

[BenMorel]

Q	A
Type	improvement
BC Break	yes
Fixed issues	#3334

Summary

This PR proposes a first batch of changes to the Driver\Connection interface, suggested in #3334:

Return void in beginTransaction(), commit(), rollBack()

Takeaway from the issue:

Because Doctrine forces PDO::ERRMODE_EXCEPTION, it makes no sense IMO to return false on error in any method; we'd rather enforce always throwing exceptions when something goes wrong, which would make the interface consistent with the methods that already guarantee to throw on error, such as prepare() and query().

As it stands, these methods in the PDOConnection can never return false, they always return true or throw an exception, so the bool return type makes no sense in this driver.

Furthermore, I can see that many implementations actually don't respect the current contract, and don't return any value in these methods (DBAL\Connection, MasterSlaveConnection, DB2Connection, ...), so this change makes even more sense.

Type-hint parameters and return types

• beginTransaction(), commit(), rollBack() as described above
• lastInsertId() that always receives a string|null and always returns string
• quote() that should always return string or throw an exception (?)

These changes do not break any tests.

[morozov]

@BenMorel the patch looks reasonable code-wise. Are you planning on fixing failures on Oracle? I'd like to get this in before I finish my project-wide type changes for 3.0.

The IBM DB2 seems caused by not having #3320 in develop. I'll rebase the branch later and you'll have to rebase yours then.

[BenMorel]

@morozov Oracle fixed in 76fd95f.

The first failure was that the test expected lastInsertId() to return an int, where we now always return string:

self::assertSame(1, $this->driverConnection->lastInsertId($sequence));

The second failure, however, expected false when we also return string:

self::assertFalse($this->connection->lastInsertId(null));

If fixed it with assertSame('', ..., but should we document the fact that is returns an empty string when no sequence name is given? Or should we throw an exception in this case?

[morozov]

The first failure was that the test expected lastInsertId() to return an int, where we now always return string:

self::assertSame(1, $this->driverConnection->lastInsertId($sequence));

Makes sense.

The second failure, however, expected false when we also return string:

self::assertFalse($this->connection->lastInsertId(null));

If fixed it with assertSame('', ..., but should we document the fact that is returns an empty string when no sequence name is given? Or should we throw an exception in this case?

It should be an exception. In fact, previously false indicated an exceptional situation (same as in PDO in a non-exception error mode). Please make sure that the new code doesn't rely on strict types disabled. For instance, currently OCI8Connection::lastInsertId() declared as returning a string but returns an int:

dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php

Lines 142 to 156 in 5da6be5

	public function lastInsertId(string $name = null) : string
	{
	if ($name === null) {
	return false;
	}
	$sql = 'SELECT ' . $name . '.CURRVAL FROM DUAL';
	$stmt = $this->query($sql);
	$result = $stmt->fetchColumn();
	if ($result === false) {
	throw new OCI8Exception('lastInsertId failed: Query was executed but no result was returned.');
	}
	return (int) $result;

Please rebase on the latest develop to fix the failure on IBM DB2 and see if there are any PHPStan and PHP_CodeSniffer failures.

[BenMorel]

It should be an exception. In fact, previously false indicated an exceptional situation (same as in PDO in a non-exception error mode).

OK, which kind of exception should it throw? A DriverException, or a BadMethodCallException because as I understand it, an exception can only be thrown when calling the method without a parameter, on platforms that require it?

Please make sure that the new code doesn't rely on strict types disabled.

Fully agree. Should we actually plan to use strict_types everywhere in 3.0? I'm sure there are plently of other places where implicit conversion happens without us knowing. I'd be happy to file another PR for this.

For instance, currently OCI8Connection::lastInsertId() declared as returning a string but returns an int

✔️ This one is fixed.

Please rebase on the latest develop to fix the failure on IBM DB2 and see if there are any PHPStan and PHP_CodeSniffer failures.

• ✔️ rebased on the latest develop
• ✔️ fixed PHPStan failures

About PHPCS failures, unfortunately it does not seem to recognize the existing {@inheritdoc} docblocks, and is full of errors:

Parameter $name has null default value, but is not marked as nullable.

Is there a way to make it understand {@inheritdoc}? Or do you want me to copy/paste the actual docblock from the parent class/interface instead?

[morozov]

OK, which kind of exception should it throw?

OCI8Exception should be fine.

Should we actually plan to use strict_types everywhere in 3.0? I'm sure there are plently of other places where implicit conversion happens without us knowing. I'd be happy to file another PR for this.

I meant a due-dilligence visual check. to prevent new inconsistencies. We'll introduce strict_types across the codebase after #3348.

Is there a way to make it understand {@inheritdoc}? Or do you want me to copy/paste the actual docblock from the parent class/interface instead?

{@inheritdoc} should work just fine. Not sure how exactly it's handled but it worked for me a lot of times. Please don't copy/paste doc-blocks.

[BenMorel]

OCI8Exception should be fine.

Handled in 1a0ff77.

I meant a due-dilligence visual check. to prevent new inconsistencies. We'll introduce strict_types across the codebase after #3348.

Handled in 1cf6e2f. I added a (string) type-cast everywhere we call methods than could return an int.

{@inheritdoc} should work just fine. Not sure how exactly it's handled but it worked for me a lot of times.

It doesn't, though:

FILE: lib/Doctrine/DBAL/Driver/PDOSqlsrv/Connection.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Parameter $name has null default value, but is not
    |       |     marked as nullable.
    |       |     (SlevomatCodingStandard.TypeHints.NullableTypeForNullDefaultValue.NullabilitySymbolRequired)

Failing method:

    /**
     * {@inheritDoc}
     */
    public function lastInsertId(string $name = null) : string

Its parent method:

    /**
     * {@inheritdoc}
     */
    public function lastInsertId(string $name = null) : string

Implemented interface method:

    /**
     * Returns the ID of the last inserted row or sequence value.
     *
     * @param string|null $name
     *
     * @return string
     */
    public function lastInsertId(string $name = null) : string;

Not sure if the problem therefore comes from a wrong case (@inheritdoc vs @inheritDoc), or from the fact that there is a parent class in between the method and the interface that declares the proper types.

[morozov]

Parameter $name has null default value, but is not marked as nullable.

It means that the parameter should be declared as ?string $name = null, not string $name = null. You can run vendor/bin/phpcbf and fix all issues automatically.

[BenMorel]

It means that the parameter should be declared as ?string $name = null, not string $name = null. You can run vendor/bin/phpcbf and fix all issues automatically.

Ah, gotcha. I got somehow convinced that this error was related to improper documentation. Fixed 👍

In Driver\Connection, I prefered to add explicit descriptions of parameters and return types, rather than removing the existing @param and @return annotations. I like very thoroughly documented interfaces, even if it feels redundant sometimes.

Oh, and I explicitly added @throws DriverException on Driver\Connection::lastInsertId(), now that OCI8 throws such an exception when using a null sequence name.

This is a first step towards my suggestion regarding exception handling in #3334.

Anyway, I'm done on this PR now, do you see anything else?

[morozov]

In Driver\Connection, I prefered to add explicit descriptions of parameters and return types, rather than removing the existing @param and @return annotations. I like very thoroughly documented interfaces, even if it feels redundant sometimes.

You're misinterpreting the standard. The standard forbids useless annotations which only contain the variable type which is already specified in the method signature and not containing a description. Meaningful descriptions are more than welcome.

[BenMorel]

You're misinterpreting the standard. The standard forbids useless annotations which only contain the variable type which is already specified in the method signature and not containing a description. Meaningful descriptions are more than welcome.

I got that, and I had 2 choices: removing the annotation, or adding a description to it. I chose the second option!

[BenMorel]

@morozov Do you see any other issue with this PR?

[morozov]

The more I deal with Connection::lastInsertId() (see #3074), the less I like it. I think a significant part of the confusion comes from the fact that the calls with and without the $name are targeted at the same method but have totally different semantics (named sequences and identity columns). This confusing API was originally borrowed from PDO but we don't have to follow it strictrly.

What if you drop Connection::lastInsertId() from this pull request so that we can focus on what's already clear?

[morozov]

The connection is established in the constructor. If there's a run-time error, it shouldn't be hidden but instead reflected in the exception.

[BenMorel]

From the docs, it looks like this method can only return an error (false) "if the $conn is not valid". So IMO the error message should just reflect that (for example if the connection was closed in the meantime).

[morozov]

An invalid connection may have many reasons behind it. If it was valid at the construction time but isn't anymore, I want to know the exact reason (should be available via SQLAnywhereException::fromSQLAnywhereError($this->connection)).

[BenMorel]

I didn't know about this method, fixed!

[BenMorel]

You're right about lastInsertId(), it could be split into 2 methods (lastInsertId() and getSequenceNumber()?), this would clarify the intent a lot!

I can definitely remove lastInsertId() from this PR, but:

• there are a few things that would need to be ported to another PR: the clarified docs on the method itself, and the modifications to drivers (OCI8, PDOSqlsrv, ...)
• I quickly checked your other PR, and don't get the point of the LastInsertId object?

From what I could see by quickly browsing the (quite numerous) changes, it looks like you're attempting to retain the last insert ID, even if there were other statements executed in between. It's a feature that could create more problems than it would solve, IMHO:

As I said in a comment above, whenever I used lastInsertId(), I know for a fact that the previous statement generated such an ID. If I executed another statement (a SELECT?) in the meantime, I consider this a defect in my code and I want to know about it.

What if today I have an INSERT followed by a SELECT? Your approach will work fine, and allow me to get the last insert ID after both statements.
Then some time later, I introduce another INSERT somewhere after this SELECT. At first glance it's safe: the previous statement is a SELECT; and suddenly the previous code fails silently (getting the wrong ID).

Again, the more I think about it, the more I think this is an exceptional situation: either you get an ID from the very last statement executed, or you get an exception if it didn't yield an ID.

What do you think?

[morozov]

I quickly checked your other PR, and don't get the point of the LastInsertId object?

It's not the point. I mentioned that PR to show that this Connection::lastInsertId() API is a can of worms. We already have a history of burning days of work without producing any changes. And I don't want this API to derail the originally planned work: return void in beginTransaction(), commit(), rollBack()

Again, the more I think about it, the more I think this is an exceptional situation: either you get an ID from the very last statement executed, or you get an exception if it didn't yield an ID.

I agree. I thought about it some more and also came to the conclusion that you shouldn't want just to look if there's a last inserted ID, you expect it to be there in a certain situation and be able to use it.

What do you think?

You'll need to add more tests next to \Doctrine\Tests\DBAL\Functional\WriteTest::testLastInsertId*() for the following cases:

1. Last inserted ID is empty (e.g. just connected).
2. The named sequence doesn't exist.

[BenMorel]

I had to resort to this nasty code, to be able to call the factory method on the abstract class, as I can't use a factory method on an anonymous class.
Exceptions refactoring is really needed here!

[BenMorel]

Note that Scrutinizer wrongly reports that static::class === self::class is always true, I'll file a bug with them.

[BenMorel]

What if you drop Connection::lastInsertId() from this pull request so that we can focus on what's already clear?

lastInsertId() is now becoming clear, are you OK to merge it together with this PR, and once this is done, I'll open another PR to split it into 2 methods?

[morozov]

lastInsertId() is now becoming clear, are you OK to merge it together with this PR […]

Yes. I only suggested it to not hold the rest of the changes.

[BenMorel]

So apart from the type-cast on SQLAnywhere, that we don't know how to test (it doesn't hurt to leave it, as for sure it cannot create a bug, whereas removing it could create one), I think I'm done on this PR, @morozov!

Please let me know if you see anything else; I'd like to move forward with splitting lastInsertId() into 2 methods, and other stuff too. Thanks!

[morozov]

Is this method supposed to be called on the abstract class? I'd rather remove this block since it's a temporary solution than complicate it that much.

Another thought I had in mind is, do we really need driver-specific exceptions which are not originated in the underlying drivers? The primary reason for having OCI8Exception, MysqliException, etc. separated out is that different drivers provide different information about errors and different APIs for obtaining this information.

In the case of exceptions like this which originate in the DBAL, it's not applicable. The information is always the same.

Unlike most other use cases, where this separation is useful when a client is interested in catching only a specific type of exceptions, DBAL clients are not interested in driver-specific exceptions because the driver is abstracted out.

Another point is, an exception originated in DBAL, cannot properly implement Doctrine\DBAL\Driver\DriverException because it doesn't have error code and SQLSTATE by definition.

Can we move these new methods to Doctrine\DBAL\DBALException (even if it's an API change) and find a better place later?

[BenMorel]

Is this method supposed to be called on the abstract class? I'd rather remove this block since it's a temporary solution than complicate it that much.

Yes, this is to allow calling AbstractDriverException::noInsertId(). There is no other solution to factorize the noInsertId() method as you requested it, without either making AbstractDriverException not abstract, or creating more driver-specific exception classes.

Another thought I had in mind is, do we really need driver-specific exceptions which are not originated in the underlying drivers? (...) Unlike most other use cases, where this separation is useful when a client is interested in catching only a specific type of exceptions, DBAL clients are not interested in driver-specific exceptions because the driver is abstracted out.

Exactly. That's why I suggested earlier that DriverException might not be an interface, but a concrete class.

I get your point that subclasses allow for driver-specific factory methods such as fromSqlSrvErrors(), fromPDOException() and so on, but they provide nothing that could not be implemented in the Driver\Connection classes.

For example, in SqlSrvConnection: throw SQLSrvException::fromSqlSrvErrors() could be replaced with throw $this->createExceptionFromSqlSrvErrors(), that would create a generic DriverException. This removes the need from an SQLSrvException class altogether.

Another point is, an exception originated in DBAL, cannot properly implement Doctrine\DBAL\Driver\DriverException because it doesn't have error code and SQLSTATE by definition.

Error code and SQLSTATE are currently optional in DriverException, but anyway I'm not sure why you would throw a DriverException from another place of the DBAL?

As I see it, the driver is the lowest layer of DBAL, and the rest of the DBAL is another layer built on top of it: it uses drivers and handles DriverExceptions, but never throws them.

Can we move these new methods to Doctrine\DBAL\DBALException (even if it's an API change) and find a better place later?

DBALException does not implement DriverException, so this would not respect the contract!

What I would suggest, if you see no other issue here, is that we merge this PR as is: it provides well encapsulated changes, and has just a handful of well-documented exotic ways to throw exceptions; as soon as this is merged, I can file another PR to propose to change interface DriverException to class DriverException, and get rid of the driver-specific exceptions. This should be a simple change that will clean up the codebase, and will fix all the temporary hacks introduced here. And we can start a fresh discussion there if issues arise.

What do you think?

[morozov]

For example, in SqlSrvConnection: throw SQLSrvException::fromSqlSrvErrors() could be replaced with throw $this->createExceptionFromSqlSrvErrors(), that would create a generic DriverException. This removes the need from an SQLSrvException class altogether.

This way, you could throw these exceptions only from connections. They should be also available to statements. That's why they are separate classes.

DBALException does not implement DriverException, so this would not respect the contract!

What contract? Connection::lastInsertId() doesn't declare any thrown exception. Throwing an exception (which can happen even in master) is already vilation of the contract.

[BenMorel]

This way, you could throw these exceptions only from connections. They should be also available to statements. That's why they are separate classes.

No problem if implemented as a static method on the Connection, that can be used by the Statement as well.

What contract? Connection::lastInsertId() doesn't declare any thrown exception.

It does now, that's the whole point of what we've been discussing so far, and the contract that the newly introduced tests enforce.

[BenMorel]

I've pushed a commit in another branch, that I will move to a PR once/if this is merged:
BenMorel@f9e0c5a

It does exactly what I suggested:

• AbstractDriverException is now a concrete DriverException, replacing the interface
• All sub-exceptions are now gone
• All exotic new class ... extends AbstractDriverException are now replaced with new DriverException
• Factory methods from sub-exceptions are moved to a public static method in the Connection, that can be used from within the Connection itself and from the Statement
• All relevant Connection methods are now explicitly documented as throwing DriverException

This last point is very important: so far, most of the methods had no documented exceptions, while query() and exec() were documented as throwing DBALException, a contract that was never respected:

• PDOConnection::query() and exec() throw Driver\PDOException, which is a DriverException, not a DBALException
• DB2Exception::query() and exec() throw DB2Exception, which is a DriverException
• SQLSrvException::query() and exec() throw SQLSrvException (via SQLSrvStatement::execute()), which is a DriverException
• ... and so on.

This future PR will make these things right.

[BenMorel]

@morozov If you don't see any other issue here, I'll be happy get this merged and move forward to #3367 and #3368, that are ready for review!

[morozov]

The PR cannot be merged in its current state. If it requires extra rework of exceptions, it should be done first, not the other way around.

[BenMorel]

I've updated #3367 to be a standalone PR, not based on this one.
It's ready for review, @morozov.

Some things are intrinsically related, though: for now methods such as commit() or rollBack() are marked both as @return false and @throws DriverException.

This may feel weird, but reflects the current reality: some drivers such as DB2 already throw exceptions here, instead of respecting the current contract.

As described in the PR description, that PR is aimed to "make things right" regarding DriverExceptions; once/if merged, the present PR will finish the job by removing the @return false-style methods and make other drivers respect the new contract.

If you think that changes in both PRs are too intrisically related, I can also merge the 2 PRs, but this will be a bigger change, harder to review.

[morozov]

Closing as superseded by #3480 and #3568.