Improve client builder and options

[Jean85]

This PR addresses a few last things:

• ClientBuilder ctor now accepts Options, not a naked array
• Adds Options:setDsn
• Removes the magic method ClientBuilder::__call that forwarded calls to the options
• Adds return types to ClientBuilderInterface

Last thing: should we make Options `final?

[ste93cry]

I explicitly avoided to use self in the interfaces because PHP doesn't have covariant returns and this forces anyone implementing this interface to define that the methods returns the interface and not the type of the class itself which can cause issues with fluent interfaces like this one. Please revert everything back

[ste93cry]

So, for the question related to making the Options class final 👍 from me. For the other points I see no real benefit in most of the changes. The one that concerns me most is that to instantiate the builder you now have to first instantiate the Options class and you lose a lot of simplicity for no real benefit since that class has to be created from an array of params anyway. We can provide a createFromOptions method if you really think that configuring the client options outside the builder is something really needed but I want to absolutely avoid over-complicating the builder instantiation

[Jean85]

I can see your points. If we want to reduce the impact, we may reduce this PR to:

• making Options final
• add a ClientBuilder::createFromOptions()
• Add a way to pass the dsn to Options at construction apart from the naked array, something like Options::createWithDsn().

@HazAT @ste93cry WDYT?

[ste93cry]

Both the first and second point of the list are fine. The last point makes me wonder if we really need it. What's the improvement of having a parameter dedicated to just the DSN instead of passing it directly together with all the other options?

[Jean85]

The only bonus for the third point is that I'm forced to pass it with an array, always, contrary to all other options with have a setter instead.

[ste93cry]

contrary to all other options with have a setter instead.

This is because you can't change the DSN after creating a client since the transport won't be updated to reflect the new value. The only downside I see in making this change is that you're treating the DSN option differently from the others

[Jean85]

I've reverted the setDns commit, and applied @HazAT suggestion from #728 (comment). Apart from that, IMHO we should leave the return types in the ClientBuilderInterface, since it's better that nothing, and @return $this is documented on each method to underline that is a fluent interface.

[ste93cry]

IMHO we should leave the return types in the ClientBuilderInterface, since it's better that nothing, and @return $this is documented on each method to underline that is a fluent interface.

The problem with using self as return type of methods of an interface is that it doesn't means "this", so classes extending the interface will have to typehint against the interface for their return type. In turn this means that (even if unlikely that it will happen) if somebody creates his own client builder and adds some methods to the class he won't be able to use them in the fluent interface without losing autocomplete and without any possibility to inform users that those methods really exists but not in the interface. Said that and given that the return type typehint is acting wrong there are no benefits in applying them, PHPDoc is enough in this case until we have proper support from PHP for such things

[Jean85]

In that regard, The PHPDoc says @return $this, so the IDE should pick that up to infer the actual subtype of the builder in use.

Yes, we don't have contravariance in PHP so it's not solvable at the language level, but at the IDE level it seems fine to me.

[ste93cry]

Yes, we don't have contravariance in PHP so it's not solvable at the language level, but at the IDE level it seems fine to me.

Yea probably IDEs will be able to work if they give priority to the annotation instead of the typehint, but as you said yourself this problem is in the PHP language itself and we should not try to workaround it using a docblock since it's misleading and doesn't respect the real code

[Jean85]

I've addressed or answered every comment from the review.

we should not try to workaround it using a docblock since it's misleading and doesn't respect the real code

I disagree on this. PHPDoc here helps to narrow the type that can be inferred from the code alone, in the same way we document arrays as @var string[]. PHPStan and other static analysis tools like IDEs do give priority to PHPDoc over code for this exact reason; so in this way we have the stricter language barrier, and the right documentation with the help of PHPDoc.

[ste93cry]

I hope that with this example the reasons why I am against this change are clearer. Let's take as an example the following code:

interface HelloWorldInterface
{
    /**
     * @return $this
     */
    public function foo(): self;
}

class HelloWorld implements HelloWorldInterface
{
    /**
     * {@inheritdoc}
     */
    public function foo(): HelloWorldInterface
    {
        echo 'foo method called' . PHP_EOL;

        return $this;
    }
    
    public function bar(): self
    {
        echo 'bar method called' . PHP_EOL;

        return $this;
    }
}
    
(new HelloWorld())->foo()->bar()->foo();

This is perfectly fine and runs on PHP, however PHPStan reports an error. Infact the only reason the build is green is because you're ignoring the error in the phpstan.neon config file, so you are actually bypassing a language limitation using a docblock that doesn't match the real return typehint and its meaning by tricking the configuration of the static checker. Also according to the typehint (if for a moment we discard any docblock) I would expect the foo method to return an instance of the HelloWorld class and not a generic object implementing the HelloWorldInterface, but due to PHP this is not possible if you typehint self in the interface

[Jean85]

This seems a bug to me: I've moved @return $this directly on the implementation and PHPStorm stopped complaining. As per PHPStan, is a tracked bug: phpstan/phpstan#1648

So I would like to avoid changing the correctness of the code basing our choiches on tools that will be fixed in the future. Unfortunately PSR-5 is still under discussion, so it's not reliable for now.