Problem/Motivation

On libxml 2.9.14 there is a difference in DOMDocument::loadHTML() behavior. The failures are not present in PHP 8.3 with libxml 2.9.10 (tested in Debian bullseye vs bookworm).

Currently, Drupal PHP 8.3 image is using libxml 2.9.14, which causes failures in D7 tests:

https://www.drupal.org/pift-ci-job/2790709
https://www.drupal.org/pift-ci-job/2790712

Similar issue was investigated in D10, but it seems like the solution was to replace DOMDocument parsing with another library, which seems difficult for D7 in this phase.

See the full history here: #3383577: TextSummaryTest:testLength() fails on some libxml versions

Steps to reproduce

Run tests on PHP 8.3 with libxml 2.9.14

Proposed resolution

Find a workaround how to fix this changed behavior to have green tests again.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3397882

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

poker10 created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2441811: Upgrade filter system to HTML5

Drupal 10.2 solved the issue via #2441811: Upgrade filter system to HTML5

probably it needs some wrapper which is checking for libxml version or relax test somehow

poker10’s picture

poker10 commented

An answer from libxml2 team is that this was a deliberate change, so we need to find workaround as @andypost mentioned.

See: https://gitlab.gnome.org/GNOME/libxml2/-/issues/615

andypost’s picture

andypost
he/him
Primary language Russian
commented

Looks like checking for libxml version in test looks the most viable solution

poker10’s picture

poker10 commented

Yes, we can fix the test, but I am not sure if this change in libxml2 could not have a potential impact on the real core functionality in some edge cases (and that we will not hide some problems when we use this workaround). Other than that, I am not sure how many different "characters" are affected by this change - so far we know that parsing < is problematic, but there can be more I think.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

There's been 15 releases of libxml2 since 2.9.14
any chance that this issue is fixed in https://gitlab.gnome.org/GNOME/libxml2/-/releases/v2.12.3 v2.12.3?

poker10’s picture

poker10 commented

We can try it, but I do not think so, because when I raised this question here: https://gitlab.gnome.org/GNOME/libxml2/-/issues/615, the answer was it is not a bug, but a deliberate change in the library.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

ah ok, ya just looked at your issue 615 on github, so the new versions would likely not have changed from the 2.9.14
Funny they couldn't make it work for both html4 and html5

andypost’s picture

andypost
he/him
Primary language Russian
commented

There's bigger changes are coming in Dom PHP extension https://wiki.php.net/rfc/opt_in_dom_spec_compliance

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3282786: The libxml2 in buster has an HTML whitespace bug

poker10 opened merge request !8028

poker10’s picture

poker10 commented
Status: Active » Needs review

We discussed this issue with @Fabianx on Slack and we agreed it would be the best to just skip the testing of this edge-case for the LIBXML version >= 2.9.14. It seems like the change in libxml2 have not caused other issues with D7, as other tests are not failing.

I have created a MR here with the proposed fix (it just skips the testing of "<" when tested with filter_htmlcorrector in case the LIBXML version is >= 2.9.14. It was possible to do this in several ways, but I think this approach has the smallest impact (unfortunatelly it seems not easily possible to remove the array element entirely, as the for loop is testing the lenght by array key position).

All current tests seems to be green with this change: https://git.drupalcode.org/project/drupal/-/pipelines/170137 . Also the PHP 8.3 test is without this failure (only the session one is still present after applying the fix from there): https://www.drupal.org/pift-ci-job/2893113 .

For illustration, if we combine fix from this issue and fix from the other one (session failure), the 8.3 testing is green: https://git.drupalcode.org/project/drupal/-/pipelines/170105 (see the META issue).

Moving to Needs review.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Needs review » Reviewed & tested by the community

100% green here for 8028

poker10’s picture

poker10 commented
Issue tags: +Pending Drupal 7 commit

  • mcdruid committed 28234957 on 7.x 
    Issue #3397882 by poker10, andypost, joseph.olstad: [D7 PHP 8.3]...
mcdruid’s picture

mcdruid commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Agree that this looks like the most sensible pragmatic solution.

Also LOL at the dry humour in https://gitlab.gnome.org/GNOME/libxml2/-/issues/474

Thanks!

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

We're misusing the lib for practical reasons

this ? lol

Status: Fixed » Closed (fixed)

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