minkphp
Mink 2.0 architecture changing ideas
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.
- [x] 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
- [x] 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)~~
- [x] Prefer exact matches in the NamedSelector: #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.
- [x] Refactor the way XPath selection works to fix #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 #333 for the initial report
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.
@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.
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.
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.
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:
- Browser
- Browser->Driver - way to drive a browser
- Browser->Page - the DocumentElement representing current page in browser
- 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.
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.
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.
@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.
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.
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?
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.
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.
@aik099 I suggested the dependency change for the whole Element namespace, not only for NodeElement
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
And regarding the exceptions, they need to be rfecatored as well, as the current way to build the response info is flawed anyway. See the issue I linked in my first comment
@aik099 I suggested the dependency change for the whole Element namespace, not only for NodeElement
Then I understood you correctly. It's just NodeElement and DocumentElement are the classes that can be instantiated. Of course the changes are need to be made in their parent class.
Maybe we can create 2.0 branch (based on develop) and will try to implement agreed changes in there?
yeah, I also though about it. I will have a few hours in a train on Friday, so I may start it
Here what I've done so far: https://dl.dropboxusercontent.com/u/17535850/mink-2.0-start.diff . You can start on that.
Maybe I'll just create that branch right away and send commit changes there. Ok?
yeah, create a branch in the repo (eventually with a non-semver name for now to avoid issues with composer for people using unbound constraint). This way, we can collaborate on it more easily
We can also change Session class constructor the way to follow DI rule: pass required dependencies in constructor; pass optional dependencies via setter methods. This applies to Session class.
@everzet @aik099 What do you think about my suggestion related to the driver interface (see the first comment) ?
I'm aware of all, but DriverInterface removal changes. Initially CoreDriver was not a problem, but since it's the only class that was implementing all DriverInterface methods (at least now it does), then having DriverInterface around looks like code duplication and double work to anyone, who tends to add new methods to the drivers.
If we compare abstract CoreDriver class and DriverInterface interface, then using either will not break the SOLID rule depend on abstractions and not on implementation details.
Also if we remove DriverInterface, then we can make CoreDriver code shorter by adding a single __call method to throw an exception when call to undefined method (from methods each driver must implement) will happen. This however will make new driver developer life harder as there won't be a method to copy-paste to new driver implementation.
Nice trick with checkboxes.
@aik099 the issue is that adding a new method in an interface is a BC break. The CoreDriver only mitigates the BC break by allowing drivers to benefit from an upstream implementation, but it is still a BC break for a direct implementation of the interface. This means that we either cannot follow semver or we have to bump the major version all the time. On the other hand, adding a concrete method in an abstract class is not a BC break for child classes (adding an abstract method has the same issue than for interfaces)
Regarding __call in CoreDriver if we remove the DriverInterface, it is a really bad idea. If we remove the interface, CoreDriver would become the class defining the driver API. We need to keep it explicit
In hopes, that no code outside Behat repos is using DriverInterface we can safely remove it.