Extract Result from the Statement interface

[morozov]

Q	A
Type	improvement
BC Break	yes

Rationale

The Statement interface currently fulfills two roles: “statement execution” and “fetching the resulting data and metadata”. There are certain problems with this approach:

1. Given a statement object, it's not clear whether it was executed (and therefore, has a result) or not. This leads to having guarding statements across the codebase:

   dbal/src/Driver/Mysqli/MysqliStatement.php

   Lines 307 to 311 in 96f8cab

   	// do not try fetching from the statement if it's not expected to contain the result
   	// in order to prevent exceptional situation
   	if (! $this->result) {
   	return false;
   	}

2. A consumer of the statement result could execute the original statement which would be legal from the API standpoint but most likely unwanted or mistaken:

   dbal/tests/Functional/Schema/SqliteSchemaManagerTest.php

   Lines 275 to 276 in b0ca943

   	$query = $this->connection->query('SELECT id FROM test_pk_auto_increment WHERE text = "2"');
   	$query->execute();

3. Classes like Portability\Statement that have to deal with the statement in its different phases have to depend on union types:

   dbal/src/Portability/Statement.php

   Lines 15 to 16 in 7204a7d

   	/** @var DriverStatement|ResultStatement */
   	private $stmt;

   …and perform awkward type assertions:

   dbal/src/Portability/Statement.php

   Line 37 in 7204a7d

   assert($this->stmt instanceof DriverStatement);

Summary of the changes

1. The Statement interface no longer extends ResultStatement
2. The ResultStatement interface has been renamed to Result.
3. Instead of returning bool (or void in master), the query-related methods now explicitly return a Result.
4. The statement data and metadata now should be fetched from the result, instead of the statement.
5. Public methods in the Result interface and its implementors have been reordered as “fetching data”, “fetching metadata”, “close” from most to least commonly used and more or less corresponding to the statement result lifecycle.
6. The Abstraction package is introduced for abstraction-level interfaces. Currently, only the Result interface is available. The Connection and the Statement ones are to follow.
7. The temporary forward compatibility layer introduced in Deprecated the concept of the fetch mode #4019 is effectively replaced by a permanent forward-compatible API.

Additional benefits

1. The non-happy cases that were previously covered by the testFetchFromNonExecutedStatement(), testCloseCursorAfterCursorEnd() and testFetchFromNonExecutedStatementWithClosedCursor() tests and had to be handled at runtime are no longer possible by design.
2. Reduced existing code complexety. Each of the execution and data fetching aspects can be implemented and tested independently.
3. Easier code extensibility. In order to extend a certain aspect (e.g. fetching data), it's no longer needed to write boilerplate code for the unrelated aspect (binding parameters).
4. The “fluent” usage of the statements is now possible: $conn->prepare($sql)->execute($params)->fetch();

Implementation details

1. ArrayStatement has been renamed to ArrayResult.
2. ResultCacheStatement has been renamed to CachingResult.
3. The Statement::testPrepareWithIterator() had to and could be reworked but was deleted instead since it doesn't cover any special case (binding parameters and iteration over the statement are orthogonal and don't need to be tested together).
4. The behavior of fetching from a closed result is no longer portable.

Discoveries (future scope)

1. The portability layer is currently implemented on two levels of abstraction at the same time: most of its components implement driver-level interfaces but the Connection class extends the DBAL connection. It leads to the double wrapping of the abstraction-level result into a driver-level portable result and then again into an abstraction-level result to fulfill the API contract. This can be avoided if the layer is moved to the driver level completely.
2. The caching layer, instead of being part of the DBAL Connection class can be made a driver-level wrapper to simplify the code.
3. All driver-level statements implement query() as prepare()->execute(). While it's valid, it's suboptimal for those drivers that implement query() natively (mysqli, sqlsrv). It takes only one TCP round-trip to execute a query and three to prepare a statement, execute it and discard the handle.

Questions

1. Given that the new fetchColumn() is now called on a result instead of the statement, it's no longer a silent change in an existing method’s semantics. Can we use this name instead of having to use fetchFirstColumn()?

   We could if we provided an upgrade path in 2.11.x but it wasn't possible.

2. Should we rename closeCursor() to something like free()? A “free” procedure is called by the mysqli and ibm_db2 implementations internally and seems more applicable to a result.

   Done.

TODO:

• Backport to 2.11.x (Replace forward-compatible ResultStatement interfaces with Result #4049).
• Update UPGRADE.md.

[greg0ire]

1. Yes IMO
2. No opinions or objections

[codecov-commenter]

Codecov Report

Merging #4045 into 3.0.x will increase coverage by 0.07%.
The diff coverage is 51.13%.

@@             Coverage Diff              @@
##              3.0.x    #4045      +/-   ##
============================================
+ Coverage     78.01%   78.09%   +0.07%     
+ Complexity     4616     4603      -13     
============================================
  Files           181      189       +8     
  Lines         11579    11552      -27     
============================================
- Hits           9033     9021      -12     
+ Misses         2546     2531      -15     

Impacted Files	Coverage Δ	Complexity Δ
src/Driver/OCI8/OCI8Connection.php	0.00% <0.00%> (ø)	22.00 <1.00> (ø)
src/Driver/OCI8/OCI8Statement.php	47.67% <0.00%> (+16.84%)	25.00 <5.00> (-17.00)	⬆️
src/Driver/OCI8/Result.php	0.00% <0.00%> (ø)	14.00 <14.00> (?)
src/Driver/SQLAnywhere/Result.php	0.00% <0.00%> (ø)	10.00 <10.00> (?)
src/Driver/SQLAnywhere/SQLAnywhereConnection.php	0.00% <0.00%> (ø)	24.00 <1.00> (ø)
src/Driver/SQLAnywhere/SQLAnywhereStatement.php	0.00% <0.00%> (ø)	19.00 <6.00> (-12.00)
src/Query/QueryBuilder.php	95.08% <ø> (ø)	139.00 <0.00> (ø)
src/Cache/ArrayResult.php	75.67% <33.33%> (ø)	16.00 <3.00> (?)
src/Result.php	43.39% <43.39%> (ø)	25.00 <25.00> (?)
src/Driver/IBMDB2/Result.php	50.00% <50.00%> (ø)	15.00 <15.00> (?)
... and 27 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 3d8c575...48625f1. Read the comment docs.