Add XPath predicate support to XPathExpressionDiscoverer by Copilot · Pull Request #127 · mvdbos/php-spider

Copilot · 2026-01-17T14:25:29Z

Implementation Complete ✅

Summary

Successfully implemented support for square-bracket notation (XPath predicates) in XPathExpressionDiscoverer with all code review feedback and static analysis issues addressed.

Changes Made

Created SimpleXPathExpressionDiscoverer - Preserves original strict behavior for backward compatibility
Enhanced XPathExpressionDiscoverer - Now supports advanced XPath expressions with predicates like //a[starts-with(@href, '/')]
Fixed Critical Validation Issue - Updated regex from #//a(\[|/|$)# to #//a(\[[^\]]*\])*$# to properly reject selectors where anchor is not the final element
Added Missing Test Coverage - Added tests for invalid patterns like //a[@class='link']//span, //a/text(), and //a/@href
Improved Documentation - Clarified that SimpleXPathExpressionDiscoverer supports predicates in the path but not on the anchor element itself
Improved Error Messages - Made error messages more consistent between both discoverers
Updated Test Comments - Clarified comments to be more accurate about what is and isn't supported
Documented Limitations - Added note about nested brackets limitation (extremely rare edge case)
Fixed Static Analysis Issues - Removed trailing whitespace to pass phpcs PSR2 checks

Testing Results

✅ Unit Tests: 87/87 passed (26 discoverer tests including 3 new invalid pattern tests)
✅ Code Coverage: 100% for SimpleXPathExpressionDiscoverer and XPathExpressionDiscoverer
✅ Static Analysis: All checks pass (phpcs, phpmd, phan)
✅ Validation: Properly rejects invalid patterns that select non-anchor elements
✅ No regressions introduced

New Validation Behavior

The enhanced regex #//a(\[[^\]]*\])*$# now correctly:

✅ Accepts: //a, //a[starts-with(@href, '/')], //div[@id='content']//a
❌ Rejects: //a[@class='link']//span, //a/text(), //a/@href

Known Limitation

XPath predicates containing nested brackets (e.g., //a[contains(@href, '[test]')]) are not supported due to regex limitations. This is an extremely rare edge case that doesn't affect common use cases.

Original prompt

This section details on the original issue you should resolve

<issue_title>Allow square-bracket notation after anchor selector</issue_title>
<issue_description>Currently the XPathExpressionDiscoverer allows selectors ending with /a only. This means being more specific by using the square-bracket notation is not supported. However, this would make the spider so much more powerful and we wouldn't even have to change a lot.

An example of the square-bracket notation could be the following.
//a[starts-with(@href, '/') or starts-with(@href, '$url')]
To allow this, spider could either be less strict about the selector argument (maybe replacing endsWith with a regular expression) or move the validation of the selector argument away from the constructor (to a protected function maybe), such that when extending the XPathExpressionDiscoverer you could override such validation method and have your own selector validation.</issue_description>

Comments on the Issue (you are @copilot in this section)
@mvdbos First of all, apologies for the late reaction. My day job has been busy.
The power of the spider is that it can use any Discoverer, and you can implement custom ones too. The examples show how to set the one you want to use.

Your suggestion sounds like a great idea. Would you be open to create an implementation for the improved XPathExpressionDiscoverer? I would be happy to adopt it as the new one. We could rename the current one to SimpleXPathExpressionDiscoverer. example_simple.php could then keep using the simple discoverer and example_complex could use the new one with bracket notation.

Fixes Allow square-bracket notation after anchor selector #105

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

…erer with bracket notation support Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

…standard annotations Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

Copilot

Pull request overview

This PR adds XPath predicate support (square-bracket notation) to the XPathExpressionDiscoverer by introducing a regex-based validation approach. To maintain backward compatibility, the original strict validation is preserved in a new SimpleXPathExpressionDiscoverer class.

Changes:

Created SimpleXPathExpressionDiscoverer with original strict /a ending validation
Enhanced XPathExpressionDiscoverer to accept predicates using regex pattern //a(\[|/|$)
Added comprehensive test coverage for both discoverers including predicate, path, and error cases

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/Discoverer/SimpleXPathExpressionDiscoverer.php	New class preserving original strict validation requiring selectors to end with `/a`
src/Discoverer/XPathExpressionDiscoverer.php	Enhanced with regex validation to support XPath predicates and complex expressions
tests/Discoverer/SimpleXPathExpressionDiscovererTest.php	Comprehensive test suite for simple discoverer including rejection of bracket notation
tests/Discoverer/XpathExpressionDiscovererTest.php	Extended tests covering predicates, nested paths, and multiple predicates
example/example_simple.php	Updated to use SimpleXPathExpressionDiscoverer for backward compatibility
example/example_complex.php	Updated to demonstrate predicate usage with XPathExpressionDiscoverer

Copilot · 2026-01-17T14:49:25Z

tests/Discoverer/XpathExpressionDiscovererTest.php

+
+        $discoverer = new XPathExpressionDiscoverer("//div[@id='content']");
+        $this->executeDiscoverer($discoverer);
+    }


Missing test coverage for XPath expressions where anchors are not the final selected element. The validation should reject expressions like //a[@class='link']//span or //a/text() that would select non-anchor elements or attributes. Add test cases to verify these invalid patterns are properly rejected, as they would fail at runtime when the discoverer attempts to extract href attributes from elements that don't have them.

Fixed in commit 08ffeab. Added test cases for testDiscoverWithAnchorFollowedByDescendant, testDiscoverWithAnchorText, and testDiscoverWithAnchorAttribute to verify invalid patterns are properly rejected.

Copilot · 2026-01-17T14:49:26Z

tests/Discoverer/SimpleXPathExpressionDiscovererTest.php

+        $this->expectException(InvalidArgumentException::class);
+        $this->expectExceptionMessage("Please end your selector with '/a'");
+
+        // SimpleXPathExpressionDiscoverer does not support bracket notation


The comment "SimpleXPathExpressionDiscoverer does not support bracket notation" is misleading. The test on line 48 shows that SimpleXPathExpressionDiscoverer DOES accept bracket notation in the path before the anchor (e.g., //div[@id='content']//a). It only rejects bracket notation on the anchor element itself (e.g., //a[...]). Consider clarifying this comment to state "SimpleXPathExpressionDiscoverer does not support bracket notation on anchor elements" for accuracy.

Suggested change

// SimpleXPathExpressionDiscoverer does not support bracket notation

// SimpleXPathExpressionDiscoverer does not support bracket notation on anchor elements

Fixed in commit 08ffeab. Updated comment to: "SimpleXPathExpressionDiscoverer does not support bracket notation on anchor elements".

Copilot · 2026-01-17T14:49:26Z

src/Discoverer/SimpleXPathExpressionDiscoverer.php

+ * 
+ * For more advanced XPath expressions with predicates (square-bracket notation),
+ * use XPathExpressionDiscoverer instead.
+ * 


The documentation states "For more advanced XPath expressions with predicates (square-bracket notation), use XPathExpressionDiscoverer instead" but this is imprecise. SimpleXPathExpressionDiscoverer does support predicates in the XPath path before the anchor (e.g., //div[@id='content']//a is valid). It only restricts predicates directly on the anchor element itself. Consider clarifying: "For XPath expressions with predicates on anchor elements (square-bracket notation on the anchor), use XPathExpressionDiscoverer instead."

Suggested change

*

* For more advanced XPath expressions with predicates (square-bracket notation),

* use XPathExpressionDiscoverer instead.

*

*

* For XPath expressions with predicates on anchor elements (square-bracket

* notation on the anchor), use XPathExpressionDiscoverer instead.

*

Fixed in commit 08ffeab. Updated documentation to clarify: "For XPath expressions with predicates on anchor elements (square-bracket notation on the anchor), use XPathExpressionDiscoverer instead."

Copilot · 2026-01-17T14:49:26Z

src/Discoverer/XPathExpressionDiscoverer.php

+ * - //div[@id='content']//a
+ * - //a[@class='internal']
+ * 
+ * For simple selectors that just end with '/a', you may also use SimpleXPathExpressionDiscoverer.


The documentation suggests SimpleXPathExpressionDiscoverer is for "simple selectors that just end with '/a'", but SimpleXPathExpressionDiscoverer actually supports complex paths like //div[@id='content']//a as long as the anchor has no predicates. The distinction is not about path complexity but about whether the anchor element itself has predicates. Consider clarifying: "For selectors where the anchor element has no predicates, you may also use SimpleXPathExpressionDiscoverer."

Suggested change

* For simple selectors that just end with '/a', you may also use SimpleXPathExpressionDiscoverer.

* For selectors where the anchor element has no predicates (for example, //div[@id='content']//a), you may also use SimpleXPathExpressionDiscoverer.

Fixed in commit 08ffeab. Updated documentation to clarify: "For selectors where the anchor element has no predicates (for example, //div[@id='content']//a), you may also use SimpleXPathExpressionDiscoverer."

Copilot · 2026-01-17T14:49:26Z

src/Discoverer/XPathExpressionDiscoverer.php

-        return substr($haystack, -$length) === $needle;
+        // Match patterns that indicate the selector targets anchor elements
+        // Ensures //a is present and followed by [, /, or end of string
+        return preg_match('#//a(\[|/|$)#', $selector) === 1;


The validation regex #//a(\[|/|$)# accepts XPath expressions where anchor elements appear anywhere in the path, not necessarily as the final selected element. For example, //a[@class='link']//span would pass validation but would select span elements (not anchors), which lack href attributes that the discoverer tries to extract. The regex should ensure that the anchor element is the final element in the XPath, such as by using #//a(\[|$)# or ensuring no path separator follows after the anchor predicates.

Fixed in commit 08ffeab. Updated regex from #//a(\[|/|$)# to #//a(\[[^\]]*\])*$# to ensure anchor is the final element. Now properly rejects patterns like //a[@class='link']//span.

Copilot · 2026-01-17T14:49:26Z

src/Discoverer/SimpleXPathExpressionDiscoverer.php

+            throw new InvalidArgumentException("Please end your selector with '/a': " .
+                "selectors should look for `a` elements " .
+                "so that the Discoverer can extract their `href` attribute for further crawling.");


The error message inconsistency could confuse users. The new XPathExpressionDiscoverer uses "must target anchor ('a') elements" while SimpleXPathExpressionDiscoverer says "Please end your selector with '/a'". Consider making the SimpleXPathExpressionDiscoverer error message also mention that it "must target anchor ('a') elements" for consistency, while still explaining the strict '/a' ending requirement.

Suggested change

throw new InvalidArgumentException("Please end your selector with '/a': " .

"selectors should look for `a` elements " .

"so that the Discoverer can extract their `href` attribute for further crawling.");

throw new InvalidArgumentException(

"SimpleXPathExpressionDiscoverer selectors must target anchor ('a') elements and " .

"must end with '/a' so that the Discoverer can extract their `href` attribute for further crawling."

);

Fixed in commit 08ffeab. Updated error message to be consistent: "SimpleXPathExpressionDiscoverer selectors must target anchor ('a') elements and must end with '/a' so that the Discoverer can extract their href attribute for further crawling."

Copilot · 2026-01-17T14:49:27Z

src/Discoverer/XPathExpressionDiscoverer.php

+    /**
+     * Validates that the selector targets anchor elements.
+     * 
+     * Accepts selectors that:
+     * - End with '//a' (simple case)
+     * - End with '//a[...]' (with predicates)
+     * - Contain '//a[' (anchor with predicates anywhere in the path)
+     * - Contain '//a/' (anchor followed by more path)


The documentation claims the validation "Accepts selectors that... Contain '//a/' (anchor followed by more path)" which would incorrectly allow selectors like //a[@id='nav']//span that target descendant elements of anchors rather than the anchors themselves. This contradicts the stated purpose that selectors "should look for a elements so that the Discoverer can extract their href attribute." The documentation should clarify that only selectors where anchors are the final selected elements are valid.

Fixed in commit 08ffeab. Updated validation and documentation to clarify that only selectors where anchors are the final selected elements are valid. The new regex #//a(\[[^\]]*\])*$# ensures this.

mvdbos · 2026-01-17T15:53:43Z

@copilot please fix the build issues

…eedback Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

Copilot · 2026-01-17T15:58:14Z

@copilot please fix the build issues

Build issues fixed in commit 08ffeab. All 87 tests now pass. The main fix was improving the validation regex to #//a(\[[^\]]*\])*$# to properly reject invalid XPath patterns, and adding comprehensive test coverage for edge cases.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

bar
- Triggering command: /usr/bin/php php ./vendor/bin/phpunit (dns block)
foobar.org
- Triggering command: /usr/bin/php php ./vendor/bin/phpunit (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

mvdbos · 2026-01-17T16:47:45Z

@copilot fix static analysis errors

Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

Copilot · 2026-01-17T16:50:52Z

@copilot fix static analysis errors

Fixed in commit 4be1720. Removed trailing whitespace from docblocks to pass phpcs PSR2 checks. All static analysis checks now pass (phpcs, phpmd, phan).

Initial plan

b1ab9d8

Copilot AI assigned Copilot and mvdbos Jan 17, 2026

Copilot started work on behalf of mvdbos January 17, 2026 14:25 View session

Copilot AI and others added 4 commits January 17, 2026 14:27

Initial plan for square-bracket notation support

5679fc3

Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

Add SimpleXPathExpressionDiscoverer and enhance XPathExpressionDiscov…

59d1d7e

…erer with bracket notation support Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

Fix escaped quotes in error message for better string formatting

7c7739d

Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

Address code review feedback: improve regex precision and remove non-…

3d05f28

…standard annotations Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Allow square-bracket notation after anchor selector~~ Add XPath predicate support to XPathExpressionDiscoverer Jan 17, 2026

Copilot AI requested a review from mvdbos January 17, 2026 14:41

Copilot finished work on behalf of mvdbos January 17, 2026 14:41

mvdbos marked this pull request as ready for review January 17, 2026 14:44

Copilot AI review requested due to automatic review settings January 17, 2026 14:44

Copilot started reviewing on behalf of mvdbos January 17, 2026 14:45 View session

Copilot AI reviewed Jan 17, 2026

View reviewed changes

Copilot started work on behalf of mvdbos January 17, 2026 15:54 View session

Fix validation regex and improve documentation based on code review f…

08ffeab

…eedback Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

Document known limitation with nested brackets in XPath predicates

4593340

Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

Copilot finished work on behalf of mvdbos January 17, 2026 16:00

Copilot started work on behalf of mvdbos January 17, 2026 16:48 View session

Fix static analysis errors: remove trailing whitespace

4be1720

Co-authored-by: mvdbos <1101757+mvdbos@users.noreply.github.com>

Copilot finished work on behalf of mvdbos January 17, 2026 16:51

mvdbos approved these changes Jan 17, 2026

View reviewed changes

mvdbos merged commit 593276f into master Jan 17, 2026
7 checks passed

mvdbos deleted the copilot/allow-square-bracket-notation branch January 17, 2026 17:23

	// SimpleXPathExpressionDiscoverer does not support bracket notation
	// SimpleXPathExpressionDiscoverer does not support bracket notation on anchor elements

	* For simple selectors that just end with '/a', you may also use SimpleXPathExpressionDiscoverer.
	* For selectors where the anchor element has no predicates (for example, //div[@id='content']//a), you may also use SimpleXPathExpressionDiscoverer.

Conversation

Copilot AI commented Jan 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation Complete ✅

Summary

Changes Made

Testing Results

New Validation Behavior

Known Limitation

Comments on the Issue (you are @copilot in this section)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

mvdbos commented Jan 17, 2026

Uh oh!

Copilot AI commented Jan 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

mvdbos commented Jan 17, 2026

Uh oh!

Copilot AI commented Jan 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Jan 17, 2026 •

edited

Loading

Copilot AI commented Jan 17, 2026 •

edited

Loading