[DomCrawler] filterXPath limited usage since 2.3.14

[dol]

I used the DomCrawler::filterXPath method to do some filtering. Since this update 80438c2, the filterXPath method can't handle a simple queries like 'child::div'.
The error is the same as in #10986 .

Warning: DOMXPath::query(): Invalid expression in /vendor/symfony/symfony/src/Symfony/Component/DomCrawler/Crawler.php on line 832

Most of the listed XPath axes have a problem with this change.

The main cause to this problem is, that the method 'relativize' will append 'self::' to every expression, that's not matched in here.

As a result to this, the XPath expression is invalid.
E.g:
Input

child::div

relativized Xpath

self::child::div

The W3C spec don't allow Xpath expressions like the relativized one.

[fabpot]

ping @stof

[xabbuh]

The issue isn't limited to the child:: axis, but also affects nearly all other possible axes defined in the W3C spec.

[dol]

@fabpot + @xabbuh Thank you for the fix of the axis.

Can someone explain the idea behind the relativize of the XPath expressions?
IMHO is nearly impossible to get this right due to the complexity of XPath. I tested some simple XPath expressions like 'string' (string) or count(*) (function) and the relativize routine generates an invalid XPath expression.

To get more test cases, I extracted the XPath expression from this test suite. I removed XSLT specific XPath expressions. My test case file can be found at this gist.

All the XPath expression in 01_xpath_test_suite.txt are valid when used with DOMXPath::query.
If I evaluate the same expressions via the Crawler::filterXPath it breaks in 572 of 1305 cases.
The list of breaking expressions can be found in the same gist.

A good example of the complexity is the XPath expression //ancestor::* that is converted to descendant-or-self::ancestor::*. In this case, we have a similar problem as reported in this issue. The fix of this edge case might be very complex.

With the extended test case file it's IMHO possible to fix all broken cases. But I'm not convinced if the complexity to relativize an XPath is worth the benefit.

[xabbuh]

@stof Can you explain the reasons that made it necessary to introduce the relativize() method?

[stof]

@xabbuh the issue is that filterXPath mixes 2 responsibilities:

• filtering the current collection
• finding children of elements in the collection

And depending of how you write the XPath, it actually mixes both. For instance, when using a CSS query h1 (which generates a XPAth query descendant-or-self::h1), it finds all h1 tags in the current collection or in any child.

the previous API was written this way because it was copying nodes to a separate document with a special root tag. So the collection elements were actually children of the root were the XPath was applied. This means that if you query for the descendant::h1, it needs to match elements of the collection, not descendants of these elements. This also means that ancestor was never working in Symfony (there was no ancestor previously), which is why it has not been taken into account in the migration. The "fix" done in #11548 is not a fix but a BC break. It changes the behavior of the method.

The way to avoid the need for relativize would be to change the API of the Crawler to have 2 different methods:

• a method filtering the collection for real (i.e. all elements of the new collections are elements of the previous collection too, not elements deeper in the DOM).
• a separate method for finding child elements matching a query.

If you look at jQuery, this is the equivalent of $.fn.filter() and $.fn.find() respectively. The current behavior of filterXPath has no equivalent in jQuery.

[xabbuh]

@stof We could remove the ancestor axes support again. This would also apply to the previous* and following* axis, wouldn't it?

But, the question is then, how you would handle invalid values passed to filterXPath()? Passing them to relativize() is rather confusing since the error message you get is really meaningless and confusing. Throwing an exception if the given XPath expression is supported, may be an option. But that would be a BC break, wouldn't it?

[stof]

@xabbuh yes. and for other axes, we need to relativize them to match the behavior of the previous implementation.

for other axes, we need to turn the query in a query matching nothing, to be BC with the way it worked in previous version (using ancestor::* was not throwing an exception in 2.3.0 but matching nothing). This is already what we do for /div (and we should do the same for child::div which is strictly equivalent but was missed in the update)

[xabbuh]

@stof Obviously, I do miss something, but I don't see why the other axes have to be relativized? Do you have an example in mind (we should at least add one or more tests for them to avoid future regressions).

[stof]

@xabbuh read again how the behavior of XPath is working. filterXPath considers that its XPath is applied on a virtual parent of the elements of the collection, not on the collection elements themselves (yeah, it is a weird API, but we are stuck with BC here)

[xabbuh]

@stof Thanks, after having a look how this was handled in old Symfony versions, I understood what you mean. #11624 should fix the behavior in a BC way.

[dol]

@stof Thank you for the BC fix. 🎁