Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Nested attribute selectors #21

Merged
merged 3 commits into from
Apr 9, 2018
Merged

Nested attribute selectors #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc6e98d
Add a (failing) test for nested attribute selectors
MidnightDesign Apr 5, 2018
42ad28e
Be more conservative with attribute selector values
MidnightDesign Apr 5, 2018
00a36de
Fix regular expression
MidnightDesign Apr 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Document/Query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public static function cssToXpath($path)

// Arbitrary attribute value contains whitespace
$path = preg_replace_callback(
'/\[\S+["\'](.+)["\']\]/',
'/\[\S+?["\'](.+?)["\']\]/',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking about this regexp here, and I think we should use backreference here to match match always "..." or '...' and not "...' or '...". But it's separate issue/improvement ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think that may be necessary. The problem is that if you have a ", and then a ' later, this will match... which will give you an incomplete match for the attribute value. Consider something like this: "javascript:alert('foo')". The above will return "javascript:alert'` as the match. So a backreference is definitely needed here to ensure that we search until we find a matching closing character.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weierophinney It is, yes. But that has nothing to with this PR. That problem already existed and should probably be fixed in a separate PR. If you want it to be fixed right here, you're welcome to update this one. I'm not good enough with regular expressions.

function ($matches) {
return str_replace($matches[1], preg_replace('/\s+/', '\s', $matches[1]), $matches[0]);
},
Expand Down
6 changes: 6 additions & 0 deletions test/Document/QueryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,4 +172,10 @@ public function testCanTransformWithAttributeAndDot()
$test = Query::cssToXpath('a[@href="http://example.com"]');
$this->assertEquals("//a[@href='http://example.com']", $test);
}

public function testTransformNestedAttributeSelectors()
{
$test = Query::cssToXpath('select[name="foo"] option[selected="selected"]');
$this->assertEquals("//select[@name='foo']//option[@selected='selected']", $test);
}
}