Allow pipes (|) in attributes by implementing better XPath parsing #720
Conversation
| } | ||
|
|
||
| // Split any unions into individual expressions. | ||
| foreach (preg_split(self::UNION_PATTERN, $xpath) as $expression) { |
There was a problem hiding this comment.
self::UNION_PATTERN constant should be removed too. Or is used in other places?
There was a problem hiding this comment.
it is used further above and for backwards compatibility reasons we should not remove it, other libraries might rely on it.
|
How do I see what failed on Scrutinizer? It just tells me that one failure condition exists, but no details? |
src/Selector/Xpath/Manipulator.php
Outdated
| * | ||
| * @return string[] | ||
| */ | ||
| protected function splitUnionParts($xpath) |
src/Selector/Xpath/Manipulator.php
Outdated
| // Split any unions into individual expressions. We need to iterate | ||
| // through the string to correctly parse opening/closing quotes and | ||
| // braces which is not possible with regular expressions. | ||
| $union_parts = array(); |
| 'containing pipe' => array( | ||
| 'some_xpath', | ||
| "some_tag[(contains(normalize-space(string(.)), 'foo|bar') | other_tag[contains(./@some_attribute, 'foo|bar')])]", | ||
| "some_xpath/some_tag[(contains(normalize-space(string(.)), 'foo|bar') | other_tag[contains(./@some_attribute, 'foo|bar')])]", |
There was a problem hiding this comment.
wrong expectation. Should be some_xpath/other_tag for the second part of the union
There was a problem hiding this comment.
well, that's the question: the original regex suggests that within square brackets expressions should not be prefixed. Are you saying that we should also change the 'complex condition' test case further above to produce a different result?
There was a problem hiding this comment.
ah sorry, I missed the fact it was inside square braces
|
While we wait for feedback here I'm now integrating my Mink fork in our PHPUnit initiative development branch for Drupal klausi/drupal@9db23a2 |
src/Selector/Xpath/Manipulator.php
Outdated
| // braces which is not possible with regular expressions. | ||
| $unionParts = array(); | ||
| $singleQuoteOpen = false; | ||
| $doubleQuoteOpen = false; |
There was a problem hiding this comment.
I would name them $inSingleQuotedString and $inDoubleQuotedString. It would better reflect their usage
src/Selector/Xpath/Manipulator.php
Outdated
| $bracketOpen = 0; | ||
| $lastUnion = 0; | ||
| for ($i = 0; $i < strlen($xpath); $i++) { | ||
| if ($xpath{$i} === "'" && $doubleQuoteOpen === false) { |
src/Selector/Xpath/Manipulator.php
Outdated
| for ($i = 0; $i < strlen($xpath); $i++) { | ||
| if ($xpath{$i} === "'" && $doubleQuoteOpen === false) { | ||
| if ($singleQuoteOpen === true) { | ||
| $singleQuoteOpen = false; |
There was a problem hiding this comment.
can be simplified: $singleQuoteOpen = !$singleQuoteOpen
src/Selector/Xpath/Manipulator.php
Outdated
| } else { | ||
| $singleQuoteOpen = true; | ||
| } | ||
| } else if ($xpath{$i} === '"' && $singleQuoteOpen === false) { |
There was a problem hiding this comment.
should be elseif without space to match our coding standards
src/Selector/Xpath/Manipulator.php
Outdated
| $doubleQuoteOpen = true; | ||
| } | ||
| } else if ($singleQuoteOpen === false && $doubleQuoteOpen === false) { | ||
| if ($xpath{$i} === '[') { |
There was a problem hiding this comment.
please use $xpath[$i] rather than $xpath{$i} to access the element.
And I think defining a $char variable may be more readable.
src/Selector/Xpath/Manipulator.php
Outdated
| $unionParts = array(); | ||
| $singleQuoteOpen = false; | ||
| $doubleQuoteOpen = false; | ||
| $bracketOpen = 0; |
There was a problem hiding this comment.
$bracketLevel or $openedBrackets would be a better name
|
@klausi, looks good, better than trying with regexp. That seems to be an impossible try. When this gets committed we need only to refresh composer.lock in https://www.drupal.org/node/2808085 |
|
thanks @klausi |
|
FYI, the discussion on the related Symfony BrowserKit PR highlighted the high performance impact of this change. So I'm looking at the progress on the alternate implemented proposed by @nicolas-grekas in symfony/symfony#20235 before making a Mink release (to use it instead if appropriate) |
3 participants
For issue #702 .
The pipe operator can appear in many places - a regex is too primitive, we need to actually parse the XPath expression.
This also uncovered a bug in the XPath expressions used for testing which contain a double single quote, which would be a broken XPath expression.