zendframework / maintainers

Contributors and maintainers information for Zend Framework.
BSD 3-Clause "New" or "Revised" License
50 stars 25 forks source link

How should we provide return type declarations for fluent interfaces? #12

Closed weierophinney closed 7 years ago

weierophinney commented 7 years ago

With the move to PHP 7.1, we can now document scalar method arguments, as well as return type declarations. This latter is of particular importance for end users, as they can then be assured that what they expect is what they will receive.

However, this poses a problem with fluent interfaces, particularly when inheritance of any type is involved, due to how PHP implements return type declarations: they are implemented as invariants, which leads to issues when implementations or extensions need to include the return type declaration.

As examples, given either of the following:

interface A
{
    public function select(string $table) : self;
}

class B
{
    private $address;

    public function to(string $address) : self
    {
        $this->address = $address;
        return $this;
    }
}

Implementing A or extending B runs into issues, as we cannot use self as a return type declaration when implementing either select() or overriding to(). For a single layer of extension, this is not a problem, as we can use parent instead:

class C extends B implements A
{
    private $table;
    private $where;

    public function select(string $table) : parent
    {
        $this->table = $table;
        return $this;
    }

    public function to(string $address) : parent
    {
        $this->where = $address;
        parent::to($address);
        return $this;
    }
}

IDE problems

While the above parses correctly, it poses a problem in IDEs, as they now assume that the return values are of type A and B, respectively and not C. As such, they will not allow expansion of methods in C on the return value.

However, if we have another level of inheritance — e.g., class D extending C — neither self nor parent now work as return type declarations, and you instead need to provide the return type of the root declaration:

class D extends C
{
    private $myOwnTable;
    private $myCriteria;

    public function select(string $table) : A
    {
        $this->myOwhTable = $table;
        parent::select($table);
        return $this;
    }

    public function to(string $address) : B
    {
        $this->myCritieria = $address;
        parent::to($address);
        return $this;
    }
}

As noted above, this then means that IDEs now think the return value is of another type entirely, meaning they cannot hint on the methods from the actual class.


Some possible solutions:

  1. Use annotations instead: @return self or @return static or @return $this. These, however, mean that the contract is not enforced at the engine level. I largely feel this is a non-starter, as it goes against our reasons for moving to 7.1 in the first place.
  2. Don't worry about it, and just use the root declaration. This indicates an "is-a" relationship, and enforces the idea that a class operates as a general type, promoting the idea of re-use of generic types, and not concrete types. This would be the simplest way and pose the least amount of backwards compatibility breakage, but could pose issues for consumers in components such as zend-form that have multiple levels of inheritance when using IDEs.
  3. Deprecate usage of fluent interfaces except when used for implementing immutability (e.g., PSR-7's with*() methods), where return type declarations are indicating the interface type returned, and not the instance type. Most fluent interfaces within the framework would then instead return void. This would pose the most BC breakage, but forward-proof the APIs the most.

Please let us know in the comments what solution you prefer, and why.

MidnightDesign commented 7 years ago

I'm in favor of #3.

Fluent interfaces are just a minor convenience, but they're semantically dumb. $foo->setBar() should be void, not Foo.

alextech commented 7 years ago

These are really tough choices. I believe IDE compatibility should be top priority. IMO this is part of reason why OpenSource can be frustrating to use as opposed to some enterprise level toolkits where the tools are developed in conjunction with the language and frameworks complementing each other (usually, or at least attempt to). JavaScript is most guilty of not following this rule drastically decreasing productivity.

Fluent interfaces are just a minor convenience, but they're semantically dumb.

Most of the time. Zend DB, has a pipeline-like feel to it. $select()->from()->where()->groupBy(). flows quite nicely actually, giving it a feel similar to Adam Wathan's "refactoring to collections" material. It won't be a major mental shift to do $select->from(); $select->where(); $select->groupBy() but will be epic BC (although I am curious how accurate is packagist's download counter is in relation to its usage)

geerteltink commented 7 years ago

Option 3 for too many reasons: https://ocramius.github.io/blog/fluent-interfaces-are-evil/

weierophinney commented 7 years ago

@alextech I think zend-db's SQL and DDL subcomponents are special cases, as there's actually not a ton of inheritance, nor are folks expected to extend them and/or implement interfaces from them . As such, we can likely continue to use fluent interfaces there.

weierophinney commented 7 years ago

Moving the discussion to discourse: https://discourse.zendframework.com/t/how-should-we-provide-return-type-declarations-for-fluent-interfaces/30