Replace forward-compatible ResultStatement interfaces with Result

[morozov]

Q	A
Type	deprecation
BC Break	no

This patch backports the interfaces being introduced in #4045. Unfortunately, since Statement::execute() already returns bool there's no way to implement a forward-compatible behavior.

Before:

$statement->closeCursor();

After:

$statement->free();

[greg0ire]

Unfortunately, since Statement::execute() already returns bool there's no way to implement a forward-compatible behavior.

Sounds indeed complicated, maybe @derrabus would have an idea?

Only ugly solutions come to mind:

• relying on state (some global constant, or some local property)
• supporting an extra argument to specify you want the new behavior
• renaming the method

[derrabus]

Thank you for the ping. Splitting statement and result into different classes is a reasonable change. Thanks again for working on this.

So the question is, how to deal with the changed return type of Statement::execute()? It would be great if we could craft a forward-compatible solution here.

Only ugly solutions come to mind:

• relying on state (some global constant, or some local property)

• supporting an extra argument to specify you want the new behavior

• renaming the method

All three would probably work. Right now, I would be in favor of version 2.

Another idea could be that Connection::prepare() is replaced by a new method that returns a statement with the new behavior. (Not a suggestion, just brainstorming)

I could offer to work on forward-compatible behavior for execute() once this PR is merged.

[derrabus]

Would it make sense to backport the new result classes from #4045 as well, so that the old statement classes would act as proxies to the new result classes for all functionality that has been moved?

[morozov]

So the question is, how to deal with the changed return type of Statement::execute()? It would be great if we could craft a forward-compatible solution here.

Could you provide a real example of the boolean return value of execute() being used by the caller? As far as I can tell, it's a rudiment that exists because DBAL extends the PDO APIs, but the boolean return value is only meant to be used with PDO::ERRMODE_WARNING which is not the case for DBAL. The failure to execute a statement is in fact reported by throwing an exception:

dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php

Line 30 in dbc623d

$this->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

[morozov]

Would it make sense to backport the new result classes from #4045 as well, so that the old statement classes would act as proxies to the new result classes for all functionality that has been moved?

This was the original intent but given that we cannot return the result from execute(), what's the point of changing statement implementations? Right now, the statement itself implements the Result interface, so the consuming code can be changed to expect this type instead of the old one.

[derrabus]

Could you provide a real example of the boolean return value of execute() being used by the caller?

In a pure DBAL environment, it is pointless to check for the return value because, as you pointed out, true is the only value to expect here.

However, I could imagine cases where the connection is passed to some library functionality that operates on generic PDO connections and thus needs to handle cases where PDO::ATTR_ERRMODE is not set to PDO::ERRMODE_EXCEPTION.

That code would probably look like this:

if (!$someStatement->execute($someParams)) {
    // Do some error handling.
}

That code would still work even if execute() would suddenly return an object. A piece of code that would break would be this one:

if (true !== $someStatement->execute($someParams)) {
    // Do some error handling.
}

I wouldn't write that kind of code, but I've seen projects where this was considered best practice. I expect the impact of the return type change to be pretty low, if that was the answer you're looking for. Yet, it's a breaking change.

[derrabus]

Would it make sense to backport the new result classes from #4045 as well, so that the old statement classes would act as proxies to the new result classes for all functionality that has been moved?

This was the original intent but given that we cannot return the result from execute(), what's the point of changing statement implementations?

You're right, you wouldn't gain much. My strongest argument would be that you'd keep the diff between 2.11 and 3.0 smaller which could be helpful for the maintenance later on.

[morozov]

My strongest argument would be that you'd keep the diff between 2.11 and 3.0 smaller which could be helpful for the maintenance later on.

To me, it looks like too much effort for such little gain.

Let's take the driver-level statement and result as an example. Since the new Result interface implements only part of the result-related Statement interface (e.g. the result doesn't implement fetching in the mixed mode), the proxy will have to either emulate it on top of the Result API or implement it itself. The latter will require sharing the underlying statement resource between the result and the proxy.

In the case of the wrapper-level, it's even more complicated: the wrapper statement wraps a driver-level statement that may or may not implement the new interface. In this case, proxying to the Result is only possible if the wrapped statement implements Result and the method being called is part of the Result interface. Otherwise, see above.

This is the primary reason I decided to not provide FC layer for the ArrayResult and CachingResult but instead mark them internal.

[morozov]

However, I could imagine cases where the connection is passed to some library functionality that operates on generic PDO connections and thus needs to handle cases where PDO::ATTR_ERRMODE is not set to PDO::ERRMODE_EXCEPTION.

This doesn't look like a valid case. The fact that a library does if (true !== $stmt->execute()) means that it expects a non-true value to be returned in the case of an error but it will get an exception from DBAL.

I expect the impact of the return type change to be pretty low, if that was the answer you're looking for. Yet, it's a breaking change.

I totally agree. Not sure how we got into this argument. We're not going to change the return value in 2.x for the sake of providing an FC layer.

[derrabus]

This doesn't look like a valid case. The fact that a library does if (true !== $stmt->execute()) means that it expects a non-true value to be returned in the case of an error but it will get an exception from DBAL.

Yes, but you won't necessarily get an exception from PDO. This is why code like this might be in place in libraries that are not aware of DBAL and operate on generic PDO connections.

It's still not good code and it might be an edge-case. But it's the best example I could come up with.

[codecov-commenter]

Codecov Report

Merging #4049 into 2.11.x will decrease coverage by 2.13%.
The diff coverage is 40.40%.

@@             Coverage Diff              @@
##             2.11.x    #4049      +/-   ##
============================================
- Coverage     73.16%   71.02%   -2.14%     
- Complexity     5031     5261     +230     
============================================
  Files           215      217       +2     
  Lines         12825    13355     +530     
============================================
+ Hits           9383     9485     +102     
- Misses         3442     3870     +428     

Impacted Files	Coverage Δ	Complexity Δ
lib/Doctrine/DBAL/Cache/ArrayStatement.php	48.52% <0.00%> (-39.28%)	30.00 <12.00> (+12.00)	⬇️
lib/Doctrine/DBAL/Cache/QueryCacheProfile.php	71.42% <ø> (ø)	11.00 <0.00> (ø)
...octrine/DBAL/Connections/MasterSlaveConnection.php	66.97% <ø> (ø)	42.00 <0.00> (ø)
.../Doctrine/DBAL/Driver/AbstractPostgreSQLDriver.php	96.36% <ø> (-1.82%)	24.00 <0.00> (ø)
...octrine/DBAL/Driver/DrizzlePDOMySql/Connection.php	0.00% <ø> (ø)	3.00 <0.00> (ø)
...ib/Doctrine/DBAL/Driver/DrizzlePDOMySql/Driver.php	66.66% <ø> (-6.07%)	5.00 <0.00> (ø)
lib/Doctrine/DBAL/Driver/IBMDB2/DB2Connection.php	45.74% <ø> (ø)	21.00 <0.00> (ø)
lib/Doctrine/DBAL/Driver/OCI8/Driver.php	14.28% <ø> (ø)	4.00 <0.00> (ø)
lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php	0.00% <ø> (ø)	26.00 <0.00> (ø)
lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php	22.22% <0.00%> (-3.73%)	68.00 <15.00> (+11.00)	⬇️
... and 149 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecff851...e7c7cb1. Read the comment docs.