Mink 2.0 architecture changing ideas

[aik099]

There was no major architecture redesign in Mink since it's initial version. Over time several things popup that might be better to implement the other way, than now.

For example NodeElement always getting driver from the session.

Feel free to post your ideas about whatever can be improved in Mink architecture.

[stof]

• Removing the Session object from the driver. It makes no sense to have both knowing of each other. the Session is a higher-level abstraction than the driver
• Make the NamedSelector escape its locator before replacing it in the XPath instead of relying on the caller to do the escaping. The caller should not need to know about the way the input needs to be escaped. See https://github.com/Behat/Mink/issues/384
• Stop calling methods directly on the driver in places where the session is injected (for instance in the Element classes). The driver should be considered as an implementation details of the session (we should keep a getter for it, to allow users to access the driver when they need to do some stuff not possible with the Mink abstraction, but Mink itself should not use it)
• Prefer exact matches in the NamedSelector: Follow "link" is following wrong links #265
• Controversial point needing to be discussed: drop the DriverInterface in favor of AbstractDriver (currently CoreDriver). This is what Doctrine DBAL does for its platform. We are technically breaking BC each time we add a new feature, as we are modifying the interface. The CoreDriver was added to mitigate the BC break for drivers extending it, but it means we are not following semver. Using an abstract class means it is not a BC break, as we would not force drivers to be modified with the method implementation to be working. The alternative would be to have several interfaces for the different parts of the driver (and adding a new feature impacting the driver would add a new interface). But this would mean that the Session has to check all the time whether the driver implements the extra interfaces, that we have to release Mink each time we add such a new interface to allow bumping the driver requirement on Mink (as they would require the new interface) and that we will have headaches finding names for these interfaces.
• Refactor the way XPath selection works to fix XPath selection is broken for element #340. This probably means that the Element itself should not be responsible for building the final XPath anymore (prepending its own path) but that a dedicated service should implement the more complex logic.
• Refactor the way Mink exception works. Currently, their string representation embeds the content of the page at the time the exception is casted to string. But this can happen long later than the exception was thrown, so giving the wrong content in case Mink was used inbetween (thus making it totally useless). See Wrong response info in the exception when delaying the rendering #333 for the initial report

[aik099]

To ensure, that Mink core code is using only Session to access driver we need for Session to implement DriverInterface. This way when we add new method to interface tests would fail if we don't add it to Session class as well. Sadly creating __call method to do this won't work, because it won't pass interface checks.

This way Session would act as a Driver decorator and can be passed in anywhere, where class implementing DriverInterface interface can be passed.

Proposed change along isn't that big to be put in 2.0 version.

[stof]

@aik099 I don't like this. the session is not a driver. I don't see why it should implement the same interface while it is a higher level abstraction.

Making the session a driver decorator with the same interface would mean that it become useless. The logic for each method of the driver interface would be in driver implementations, so it would be a no-op proxy, adding no benefit.
The responsibility of a driver is to implement the logic needed by the Session and depending of the driver used in the backend. The Session responsibility is to expose the Mink API to other classes. If we really don't have anything to put in the session layer, it does not really make sense to have this layer IMO.

[aik099]

If we really don't have anything to put in the session layer, it does not really make sense to have this layer IMO.

This is an interesting observation. Maybe there is not much to Session after all. I've spotted only few methods in Session that does something on it's own (like getting session id or current page). All other methods are blindly calling the driver.

[aik099]

Also fact, that I need to change 3 places (DriverInterface, CoreDriver, Session) when I add new driver method suggests that there is a problem with what Session is doing.

[aik099]

Maybe we need to introduce new concept and class - Browser. Regardless of driver type (real or headless) to end user it looks like it controls a browser through it. This way we'll have:

1. Browser
2. Browser->Driver - way to drive a browser
3. Browser->Page - the DocumentElement representing current page in browser
4. Browser->Page->NodeElement - element on a page, opened in browser

As a start we can rename Session class to the Browser. Moving Session::reload() and Session::getCurrentUrl() to the DocumentElement class (that represents a page) would semantically make more sense, then in Session (or Browser - new name) class.

Maybe Driver isn't an internals of Session after all and we might pass it directly to where we need it. For example during DocumentElement/NodeElement construction. Although passing both Session and Driver during each element construction seems incorrect.

[aik099]

I've inspected the usages of Session in the driver (@stof said before, that it's incorrect for low level abstractions to contact high level abstractions) and here is what I've found (at least for MinkSelenium2Driver): it's used only for creating new NodeElement objects, which require that session.

Maybe the better approach would be move away the NodeElement-based object creation into new ElementFactory class, that would implement ElementFactoryInterface (for easy substitution during testing for example), that will have session inside it (but not as a mandatory requirement as setSession method on DriverInterface now). This way we'll move away the new NodeElement(xpath, session) code, that each driver have into a single factory located on Mink side (and not on driver side as right now) and give that factory upon driver creation.

Then session creation code will look like this:

$driver = new \Behat\Mink\Driver\Selenium2Driver(
    'firefox', 'base_url'
);
$session = new \Behat\Mink\Session($driver);
$driver->setElementFactory(new ElementFactory($session));

Still look kind of awkward to me, when I wrote that down.

Besides the idea of moving new statements from the code into factories to allow substitution of internal Mink classes isn't a new one. I've already wrote down a plan for ability to have custom DocumentElement-based pages depending on visited url here: #408.

[aik099]

Besides, @stof can you explain that idea of not communication from bottom to top between abstraction layers in the application to me? Maybe there is a book/article that you can recommend which explains that software architecture concepts.

[stof]

@aik099 Actually, I think we should change the dependencies of elements to be the driver and the selector handler (which are the only 2 things for which the element uses the session IIRC).
Elements and the session are actually at the same abstraction level: they are the public Mink API. the driver is a lower level API.

[aik099]

This is great, since now Driver isn't restricted to Session anymore. In NodeElement session is used once in exception message. All other places are pure driver usage and 1 place is slectorshandler usage:

$opt = $this->find('named', array(
    'option', $this->getSession()->getSelectorsHandler()->xpathLiteral($option)
));

which can be totally removed if we switch to auto-escaping input of NamedSelector as we discussed in #384.

What you think about moving out NodeElement instantiation from the Driver? I think Driver should care only about browser control and not talking back to Session in any kind.

[aik099]

Also the NodeElement isn't the only element affected by dependency change here:

As you can see the ElementInterface which has getSession method is a base for 4 classes. Then getSession method will be replaced by getDriver() and getSelectorsHandler() respectively?

[aik099]

And Session in Exception is used only in this method:

 /**
 * Returns response information string.
 *
 * @return string
 */
protected function getResponseInfo()
{
    $driver = basename(str_replace('\\', '/', get_class($this->session->getDriver())));

    $info = '+--[ ';
    if (!in_array($driver, array('SahiDriver', 'SeleniumDriver', 'Selenium2Driver'))) {
        $info .= 'HTTP/1.1 '.$this->session->getStatusCode().' | ';
    }
    $info .= $this->session->getCurrentUrl().' | '.$driver." ]\n|\n";

    return $info;
}

which appears to display last response status in exception message. Maybe we need to resolve this through polymorphism and let drivers to prepare their own error message if they are able to do so. We can place base error code in CoreDriver to help them. If only we can somehow get rid of ->getStatusCode() and ->getCurrentUrl() calls here, then we can pass driver to the exceptions instead of a Session.

[aik099]

Right now the SelectorsHandler is created inside a Session (only when not created by hand and given to Session). Since we plan to change dependencies of all elements (e.g. NodeElement and DocumentElement) from Session to DriverInterface+SelectorsHandler, then we need to store that SelectorsHandler somewhere and pass it each time we create new NodeElement/DocumentElement.

And we're back at square one, where all turns back to Session which we're trying to get rid of to make dependencies more clear.

[stof]

@aik099 I suggested the dependency change for the whole Element namespace, not only for NodeElement

[stof]

and no, I don't think the element should expose a getter for the driver. It should have it internally, but it does not make sense to expose the driver or the selector handler in this place