Use a callable for setFactory()
bwoebi opened this issue · 14 comments
There seems to be consensus to use a factory for the loop (see #30).
The open issue is whether we shall use a Factory interface or require a callable.
Currently, with 5.5 neither are actually hintable against a return type - but in future we may also have callable hinting.
I don't see any particular advantage of requiring an interface here.
Thus, as I think callables are more approchable and easier to implement [and especially the functions don't have to be reusable in this case], a callable should be preferable in this particular case.
There is still an advantage as subscription to an interface is explicit, subscription to a callable is implicit. With an interface you need to say I implement this, unlike with a callable.
I'm pro interfaces, but don't care significantly enough that I would strongly oppose the use of callable.
I think an interface would be better here. The setFactory method can be explicit about what is expected and eventually we can enforce the return type of create through the interface.
PHP 7's anonymous classes make one-off factory classes much easier to define and create.
@trowski with PHP 5 you also just can create a class _InternalLoopFactory implements Loop\Factory, two lines above the actual Loop::setFactory() call, but do we win anything by that? I see no particular benefit in this and thus a simple callable should be fine enough?
Ad.: The docblock of setFactory can be explicit about the expected callable return type too...
Using an interface will potentially simplify configuration of DI containers in comparison to a callback.
Edit: To clarify:
class Foo
{
// $loopFactory param must be configured explicitly in DI config for this class, and every other class using it
// $baz can be configured using a general preference, e.g. LoopFactory => UvLoopFactory
function __construct(callable $loopFactory, LoopFactory $baz)
{
$this->loopFactory = $loopFactory;
}
function bar()
{
...
Loop::setFactory($this->loopFactory);
...
}
}@joshdifabio You will never use those factories with a DI container.
@bwoebi I'm for the interface, that allows multiple default classes, e.g. PersistentFactory that always returns the same instance.
@joshdifabio You will never use those factories with a DI container.
Ah, yes, I see that now.
@kelunik If you have a PersistentFactory, you pretty much break the premise of Loop::execute() to always run a new instance (unless explicitly passed).
Loop::execute doesn't guarantee that, it just guarantees to take either $driver or asks the DriverFactory to give it one.
So, I think most people are in favor of an interface… Closing then.
As the PR now allows to replace the default loop used, I'm also in favor of the interface as it allows people to just write Loop::setFactory(new ConcreteLoopFactory); at the start of their applications in order to choose their preferred loop in their mostly-sync application.
@bwoebi They don't even have to do that. Most often the used event loop implementation will do that already in its bootstrap.
@kelunik sure, but what if due to composer dependencies two or more event loops are pulled in? Then you have to choose one explicitly, _if_ you care about what loop you run.
(Typically not an issue as libraries code to asnyc-interop/event-loop only and require only a concrete loop for tests. But this may happen though.)
It may happen, but it should actually never be the case.
Typically not, as said. I'm just in favor as it may happen and thus needs to be possible in some way, which the interface solves nicely.