Moved the escaping of the xpath locator to the NamedSelector #641
Conversation
|
It looks like some kind of code variation from 2.0 branch, but don't understand how it correlates with other commits from #384 task, that were made in 2013 year. |
|
@aik099 it is indeed a variation of the work done in stof@ebf26f8 for #542 but keeping a BC layer instead of forcing the upgrade. The 2.0 PR will then be updated to include only the removal of the BC layer. |
|
I like the direction we're going with the drivers to allow safe transition to Mink 2.0. |
|
this one is not about drivers (#633 is enough for that) but about userland code (I think that after this change, the upgrade should be painless except for some very custom use cases) |
|
@aik099 is it OK for you ? |
| * | ||
| * @return SelectorsHandler | ||
| */ | ||
| protected function getSelectorsHandler() |
There was a problem hiding this comment.
This will surely break something within QA-Tools, once merged.
There was a problem hiding this comment.
I guess I need to do Escaper::escapeLiteral when I need to escape xpath instead of doing
$selectors_handler = $this->getSelectorsHandler();
$selectors_handler->xpathLiteral($text)Was this Escaper class part of Mink 1.6 release? Looks like it was, because commit 5bff8b5, which added this class is shown below 1.6.0 tag.
There was a problem hiding this comment.
Also what could bullet-proof way of detecting if used Mink does automatic locator escaping by NamedSelector or I need to manually escape it upfront? I'm asking because of https://github.com/qa-tools/qa-tools/blob/develop/library/QATools/QATools/HtmlElements/Element/Form.php#L86 construct.
There was a problem hiding this comment.
Once Mink 1.7 is released, the easiest way would probably be to bump the dependency to Mink 1.7 (which autoescapes)
There was a problem hiding this comment.
and yes, I should not remove this method for BC. It will be removed only in 2.0
There was a problem hiding this comment.
@aik099 you could use method_exists(NamedSelector::class, 'escapeLocator') to detect that this PR is included in the codebase if you want to keep the Mink 1.6 compatibility.
And when building your own Xpath, you should indeed use the Escaper class (which is available in 1.6.0)
The NamedSelector has no valid reason to expect receiving an escaped value for the XPath locator. Calling code has no reason to know that the locator will be inserted in an XPath query. The NamedSelector still support passing an escaped value for BC reasons but this is deprecated. The method SelectorsHandler::xpathLiteral and Element::getSelectorsHandler have been deprecated as well.
stof
force-pushed
the
escape_named_selector
branch
from
ebf26f8 to
e238faa
Compare
|
@aik099 anything else to do before the merge ? |
|
Nope. You can merge. |
Moved the escaping of the xpath locator to the NamedSelector
The NamedSelector has no valid reason to expect receiving an escaped value for the XPath locator. Calling code has no reason to know that the locator will be inserted in an XPath query.
The NamedSelector still support passing an escaped value for BC reasons but this is deprecated.
The method SelectorsHandler::xpathLiteral has been deprecated as well.
Fixes #384
This allows fixing the issue in Mink 1.7 rather than only 2.0, which allows people to migrate their code progressively (which is better)