This is a spin-off from #933270: Taxonomy terms are incorrectly affected.

It turns out that the return value of ->xpath() is inconsistent depending on the versions of PHP and libxml. When there is no match to the XPath query, sometimes FALSE is returns, sometimes an empty array.

The problem is that count(FALSE) is 1, so we risk having false positives depending on how we assert the query result.

CommentFileSizeAuthor
#5 933856-xpath-return-value.patch3.76 KBdamien tournoud
#1 933856-xpath-return-value.patch3.76 KBdamien tournoud

Comments

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
StatusFileSize
new 933856-xpath-return-value.patch3.76 KB

We should standardize on an empty array, so that you can use the query result in a foreach(...) loop.

It turns out, the RDFa tests were completely broken. The selectors were all wrong:

- position() starts with 1, not with 0
- position() cannot be used in this way (it is relative to the node context)
- the class needs a @contain

This one should pass.

damien tournoud’s picture

damien tournoud commented
Priority: Normal » Critical

This is blocking #933270: Taxonomy terms are incorrectly affected, so it is critical.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
commented

Looks good.

+++ modules/simpletest/drupal_web_test_case.php
@@ -2180,7 +2180,11 @@ class DrupalWebTestCase extends DrupalTestCase {
+      // Some combinaisons of PHP / libxml versions return an empty array

Typo; should be "combinations".

Powered by Dreditor.

carlos8f’s picture

carlos8f commented
Priority: Critical » Normal

I agree on the usefulness of the patch, but strictly speaking it doesn't block #933270: Taxonomy terms are incorrectly affected. It's simply a matter of using $this->assertFalse($links) or $this->assertTrue(empty($links)) rather than $this->assertFalse(count($links)).

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 933856-xpath-return-value.patch3.76 KB

With text edit from #3.

Status: Needs review » Needs work

The last submitted patch, 933856-xpath-return-value.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ modules/simpletest/drupal_web_test_case.php
@@ -2180,7 +2180,11 @@ class DrupalWebTestCase extends DrupalTestCase {
+      return $result ? $result : array();

Shouldn't we check

$result === FALSE

or

is_bool($return)

?

Powered by Dreditor.

damien tournoud’s picture

damien tournoud commented

xpath() returns NULL, FALSE or array() depending on the PHP version and libxml2 version. We normalize all of those to array().

damien tournoud’s picture

damien tournoud commented
Status: Needs work » Needs review

#5: 933856-xpath-return-value.patch queued for re-testing.

mark trapp’s picture

mark trapp
he/him
Primary language English
commented

#977184: FormElementTestCase::testOptions() should fail, doesn't is the result of this bug. FormElementTestCase::testOptions() should fail, but for most people (including the HEAD tester), it doesn't.

Stevel’s picture

Stevel commented
Status: Needs review » Reviewed & tested by the community

This patch makes the RDFa tests pass on an environment where they were otherwise broken.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Ugh, that really sucks we have to work around a PHP lmiitation like that, but I don't really see another way... this patch blocks pgsql tests passing.

Committed to HEAD.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.