Merge pull request #641 from stof/escape_named_selector

Moved the escaping of the xpath locator to the NamedSelector

Author: stof

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);

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));
     }
 

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));
     }
 }

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);

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);

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));
     }
 
     /**

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);
+    }
 }

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);
     }
 }

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)

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),