Deprecate Abstraction\Result

[morozov]

Q	A
Type	deprecation
BC Break	no

Problem #​1: The wrapper Result methods are effectively documented as @throws Driver\Exception

In #4019 where the Result interfaces were first introduced, the idea was to have the Abstraction\Result extend Driver\Result since from the methods and parameters standpoint, the former is currently a superset of the latter.

The above didn't take into account exception handling. Specifically, the fact that the Driver\Exception is not a sub-type of Exception. It creates the following confusion:

interface Driver\Result
{
    /**
     * @throws Driver\Exception
     */
    function fetch();
}

interface Abstraction\Result extends Driver\Result
{
}

class Result implements Abstraction\Result
{
    /**
     * @throws Exception
     */
    function fetch() {
        try {
            $this->driverResult->fetch();
        } catch (Driver\Exception $e) {
             throw new Exception();
        }
    }
}

Despite the fact that Result::fetch() catches Driver\Exception from the driver result and only throws Exception, it inherits the @throws Driver\Exception annotation from Driver\Result. It means that the callers of Result::fetch() should be able to handle Exception despite the fact that it will never be thrown:

Problem #​2: Adding fetch*() and iterate*() methods to the wrapper Result requires a breaking change

See #4293. Since the wrapper Result is exposed by the Statement as Abstraction\Result interface, making the new methods available to the consumers requires adding them to the interface which is a breaking change.

Solution

Unlike the driver level where the interfaces have multiple implementations and are meant for extensibility of the driver layer, the wrapper layer is essentially a set of convenience methods packed into a class which is not meant to have multiple implementations by definition.

The solution is to remove the interface and use the classes directly, similarly to the wrapper Connection and Statement classes.

I believe it's acceptable to introduce this deprecation in a patch release of 2.11.x:

1. This release series is mostly about deprecations.
2. The API being deprecated was introduced in 2.11.0, so it's unlikely that there is existing code that relies on this behavior.
3. There's no other usage of the interface other than for forward compatibility with 3.0.
4. If not done in 2.11.x, we'll have to release 2.12.0 or postpone the BC break until 4.0.

See the drafted removal PR for 3.0.x: #4294.

[SenseException]

So the plan for 4.0 is that Abstraction\Result will be removed from Driver\Result? Or is this not planned yet?

[morozov]

If this gets merged, then the only change I want to do next is to remove the extends DriverResult from the Abstraction\Result. Both interfaces will still have the same methods defined in them. I want to do it in 3.0, not in 4.0.

[SenseException]

Suggested change

	The usage of the `Abstraction\Result` as a sub-type of the `Driver\Result` interface is deprecated.
	The usage of the `Abstraction\Result` as a sub-type of the `Driver\Result` interface is deprecated. The interface `Driver\Result` will be removed from `Abstraction\Result` in 3.0.

[morozov]

The "will be removed" removed wording is a bit confusing: will we also remove the methods or not? Would it make sense to reword it as "the interface X will not extend Y in 3.0"?

[SenseException]

Sounds better than my text, yes. 👍

[morozov]

@SenseException your request for change is addressed. Although the scope of the PR has changed. Please review.