Fix 146

[rvodden]

*** NOT YET READY TO MERGE ***

So here is the long awaited (and not yet finished) PR for #146. This is very much a work in progress, and this PR is being raised now to give us a place to discuss some options. This establishes some architectural principles:

1. Everything is properly type safe. Duck typing isn't something we do on this project.
2. Avoid replicating symfony functionality, where its necessary keep it contained.
3. Drivers should be self contained.
4. Concerns should be clearly separated:
   1. Contexts are responsible for joining features to Elements
   2. Elements are responsible for manupulating the driver to expose element functionality to the Contexts
   3. Drivers are responsible for manipulating wordpress

(There is some ambiguity with the word Driver, it is both the class which manipulates WordPress and also the collection of Manger, Driver and Elements which provide functionality to the Contexts; I don't see this being an issue. I'll use Driver to mean the class and Driver to mean the collection in this PR).

As such the following major changes have been introduced:

1. Each element now has an interface which defines its type. The constructors of each of the contexts have been augmented to accept the appropriate elements, and these should be autowired by Symfony.
2. Drivers are now self contained. WordpressDriverManager (will be when we've finished) depricated, with its functionality either moved into Symfony or devolved to the pertinent drivers.
3. There was a separation of concerns issue with the Driver classes, they were responsible for driving WordPress and also for registering the driver with wordhat (i.e. Symfony). A Manager class (i.e. WpcliManager etc.) has been spun up to deal with the registration.
4. The service container configuration has been dramatically simplified; we now lean wherever possible on Symfony's autowiring:
   1. Linked to separation of concerns, the elements have their respective driver autowired into them using constructor injection.
   2. This would be an ideal approach for the Contexts too, but unfortunately Behat doesn't yet support Context autowiring. Therefore the WordpressAwareInitializer has been augmented to build all contexts which require elements. Next to each Trait is an Interface which defines the contract implemented by the trait. The implementation of the interface is detected by the WordpressAwareInitializer and the appropriate Element is injected by calling the setter. Once Enable Symfony DI autowiring Behat/Symfony2Extension#128 is resolved, the WordpressAwareInitializer can be refactored out and constructor injection used in the contexts.

Stuff still to do:

1. Sort out configuration. At the moment the configuration is all housed in WordpressBehatExtension, this is fine for the configuration of the plugin, but the driver specific configuration should be refactored into the DriverManagers
2. The construction process needs to be documented really clearly. I've started this (see WordpressAwareInitializer); its quite a complex process which we're not going to touch very often, so writing it down in English next to the code will help us resolve issues if they appear.
3. WPPHP needs refactoring - I've moved the code, but done nothing else with it as yet.
4. Some general tidying up. The WordpressDriverManager has some persistence code which should be somewhere else so that the WordpresDriverManager can be retired. RawWordpressContext and WordpressContext can be reviewed to check they've got the right stuff in (getDriver won't exist anymore).
5. Review the output of the higher levels of phpstan and see what else needs to be done.

Stuff we can consider doing, either as part of this refactor or separately.

1. The Drivers could be Behat extensions in their own right. This means that they can either be included in our code (like WPCLI and WPPHP) or as separate composer includes - this might be an interesting way to start the REST driver.
2. The code which is currently in traits could be (I think usefully) refactored into abstract classes within the Driver definition (i.e. PaulGibbs\WordpressBehatExtension\Driver\Element\BaseElements\BaseCacheElement would have the code currently held in CacheElementAwareTrait). I think this is slightly cleaner that using Traits.

[paulgibbs]

@rvodden I'm working my way through this.

Quick question: the changes to build/*, the phpstan config file, etc - not sure if those are only showing up in the diff because of that merge conflict. Can we get that resolved? (If those are new changes in this branch, we should split those build changes into its own branch and look at them separate).

[rvodden]

Yeah - I'l sort this out; I must confess I did a 'push and forget' ;-) Didn't structure it properly.

[paulgibbs]

@rvodden

Leaving aside minor code formatting issues, missing/documentation tweaks, and debugging and commented-out code, and recognising that you've been working mostly on the WP-CLI driver rather than the unfinished WP-PHP changes:

WordpressDriverManager

• Why using Behat\Mink\Driver\DriverInterface? Doesn't seem relevant; not used anywhere else?

services.yml

• Is it possible to have the WordPressDriverManager arguments written as, e.g. plugin_element, instead of pluginElement, to match code standards?
  • This goes for other places too, e.g. property userElement in UserAwareContextTrait, etc.

WordpressBehatExtension

• Where/how is setupWpcliDriver() called nowadays? Is this moved into WpcliManager.php, and the old method is just not removed yet
• ASIDE: happy to keep debugging info such as listServicesAndTags() around if that's going to be something we're genuinely going to have to probably debug at some point. But let's find a way of requiring some sort of --debug CLI parameter to trigger this extra verbosity?

General question about Driver Elements, e.g. Driver/Element/Wpcli/TermElement.php.

• The existing multi-driver supported works by switching the returned driver in the old $this->drivers->getDriver() method. How does this work when we are passing the new driver into a driver element's constructor?
• Ditto e.g. CacheElementInterface->cacheElement, because those elements are specific to a driver.
• Do we need to pass something by-reference as a (psuedo) pointer to get around this, or is it already handled somewhere?
• We could consider dropping multi-driver support if this is going to make things hugely complex.

Plugin Element

• Why does Driver/Element/BasePluginElement.php exist? Seems unused? (vs, one of these for every other element?)

Compiler/DriverPass.php

• "bootstrap the driver at the point it is created" -- do we want this? e.g. the WP-CLI driver shells out to WP-CLI, and the WP-PHP driver loads wordpress' PHP files. A site configured to use WP-PHP should not try to talk to WP-CLI.
• Or did you refactor the bootstrap methods to avoid this?

[paulgibbs]

Stuff we can consider doing, either as part of this refactor or separately.
The Drivers could be Behat extensions in their own right. This means that they can either be included in our code (like WPCLI and WPPHP) or as separate composer includes - this might be an interesting way to start the REST driver.

Let's not look at this for now (with this PR). The PR is already massive.

The code which is currently in traits could be (I think usefully) refactored into abstract classes within the Driver definition (i.e. PaulGibbs\WordpressBehatExtension\Driver\Element\BaseElements\BaseCacheElement would have the code currently held in CacheElementAwareTrait). I think this is slightly cleaner that using Traits.

I am generally concerned at the amount of interfaces and abstract classes we already have in this branch. We have lots of interfaces just for strong typing. I do wonder if by aiming for full type safe, we risk making the code over-engineered, and if that's a good trade off. More complex code is harder to maintain.

Please would you put together a very rough image of a diagram that shows a representative example of a vertical slice of the architecture, showing the class/interface relationships between Elements and Driver (the class and the collection)? I want to see how complex it is, and more important, want to understand it. I think I have the basic idea by reading the code, but if I'm beginning to struggle, someone less familiar with the code base is going to have a worse time?

Thanks so much for the work here. :)

[rvodden]

Sorry for being so SLOW! Life got in the way to a certain extent. I'm back now :)

WordpressDriverManager

Why using Behat\Mink\Driver\DriverInterface? Doesn't seem relevant; not used anywhere else?
services.yml

Yup - shouldn't be used. The DriverMangager is deprecated (As is DriverInterface) and I hadn't deleted the bits which refer to DriverInterface so my IDE picked up what it could find (one of many excellent reason never to push PRs which don't compile)

Is it possible to have the WordPressDriverManager arguments written as, e.g. plugin_element, instead >of pluginElement, to match code standards?
This goes for other places too, e.g. property userElement in UserAwareContextTrait, etc.
WordpressBehatExtension

Yeah - of course. I haven't done a sweep for style yet, so that would have been picked up then - I'll ammend.

Where/how is setupWpcliDriver() called nowadays? Is this moved into WpcliManager.php, and the old >method is just not removed yet

Exactly that - I just haven't moved it yet.

ASIDE: happy to keep debugging info such as listServicesAndTags() around if that's going to be >something we're genuinely going to have to probably debug at some point. But let's find a way of >requiring some sort of --debug CLI parameter to trigger this extra verbosity?

I agree - i'll look at it.

General question about Driver Elements, e.g. Driver/Element/Wpcli/TermElement.php.

The existing multi-driver supported works by switching the returned driver in the old $this->drivers->>getDriver() method. How does this work when we are passing the new driver into a driver element's >constructor?
Ditto e.g. CacheElementInterface->cacheElement, because those elements are specific to a driver.
Do we need to pass something by-reference as a (psuedo) pointer to get around this, or is it already >handled somewhere?
We could consider dropping multi-driver support if this is going to make things hugely complex.

The plan (which isn't coded yet) is to have the compiler pass parameterised. If no parameter is passed it will use the default_driver otherwise it will use whatever is passed in, and the appropriate Elements will be injected in to the Contexts as a result.

Plugin Element

Why does Driver/Element/BasePluginElement.php exist? Seems unused? (vs, one of these for every >other element?)

Its a placeholder. If there is common code across all the Elements then it can go here. If nothing ends up in it before we merge then I'll remove it (I suspect that that will be the case).

Compiler/DriverPass.php

"bootstrap the driver at the point it is created" -- do we want this? e.g. the WP-CLI driver shells out to >WP-CLI, and the WP-PHP driver loads wordpress' PHP files. A site configured to use WP-PHP should >not try to talk to WP-CLI.
Or did you refactor the bootstrap methods to avoid this?

We're now using Symfony to load the drivers. Symfony 'Lazy Loads' which means it only instantiates a class if it is asked for. By default the contexts will only ask for the Driver which is referred to in default_driver (In fact the context will ask for an element which in turn will ask for a driver). If driver switching is being used, then the 2nd driver will be initialised at the start of the context which makes use of it.

I am generally concerned at the amount of interfaces and abstract classes we already have in this branch. We have lots of interfaces just for strong typing. I do wonder if by aiming for full type safe, we risk making the code over-engineered, and if that's a good trade off. More complex code is harder to maintain.

This should make it easier. If you need to, for example, add a new method to a driver because you're implementing a new feature, then you add it to the appropriate ElementInterface and your IDE (or PHP at runtime) will tell you where in the drivers you need to add it. A little "How to most easily add new features" paragraph is probably a good idea though, just to highlight this trick to people. Should probably be next to the diagram I'll draw for the next answer :) As for the trade off between Traits and BaseElements, the traits would be replaced by the BaseElements so complexity should be about the same - the only distinction is that BaseElement is type safe and traits are not. Remember that type safety isn't just a "good thing" it also means that we can make use of Symfony's autowiring so we'll be able to remove some of our glue classes - in particular WordpressAwareInitializer.

Please would you put together a very rough image of a diagram that shows a representative example of a vertical slice of the architecture, showing the class/interface relationships between Elements and Driver (the class and the collection)? I want to see how complex it is, and more important, want to understand it. I think I have the basic idea by reading the code, but if I'm beginning to struggle, someone less familiar with the code base is going to have a worse time?

Absolutely - grand idea :)

[paulgibbs]

@rvodden There is valuable work here and the effort that has been spent is of course appreciated, but the branch needs rebasing and I'm not sure if there's been more progress since Christmas that's not yet pushed.

To try to keep Github PRs tidy and keep the project looking maintained, I'm going to close the PR for the time being.