From e238faa7cc185fb770b01b2d6ecddaaf7ddda6f7 Mon Sep 17 00:00:00 2001 From: Christophe Coevoet Date: Tue, 3 Feb 2015 21:50:36 +0100 Subject: [PATCH] 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 and Element::getSelectorsHandler have been deprecated as well. --- driver-testsuite/tests/Form/SelectTest.php | 3 +-- driver-testsuite/tests/TestCase.php | 2 -- src/Element/DocumentElement.php | 4 +--- src/Element/Element.php | 4 +++- src/Element/NodeElement.php | 4 +--- src/Element/TraversableElement.php | 22 ++++-------------- src/Selector/NamedSelector.php | 27 +++++++++++++++++++++- src/Selector/SelectorsHandler.php | 10 ++++++++ tests/Element/ElementTest.php | 5 ---- tests/Selector/NamedSelectorTest.php | 21 +++++++++++++---- tests/Selector/SelectorsHandlerTest.php | 2 ++ tests/Selector/fixtures/test.html | 2 ++ 12 files changed, 67 insertions(+), 39 deletions(-) diff --git a/driver-testsuite/tests/Form/SelectTest.php b/driver-testsuite/tests/Form/SelectTest.php index 492625a95..5808b6eed 100644 --- a/driver-testsuite/tests/Form/SelectTest.php +++ b/driver-testsuite/tests/Form/SelectTest.php @@ -66,8 +66,7 @@ public function testElementSelectedStateCheck($selectName, $optionValue, $option $session->visit($this->pathTo('/multiselect_form.html')); $select = $webAssert->fieldExists($selectName); - $optionValueEscaped = $session->getSelectorsHandler()->xpathLiteral($optionValue); - $option = $webAssert->elementExists('named', array('option', $optionValueEscaped)); + $option = $webAssert->elementExists('named', array('option', $optionValue)); $this->assertFalse($option->isSelected()); $select->selectOption($optionText); diff --git a/driver-testsuite/tests/TestCase.php b/driver-testsuite/tests/TestCase.php index ea003ce5c..eaf2fbd55 100644 --- a/driver-testsuite/tests/TestCase.php +++ b/driver-testsuite/tests/TestCase.php @@ -102,8 +102,6 @@ protected function getAssertSession() */ protected function findById($id) { - $id = $this->getSession()->getSelectorsHandler()->xpathLiteral($id); - return $this->getAssertSession()->elementExists('named', array('id', $id)); } diff --git a/src/Element/DocumentElement.php b/src/Element/DocumentElement.php index edde3c353..c2daf6601 100644 --- a/src/Element/DocumentElement.php +++ b/src/Element/DocumentElement.php @@ -46,8 +46,6 @@ public function getContent() */ public function hasContent($content) { - return $this->has('named', array( - 'content', $this->getSelectorsHandler()->xpathLiteral($content), - )); + return $this->has('named', array('content', $content)); } } diff --git a/src/Element/Element.php b/src/Element/Element.php index 07e1e119e..c717083e1 100644 --- a/src/Element/Element.php +++ b/src/Element/Element.php @@ -85,6 +85,8 @@ protected function getDriver() * Returns selectors handler. * * @return SelectorsHandler + * + * @deprecated Accessing the selectors handler in the element is deprecated as of 1.7 and will be impossible in 2.0. */ protected function getSelectorsHandler() { @@ -156,7 +158,7 @@ public function findAll($selector, $locator) return $items; } - $xpath = $this->getSelectorsHandler()->selectorToXpath($selector, $locator); + $xpath = $this->selectorsHandler->selectorToXpath($selector, $locator); $xpath = $this->xpathManipulator->prepend($xpath, $this->getXpath()); return $this->getDriver()->find($xpath); diff --git a/src/Element/NodeElement.php b/src/Element/NodeElement.php index 8e4354626..d79fb8d60 100644 --- a/src/Element/NodeElement.php +++ b/src/Element/NodeElement.php @@ -228,9 +228,7 @@ public function selectOption($option, $multiple = false) return; } - $opt = $this->find('named', array( - 'option', $this->getSelectorsHandler()->xpathLiteral($option), - )); + $opt = $this->find('named', array('option', $option)); if (null === $opt) { throw $this->elementNotFound('select option', 'value|text', $option); diff --git a/src/Element/TraversableElement.php b/src/Element/TraversableElement.php index 0621af3e5..e6bb8d7ab 100644 --- a/src/Element/TraversableElement.php +++ b/src/Element/TraversableElement.php @@ -28,8 +28,6 @@ abstract class TraversableElement extends Element */ public function findById($id) { - $id = $this->getSelectorsHandler()->xpathLiteral($id); - return $this->find('named', array('id', $id)); } @@ -54,9 +52,7 @@ public function hasLink($locator) */ public function findLink($locator) { - return $this->find('named', array( - 'link', $this->getSelectorsHandler()->xpathLiteral($locator), - )); + return $this->find('named', array('link', $locator)); } /** @@ -98,9 +94,7 @@ public function hasButton($locator) */ public function findButton($locator) { - return $this->find('named', array( - 'button', $this->getSelectorsHandler()->xpathLiteral($locator), - )); + return $this->find('named', array('button', $locator)); } /** @@ -142,9 +136,7 @@ public function hasField($locator) */ public function findField($locator) { - return $this->find('named', array( - 'field', $this->getSelectorsHandler()->xpathLiteral($locator), - )); + return $this->find('named', array('field', $locator)); } /** @@ -245,9 +237,7 @@ public function uncheckField($locator) */ public function hasSelect($locator) { - return $this->has('named', array( - 'select', $this->getSelectorsHandler()->xpathLiteral($locator), - )); + return $this->has('named', array('select', $locator)); } /** @@ -281,9 +271,7 @@ public function selectFieldOption($locator, $value, $multiple = false) */ public function hasTable($locator) { - return $this->has('named', array( - 'table', $this->getSelectorsHandler()->xpathLiteral($locator), - )); + return $this->has('named', array('table', $locator)); } /** diff --git a/src/Selector/NamedSelector.php b/src/Selector/NamedSelector.php index 4845f994f..7c1186798 100644 --- a/src/Selector/NamedSelector.php +++ b/src/Selector/NamedSelector.php @@ -10,6 +10,8 @@ namespace Behat\Mink\Selector; +use Behat\Mink\Selector\Xpath\Escaper; + /** * Named selectors engine. Uses registered XPath selectors to create new expressions. * @@ -157,12 +159,15 @@ class NamedSelector implements SelectorInterface .//*[%idOrNameMatch%] XPATH ); + private $xpathEscaper; /** * Creates selector instance. */ public function __construct() { + $this->xpathEscaper = new Escaper(); + foreach ($this->replacements as $from => $to) { $this->replacements[$from] = strtr($to, $this->replacements); } @@ -217,7 +222,7 @@ public function translateToXPath($locator) $xpath = $this->selectors[$selector]; if (null !== $locator) { - $xpath = strtr($xpath, array('%locator%' => $locator)); + $xpath = strtr($xpath, array('%locator%' => $this->escapeLocator($locator))); } return $xpath; @@ -235,4 +240,24 @@ protected function registerReplacement($from, $to) { $this->replacements[$from] = $to; } + + private function escapeLocator($locator) + { + // If the locator looks like an escaped one, don't escape it again for BC reasons. + if ( + preg_match('/^\'[^\']*+\'$/', $locator) + || (false !== strpos($locator, '\'') && preg_match('/^"[^"]*+"$/', $locator)) + || ((8 < $length = strlen($locator)) && 'concat(' === substr($locator, 0, 7) && ')' === $locator[$length - 1]) + ) { + trigger_error( + 'Passing an excaped locator to the named selector is deprecated as of 1.7 and will be removed in 2.0.' + .' Pass the raw value instead.', + E_USER_DEPRECATED + ); + + return $locator; + } + + return $this->xpathEscaper->escapeLiteral($locator); + } } diff --git a/src/Selector/SelectorsHandler.php b/src/Selector/SelectorsHandler.php index a19de6751..28b9a7884 100644 --- a/src/Selector/SelectorsHandler.php +++ b/src/Selector/SelectorsHandler.php @@ -114,12 +114,22 @@ public function selectorToXpath($selector, $locator) /** * Translates string to XPath literal. * + * @deprecated since Mink 1.7. Use \Behat\Mink\Selector\Xpath\Escaper::escapeLiteral when building Xpath + * or pass the unescaped value when using the named selector. + * * @param string $s * * @return string */ public function xpathLiteral($s) { + trigger_error( + 'The '.__METHOD__.' method is deprecated as of 1.7 and will be removed in 2.0.' + .' Use \Behat\Mink\Selector\Xpath\Escaper::escapeLiteral instead when building Xpath' + .' or pass the unescaped value when using the named selector.', + E_USER_DEPRECATED + ); + return $this->escaper->escapeLiteral($s); } } diff --git a/tests/Element/ElementTest.php b/tests/Element/ElementTest.php index a8c8a8ca9..8e73af0b6 100644 --- a/tests/Element/ElementTest.php +++ b/tests/Element/ElementTest.php @@ -36,11 +36,6 @@ protected function setUp() $this->selectors = $this->getMockBuilder('Behat\Mink\Selector\SelectorsHandler')->getMock(); $this->session = new Session($this->driver, $this->selectors); - - $this->selectors - ->expects($this->any()) - ->method('xpathLiteral') - ->will($this->returnArgument(0)); } protected function mockNamedFinder($xpath, array $results, $locator, $times = 2) diff --git a/tests/Selector/NamedSelectorTest.php b/tests/Selector/NamedSelectorTest.php index e0de1bc22..80ed74a26 100644 --- a/tests/Selector/NamedSelectorTest.php +++ b/tests/Selector/NamedSelectorTest.php @@ -3,7 +3,7 @@ namespace Behat\Mink\Tests\Selector; use Behat\Mink\Selector\NamedSelector; -use Behat\Mink\Selector\SelectorsHandler; +use Behat\Mink\Selector\Xpath\Escaper; abstract class NamedSelectorTest extends \PHPUnit_Framework_TestCase { @@ -42,10 +42,6 @@ public function testSelectors($fixtureFile, $selector, $locator, $expectedExactC $dom = new \DOMDocument('1.0', 'UTF-8'); $dom->loadHTML(file_get_contents(__DIR__.'/fixtures/'.$fixtureFile)); - // Escape the locator as Mink 1.x expects the caller of the NamedSelector to handle it - $selectorsHandler = new SelectorsHandler(); - $locator = $selectorsHandler->xpathLiteral($locator); - $namedSelector = $this->getSelector(); $xpath = $namedSelector->translateToXPath(array($selector, $locator)); @@ -56,6 +52,20 @@ public function testSelectors($fixtureFile, $selector, $locator, $expectedExactC $this->assertEquals($expectedCount, $nodeList->length); } + /** + * @dataProvider getSelectorTests + */ + public function testEscapedSelectors($fixtureFile, $selector, $locator, $expectedExactCount, $expectedPartialCount = null) + { + // Escape the locator as Mink 1.x expects the caller of the NamedSelector to handle it + $escaper = new Escaper(); + $locator = $escaper->escapeLiteral($locator); + + $this->iniSet('error_reporting', -1 & ~E_USER_DEPRECATED); + + $this->testSelectors($fixtureFile, $selector, $locator, $expectedExactCount, $expectedPartialCount); + } + public function getSelectorTests() { $fieldCount = 8; // fields without `type` attribute @@ -122,6 +132,7 @@ public function getSelectorTests() // 3 matches, because matches every HTML node in path: html > body > div 'content' => array('test.html', 'content', 'content-text', 1, 4), + 'content with quotes' => array('test.html', 'content', 'some "quoted" content', 1, 3), 'select (name/label)' => array('test.html', 'select', 'the-field', 3), 'select (with-id)' => array('test.html', 'select', 'the-field-select', 1), diff --git a/tests/Selector/SelectorsHandlerTest.php b/tests/Selector/SelectorsHandlerTest.php index a20d3d9db..d949683b7 100644 --- a/tests/Selector/SelectorsHandlerTest.php +++ b/tests/Selector/SelectorsHandlerTest.php @@ -75,6 +75,8 @@ public function testXpathLiteral() { $handler = new SelectorsHandler(); + $this->iniSet('error_reporting', -1 & ~E_USER_DEPRECATED); + $this->assertEquals("'some simple string'", $handler->xpathLiteral('some simple string')); } diff --git a/tests/Selector/fixtures/test.html b/tests/Selector/fixtures/test.html index c64ae1032..1190e9ea6 100644 --- a/tests/Selector/fixtures/test.html +++ b/tests/Selector/fixtures/test.html @@ -66,6 +66,8 @@ some content-text +
some "quoted" content
+