-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
DBAL 3.0: suggestions to improve the Driver\Connection interface #3335
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
Changes from 1 commit
d4fec7c
5a7b383
7b124ec
dec14fb
3154749
76bb463
3195492
2170801
acebbfa
77d9b2f
5c63dc0
46da0b0
3d862a7
756a733
8327c79
2a20c16
1f81dd6
91f6707
5939e49
997671b
f1f1677
32c473c
b130d98
2fb9115
ac0c12d
a143efc
77a76d8
ce32c09
fe9200a
b1ad528
9636513
ad9c3bc
09bbaa4
245377f
6eea142
d71824e
b888c04
0346d39
137a962
47f4ba7
733c63a
1eb4cf3
69b8cef
d7f06f3
e6d452e
03ccace
304bc85
fbc7fb6
d3f7686
4e92005
254f38f
ff92ad6
97b7eb4
b7e8230
2f7bd80
2b33eef
0ef7d47
dd97e68
3b88f34
f158cae
8b03839
91d25ad
a2612af
301afef
49b8d81
7f91f56
84a948f
b9a4c2c
6179736
b7a56ff
a045df6
80ae604
f59b7d4
0e70bcd
1c1f770
4ef3e44
7c254d5
93cbe45
0ee1959
542a17f
2dfd20a
cc51085
3c0a308
4a17f4d
dbf37c9
6561dab
808c42d
e52cd52
f7f0848
e388c86
4613cab
65850cd
da462ed
890eae6
8f8c235
3be0f5c
5bf41d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,42 @@ public function __construct($message, $sqlState = null, $errorCode = null) | |
| $this->sqlState = $sqlState; | ||
| } | ||
|
|
||
| /** | ||
| * @return AbstractDriverException | ||
| */ | ||
| public static function noInsertId() : self | ||
| { | ||
| $message = 'No identity value was generated by the last statement.'; | ||
|
|
||
| if (static::class === self::class) { | ||
| // WIP regarding exceptions, see: | ||
| // https://github.com/doctrine/dbal/pull/3335#discussion_r234381175 | ||
| return new class($message) extends AbstractDriverException { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that Scrutinizer wrongly reports that |
||
| }; | ||
| } | ||
|
|
||
| return new static($message); | ||
| } | ||
|
|
||
| /** | ||
| * @param string $name The sequence name. | ||
| * | ||
| * @return AbstractDriverException | ||
| */ | ||
| public static function noSuchSequence(string $name) : self | ||
| { | ||
| $message = 'No sequence with name "' . $name . '" found.'; | ||
|
|
||
| if (static::class === self::class) { | ||
| // WIP regarding exceptions, see: | ||
| // https://github.com/doctrine/dbal/pull/3335#discussion_r234381175 | ||
| return new class($message) extends AbstractDriverException { | ||
| }; | ||
| } | ||
|
|
||
| return new static($message); | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,10 +101,7 @@ public function lastInsertId(?string $name = null) : string | |
|
|
||
| // pdo_mysql and others return '0', pdo_sqlsrv returns '' | ||
| if ($lastInsertId === '0' || $lastInsertId === '') { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed since different drivers return different empty values? If so, please add a comment to the code.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is indeed. Added a comment:
|
||
| // WIP regarding exceptions, see: | ||
| // https://github.com/doctrine/dbal/pull/3335#discussion_r234381175 | ||
| throw new class ('The last statement did not return an insert ID.') extends AbstractDriverException { | ||
| }; | ||
| throw AbstractDriverException::noInsertId(); | ||
| } | ||
|
|
||
| return $lastInsertId; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -140,7 +140,7 @@ public function lastInsertId(?string $name = null) : string | |
| } | ||
|
|
||
| if ($result === 0) { | ||
| throw new SQLAnywhereException('The last insert did not affect an AUTOINCREMENT column.'); | ||
| throw SQLAnywhereException::noInsertId(); | ||
| } | ||
|
|
||
| return (string) $result; | ||
|
|
@@ -149,7 +149,7 @@ public function lastInsertId(?string $name = null) : string | |
| $result = $this->query('SELECT ' . $name . '.CURRVAL')->fetchColumn(); | ||
|
|
||
| if ($result === false) { | ||
| throw new SQLAnywhereException('No sequence with name "' . $name . '" found.'); | ||
| throw SQLAnywhereException::noSuchSequence($name); | ||
| } | ||
|
|
||
| return (string) $result; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure whether casting is needed here. No idea how to verify that. Most likely, the SQLAnywhere driver in DBAL is broken for a long time and we'll have to get rid of it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there no CI tests for SQLAnywhere? Should we leave the typecast, in doubt? |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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\DriverExceptionbecause 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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is to allow calling
AbstractDriverException::noInsertId(). There is no other solution to factorize thenoInsertId()method as you requested it, without either makingAbstractDriverExceptionnot abstract, or creating more driver-specific exception classes.Exactly. That's why I suggested earlier that
DriverExceptionmight 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 theDriver\Connectionclasses.For example, in
SqlSrvConnection:throw SQLSrvException::fromSqlSrvErrors()could be replaced withthrow $this->createExceptionFromSqlSrvErrors(), that would create a genericDriverException. This removes the need from anSQLSrvExceptionclass altogether.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.
DBALExceptiondoes not implementDriverException, 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 DriverExceptiontoclass 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way, you could throw these exceptions only from connections. They should be also available to statements. That's why they are separate classes.
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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem if implemented as a static method on the Connection, that can be used by the Statement as well.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
AbstractDriverExceptionis now a concreteDriverException, replacing the interfacenew class ... extends AbstractDriverExceptionare now replaced withnew DriverExceptionpublic staticmethod in the Connection, that can be used from within the Connection itself and from the StatementDriverExceptionThis last point is very important: so far, most of the methods had no documented exceptions, while
query()andexec()were documented as throwingDBALException, a contract that was never respected:PDOConnection::query()andexec()throwDriver\PDOException, which is aDriverException, not aDBALExceptionDB2Exception::query()andexec()throwDB2Exception, which is aDriverExceptionSQLSrvException::query()andexec()throwSQLSrvException(viaSQLSrvStatement::execute()), which is aDriverExceptionThis future PR will make these things right.