Improve type safety of driver element API

[rvodden]

Potential Problem

At the moment all the Element classes implement ElementInterface and extend from BaseElement. This presents a number of issues, none of which in isolation are a big deal, but in conjunction may mean we want to look at refactoring.

1. Not all Element classes implement every operation. This is dealt with at the moment by having BaseElement throw an UnsupportedDriverActionException by default. This doesn't distinguish between features which happen to not be implemented by the driver (like database transactions in WPCLI), and which cannot be implemented by design (for example, a create method in CacheElement is never going to make sense).

2. Linked to the above, there are operations which are driver specific and need to be held in the Element classes which don't fall neatly into the create, get, update, delete, model. For example getPermalink in ContentElement requires a different implementation for WPCLI and WPHP, and only applies to content so definitely lives in ContentElement.

3. The Element classes are coupled to the driver at runtime using an array of ElementInterface. This is great because it keeps the coupling loose. The flip side is that PHP doesn't understand all the details of the Element classes. For example, I can't see the phpdoc of an Element class in the IDE; simlarly argument exceptions won't be caught until runtime etc. The screen shot below shows phpdoc working for getDriver(). If you move the mouse pointer right so that it's over content no such tooltip appears. I figured that didn't need another screen shot ;-)

1. Some of the create, get, update, delete methods feel a little forced. For example in DatabaseElement, import and export feel more descriptive to me than get and update. Similarly in CacheElement, clear is more meaningful than delete. In both cases there are alias methods, however a solution which doesn't require aliasing will make code easier to diagnose and reduce the complexity of the classes.

2. Ensuring that features are implemented consistently is manual at the moment with out much help from php. #126 is an example of this where the getUserByLogin method of UserElement in WPPHP and WPCLI were returning different types. This logic has since been moved to UserAwareContextTrait as it was driver agnostic. I feel the principle still stands.

3. Documentation is duplicated across the drivers. For example, the phpdoc on UserElement->create() is identical in WPPHP and WPCLI. This is because its documenting the API which is being implemented rather than the implementation.

4. At the bottom of the list is that Scrutinzer and other static code inspection tools complain when you call a method implemented in a concrete class which is referenced by a variable declared as an interface which does not declare that method. The error looks like:

   Accessing cache on the interface PaulGibbs\WordpressBehat...\Driver\DriverInterface suggest that you code against a concrete implementation. How about adding an instanceof check?

I'm certainly not going to lose sleep about SCI, but it usually has errors for a reason. I think I've captured most of those reasons in the points above, but there may well be others.

Requirements

As I said at the top, none of these issues are a particularly big deal, so any proposed solution should deal with a decent number of them to be worth implementing. This means we're looking for something which:

1. Doesn't expose unrequired methods. CacheElement->create() should fail at compile time.
2. Permits nuance to each element. The architectural principal of only CRUD operations in the Element classes remains, however methods like getPermalink should be catered for.
3. Accommodate more descriptive method names across drivers without the need for aliasing. DatabaseElement->export() should be standalone and not an alias for get().
4. Assist with driver implementation consistency. That is if a driver fails to implement a required method it should be called out at compile time.
5. Retain runtime coupling whilst also ensuring type safety (as measured by phpdoc tooltips working ;-) )
6. Provides a home for API level documentation.
7. Fewer scrutinizer issues.

Solution

I propose changing the driver class hierachy a bit. Right now everything descends from BaseElement where in practice there is very little common code between all the Element classes. I suggest we acknowledge that each Element is different and author Interfaces for each of the Element classes. These will be kept outside of the drivers. We can then have WPCLI and WPPHP implementations of each of these interfaces.

Using a set of Element specific interfaces like this will mean that unrequired methods can just be left out, nuanced methods can be included, more descriptive names can be used where preferred, drivers inconsistencies will be detectable, and there's a nice neat home for API documentation.

That deals with everything except runtime composition. I want to have a play around with a few options for doing this, but I think getting Symfony to inject Element classes into the drivers, in the same way that the PageObjects are injected into the Contexts, is probably the way to go. The other option is to effect changes to WordpressDriverManager but I think we'll just end up implementing our own DI framework - which is daft when we've got one already.

[rvodden]

When we've sorted this out I'll have a go at #52 ;-)

[rvodden]

Actually - I've had a poke around and I understand the driver creation process and the element injection a bit better now. We are, I see, already using symfony to inject the Element classes so it would just be a case of tweaking that injection so it was type safe. A more traditional injection pattern like constructor injection may work well here.

[paulgibbs]

@rvodden Do you think we can make this change in a 1.x release (without breaking backwards compatibility) as this is basically internal/plumbing changes?

[rvodden]

Yeah I think we can. Let me start it off, if I do it for one element it will give me an idea of timescales.

[paulgibbs]

Thanks

[paulgibbs]

I'd like to get moving on this. @rvodden if you've no time or interest, mind if I take a first pass at it? I'll need to run it by you several times I suspect, but I can make a start.

[rvodden]

Let me push up my branch with my WIP - I've got a skeleton which you can base your stuff on

[paulgibbs]

@rvodden if you're going to try to pick this up again, may I suggest starting over with a new branch, and just trying to get small sections complete, rather than attempt to swap everything at once and then run out of steam?