Merge Registry into Driver?

[kelunik]

I think it would be better to merge Registry into Driver and make Driver an abstract class and those two methods final.

[sagebind]

I don't know how many positives or negatives would be in this approach, but it would no longer be possible to write a driver that extends a different, non-driver class.

[sagebind]

If the goal is to get rid of the trait, another idea would be to implement the registry as a separate binding service:

class Registry
{
    public static function fetchState(Driver $forDriver, $key) { /* ... */ }
    public static function storeState(Driver $forDriver, $key, $value) { /* ... */ }
}

I think the existing trait approach makes sense though and is the situation traits were meant for.

[bwoebi]

@coderstephen That, IMO, is the absolute worst solution (a class Registry).

Also, I don't think we should have an abstract class as base. It's not our task to define the implementation. We can recommend implementations and that's all. That's why we have the trait. Users shall be able to easily override it (e.g. put a var_dump() in their driver implementation to debug and call the trait method then).

[kelunik]

@bwoebi If possible, we would have implemented it in the Loop class like we did initially. This is something that really shouldn't change between drivers. It should always be the same. You can still edit Driver.php to add that var_dump temporarily.

I don't know how many positives or negatives would be in this approach, but it would no longer be possible to write a driver that extends a different, non-driver class.

If you follow the single responsibility principle, which you should, your driver shouldn't ever extend another class that's not a driver itself.

Any driver implementation really is a driver, not only behaves like one.

[bwoebi]

Perhaps you're right. The only thing I really disagree with is making the methods final.

[kelunik]

Not sure whether they should be final. But I think it makes sense, because it's nothing a loop should change. It's code belonging to the accessor really, we just can't put it there without memory leaks if we want to support reusing loops with the same state again.

[kelunik]

@assertchris @AndrewCarterUK @WyriHaximus @trowski What do you think?

[WyriHaximus]

Not sure yet, learning torwards implementing this in the Loop as we initially did.

[kelunik]

@WyriHaximus That doesn't work with reusing loops and having the exact same state then again.

[kelunik]

PR is now available: #87

[WyriHaximus]

@kelunik looking at #87 that makes sense :+1:

[assertchris]

Yeah, on reflection, I think this is neater.

[kelunik]

Can we go ahead and merge the PR?

[kelunik]

Merged.