Replace the concept of statement fetch modes with the explicit API

[morozov]

Q	A
Type	improvement
BC Break	hell yes

The approach of using FetchMode when fetching data from a statement inherited from PDO has certain downsides:

1. The type of the return value of a given method (e.g. fetch()) is determined at runtime (it can be a mixed, array<int,mixed>, array<string,mixed> and even array<int|string,mixed>) and is unclear for static analysis.
2. Besides the mode passed explicitly to a fetch() method, it can be set on the statement via setFetchMode(), so if a component gets passed an existing ResultStatement(), it cannot be certain of the fetched data type unless it specifies it explicitly.
3. Even if the component executes the statement itself, it cannot be certain about the fetched data type because it can be set globally at the connection level.
4. Having the fetch mode expressed via a set of constants requires their additional validation in the method and throwing an exception if the mode is invalid or unsupported. This puts us in an awkward position: a lot of methods can throw an invalid mode exception but it's not handled anywhere.
5. Unlike calling fetch methods where a method can be passed explicitly, it's impossible to pass the fetch mode to the statement when iterating over it. It's only possible to set it on the statement and therefore modify the shared state that others may rely on.

The proposed solution is to define an explicit fetch-oriented API that driver-level statements have to implement. This API will define a dedicated method for each combination of the following:

1. Row presentation mode (associative array, numeric array, the value of the first column).
2. Row traversal mode (one row, all rows, iterator).

Statement	Numeric	Associative	One/Column	All	Iterator
mysqli	✓
pdo	✓	✓	✓	✓	✓
sqlsrv	✓	✓
oci	✓	✓	✓	✓
ibm_db2	✓	✓
Cache		✓		✓	✓

The above matrix illustrates the driver capabilities that I want to be exposed via the new API and is meant to justify its verbosity:

1. Numeric. Support for fetching rows as numeric arrays.
2. Associative. Support for fetching rows as associative arrays.
3. One/Column. Support for fetching one value or a column.
4. All. Support for all rows in one call.
5. Iterator. Support for iteration over rows.

As one can see, the numeric and associative modes are supported by most of the drivers and they are really the essential primitives for fetching data. The “one”, “all” and “iterator” capabilities are supported only by a few but I believe it's important to be able to use their in-driver implementation instead of having to emulate them in DBAL even if the driver supports it (see a similar problem in #3744).

Breaking changes:

1. The FetchMode class and the setFetchMode() methods are removed as well as the entire concept of the fetch mode.
2. The fetch() method is replaced with fetchNumeric(), fetchAssociative() and fetchOne() (like fetchOne() in ZF1).
3. The fetchAll() method is replaced with fetchAllNumeric(), fetchAllAssociative() and… should it be fetchColumn() like fetchCol() in ZF1?
4. Connection::fetchAssoc() and ::fetchArray() are renamed to ::fetchAssociative() and ::fetchNumeric() respectively for consistency.
5. In order to make the Driver and the DBAL APIs distinct, a DBAL-level ResultStatement interface is introduced. It extends the one at the Driver level (therefore, it can gain new DBAL-level features w/o having to implement them at the Driver level).
6. The statements are no longer traversable. Only DBAL-level statements may be iterated via iterateNumeric(), iterateAssociative() or iterateColumn().
7. The StatementIterator class is removed.
8. Fetching in the “mixed”/“both” mode is no longer supported. It's not a limitation of the new design. I just haven't seen a single case in my entire life where it would be needed. Well… the caching layer could use it but it doesn't.

TODO:

• Add the new methods to the wrapper connection class for consistency.
• Provide the upgrade path and forward compatibility layer.
• Adjust code comments and documentation.
• Document the change in UPGRADE.md.

The next steps will be:

1. Separate driver-level API from the wrapper-level (Separate DBAL and driver interfaces #3591). This will allow independent evolution of the wrapper-level API w/o the need to make any changes to the drivers (e.g. add new fetch modes).
2. Replace ResultStatement with the Result interface. This will allow avoiding awkward situations like:
   1. A statement exists but cannot be fetched from because it wasn't executed.
   2. A component is given a statement to fetch the data from but it can bind new parameters to it and execute it.

[greg0ire]

Why not provide the upgrade path upfront and target 2.11.x? Since we do merges up now, it makes more sense to use that flow to keep branches in sync IMO, with a first PR that includes a BC layer and deprecations, a merge up PR, and a PR that drops deprecated code.

[greg0ire]

This new API is great, many bugs will be caught earlier thanks to it I think 👍

[morozov]

Why not provide the upgrade path upfront and target 2.11.x?

Makes sense. I'd like to finish all the work here first, and before merging it, extract the relevant parts into a patch against 2.11.x.

[morozov]

Not sure how to proceed with the forward compatibility layer for 2.x. We can easily introduce the new methods in the wrapper connection class but what about the driver layer? Unlike a class where we can add methods, adding methods to the driver connection interface is a BC break on its own. If we just add them to the driver connection classes, then in order to use them, the code that depends on the interface will have to abuse its knowledge of the specific classes that implement the old interface and the new methods. Any ideas, @greg0ire?

[greg0ire]

You could create a new interface, and have the code that depends on the interface check if the new one is implemented, and complain if it isn't. Ideally, type declarations against the old interface should be dropped so that object can implement only the new interface. This should be ok because we require 7.2, so dropping a type declaration doesn't result in incompatible type declarations.

class ConsumingCode
{
    /** @param OldInterface|NewInterface $resultStatement */
    public function foo($resultStatement): void
    {
        if (!$resultStatement instanceof NewInterface) {
            // trigger deprecation
            // throw if $resultStatement does not implement Old Interface
            // use same code as before
            return;
        }


        // code with shiny new calls
        return;
    }
}

This plan leaves us with one issue: naming.

dbal/src/Driver/ResultStatement.php

Line 10 in cc7bc1e

* Interface for the reading part of a prepare statement only.

I can't say I understand this comment but maybe it will be of some inspiration to you for finding a new name?

[morozov]

This plan leaves us with one issue: naming.

The problem is with the design, not just with naming. See my notes in the description regarding ResultStatement. I already started prototyping the new API but the rework will affect a lot of code being changed here, so I decided to not proceed before this one is done.

[morozov]

Thanks for the code example, @greg0ire. It helps to understand the idea significantly. Let's see if it's feasible to apply it at the scale of all components that depend on the Statement interface.

[greg0ire]

The other PR I just merged seems to introduce conflicts

[morozov]

This will need to be backported to 2.11.x (found by PHPStan and enforced by the refactored ResultCacheTest).

[morozov]

@greg0ire it's ready now.

[greg0ire]

Shouldn't the new docs be backported to the lower branch?

[morozov]

Yes.

[greg0ire]

Should we maybe mark this as internal in lower branches? Since we remove it here, we don't want people to type hint against it. If it's ok to type hint against it, maybe we should keep it, deprecate it, and empty it.

[morozov]

Let's not mark it internal so that people don't feel wrong about using it, and let's empty and deprecate it instead of removal in 3.0.x.

Most likely, this interface will go away or get renamed/repurposed before 2.11.x is released but I'm fine with keeping it always releasable.

[greg0ire]

Not sure if it's the right day to use that emoji, but… 🚀

[lcobucci]

@morozov nice job, mate 👍

The API is much nicer and explicit but it has a huge impact on the ORM, so I think it's crucial to align these changes with @guilhermeblanco.

[morozov]

@lcobucci thanks for the heads up. I hope the impact is huge in terms of the number of LOC that need to be changed, not in terms of complexity. As far as I can tell, most of the code can be fixed almost by find and replace.

Since the same API is already implemented in 2.11.x, I believe we can proceed with the merge, and if there are any specific issues with the new API that need to be addressed, we'll have to address them starting 2.11.x anyways.

In the meanwhile, merging this will unblock me in reworking the driver/wrapper and statement/result interfaces.