fix: update CI pipeline and fix test failures for newer PHP/libxml2#115
fix: update CI pipeline and fix test failures for newer PHP/libxml2#115
Conversation
- Update CI workflow: - Fix deprecated actions/cache@v4.0.2 -> actions/cache@v4 - Fix deprecated actions/upload-artifact@v3 -> actions/upload-artifact@v4 - Fix deprecated ::set-output -> use $GITHUB_OUTPUT - Update shivammathur/setup-php@2.16.0 -> @v2 - Update PHP matrix: drop EOL 7.0-7.2, add 8.2/8.3 - Update phpstan condition from php 7.4 to 8.1 - Fix vendor/voku/simple_html_dom AbstractDomParser.php: - Change invalid placeholder element names (starting with underscores) to valid HTML custom element names (htmlmin-wrapper, htmlmin-special-script) to fix compatibility with newer libxml2 versions - Fix src/voku/helper/HtmlMin.php: - Add null check for parentNode in protectTagHelper - Fix src/voku/helper/HtmlMinDomObserverOptimizeAttributes.php: - Use removeAttribute() instead of property assignment to fix attribute sorting when PHP 8 DOMElement has id/prefix as native properties - Update phpunit.xml to modern phpunit 9.3 schema format - Update tests/HtmlMinTest.php: - Fix expected values for attribute ordering (now correctly sorted) - Fix testNullParentNode expected value for malformed HTML Agent-Logs-Url: https://github.com/voku/HtmlMin/sessions/0a280232-c351-4acc-bf06-4b51437ab97e Co-authored-by: voku <264695+voku@users.noreply.github.com>
…threading concern Agent-Logs-Url: https://github.com/voku/HtmlMin/sessions/0a280232-c351-4acc-bf06-4b51437ab97e Co-authored-by: voku <264695+voku@users.noreply.github.com>
|
coverage: 94.656% (-0.9%) from 95.507% — copilot/update-github-ci-pipeline into master |
Apply fixes from StyleCI
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the PHPUnit configuration, improves attribute removal logic, and adds a patch to ensure compatibility with newer libxml2 versions by updating placeholder element names in the AbstractDomParser. Additionally, it includes a null-check for parent nodes to prevent potential errors. While the use of Reflection to modify private static properties is a necessary workaround for the libxml2 compatibility issue, it introduces tight coupling to internal implementation details that may require maintenance if the underlying dependency changes.
src/voku/helper/HtmlMin.php
Outdated
| // Patch simple_html_dom's placeholder element names to use valid HTML custom | ||
| // element names. The default names (starting with underscores) are treated as | ||
| // invalid by newer libxml2 versions and cause the placeholder mechanism to fail. | ||
| // This is idempotent: it checks the current value before patching. | ||
| $ref = new \ReflectionProperty(\voku\helper\AbstractDomParser::class, 'domHtmlWrapperHelper'); | ||
| $ref->setAccessible(true); | ||
| if (\strpos((string) $ref->getValue(), '____') === 0) { | ||
| $ref->setValue(null, 'htmlmin-wrapper'); | ||
|
|
||
| $ref2 = new \ReflectionProperty(\voku\helper\AbstractDomParser::class, 'domHtmlSpecialScriptHelper'); | ||
| $ref2->setAccessible(true); | ||
| $ref2->setValue(null, 'htmlmin-special-script'); | ||
| } |
There was a problem hiding this comment.
Using ReflectionProperty to modify static private properties of AbstractDomParser can be fragile. This approach creates a tight coupling to the internal implementation details of the AbstractDomParser class. If the AbstractDomParser class changes its internal structure (e.g., renames domHtmlWrapperHelper or domHtmlSpecialScriptHelper), this patch will break. While it addresses a compatibility issue with libxml2, it's a workaround that might require re-evaluation if the dependency is updated.
|
@copilot Review the following cleaned github pull request excerpt. fix: update CI pipeline and fix test failures for newer PHP/libxml2#115 Updates the GitHub CI pipeline to remove deprecated actions and fixes PHP/libxml2 compatibility issues that caused test failures. Source fixes (HtmlMin.php, HtmlMinDomObserverOptimizeAttributes.php) Config/tests This change is Reviewable Coverage Status coverage: 95.256% (-0.3%) from 95.507% — copilot/update-github-ci-pipeline into master Failed conditions See analysis details on SonarQube Cloud Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE /gemini review This pull request updates the PHPUnit configuration, improves attribute removal logic, and adds a patch to ensure compatibility with newer libxml2 versions by updating placeholder element names in the AbstractDomParser. Additionally, it includes a null-check for parent nodes to prevent potential errors. While the use of Reflection to modify private static properties is a necessary workaround for the libxml2 compatibility issue, it introduces tight coupling to internal implementation details that may require maintenance if the underlying dependency changes. Using ReflectionProperty to modify static private properties of AbstractDomParser can be fragile. This approach creates a tight coupling to the internal implementation details of the AbstractDomParser class. If the AbstractDomParser class changes its internal structure (e.g., renames domHtmlWrapperHelper or domHtmlSpecialScriptHelper), this patch will break. While it addresses a compatibility issue with libxml2, it's a workaround that might require re-evaluation if the dependency is updated. 3 failing, 1 neutral, 12 successful checks |
There was a problem hiding this comment.
Code Review
This pull request updates the PHPUnit configuration to version 9.3 and introduces a reflection-based patch in the HtmlMin constructor to ensure compatibility with newer libxml2 versions by using valid custom element names. Additionally, it adds a null check for parent nodes in protectTagHelper and switches to removeAttribute for attribute cleanup. Feedback suggests optimizing the reflection logic by using a static variable to ensure it only executes once per request.
src/voku/helper/HtmlMin.php
Outdated
| // Patch simple_html_dom's placeholder element names to use valid HTML custom | ||
| // element names. The default names (starting with underscores) are treated as | ||
| // invalid by newer libxml2 versions and cause the placeholder mechanism to fail. | ||
| // This is idempotent: it checks the current value before patching. | ||
| $ref = new \ReflectionProperty(\voku\helper\AbstractDomParser::class, 'domHtmlWrapperHelper'); | ||
| $ref->setAccessible(true); | ||
| if (\strpos((string) $ref->getValue(), '____') === 0) { | ||
| $ref->setValue(null, 'htmlmin-wrapper'); | ||
|
|
||
| $ref2 = new \ReflectionProperty(\voku\helper\AbstractDomParser::class, 'domHtmlSpecialScriptHelper'); | ||
| $ref2->setAccessible(true); | ||
| $ref2->setValue(null, 'htmlmin-special-script'); | ||
| } |
There was a problem hiding this comment.
To improve performance, this reflection-based patch should only run once per request. Using a static variable ensures the patching logic is executed only during the first instantiation of HtmlMin, avoiding redundant reflection overhead on subsequent instantiations.
static $isDomParserPatched = false;
if ($isDomParserPatched === false) {
// Patch simple_html_dom's placeholder element names to use valid HTML custom
// element names. The default names (starting with underscores) are treated as
// invalid by newer libxml2 versions and cause the placeholder mechanism to fail.
// This is idempotent: it checks the current value before patching.
$ref = new \ReflectionProperty(\voku\helper\AbstractDomParser::class, 'domHtmlWrapperHelper');
$ref->setAccessible(true);
if (\strpos((string) $ref->getValue(), '____') === 0) {
$ref->setValue(null, 'htmlmin-wrapper');
$ref2 = new \ReflectionProperty(\voku\helper\AbstractDomParser::class, 'domHtmlSpecialScriptHelper');
$ref2->setAccessible(true);
$ref2->setValue(null, 'htmlmin-special-script');
}
$isDomParserPatched = true;
}…nd docblock - Move placeholder patching from constructor into patchSimpleHtmlDomPlaceholders() so it has a self-documenting name and reduced cognitive complexity in the ctor - Add try/catch with trigger_error warning so a failed patch (e.g. after a vendor rename) is surfaced rather than silently broken - Docblock explains: why Reflection is necessary (self:: compile-time binding makes subclassing ineffective), the libxml2 version where the issue appears, and a TODO for when upstream fixes the placeholder names Agent-Logs-Url: https://github.com/voku/HtmlMin/sessions/8fcd3cdb-8f2a-4343-baa2-5c3c012f017b Co-authored-by: voku <264695+voku@users.noreply.github.com>
... The concern about tight coupling is valid. Subclassing won't help here because all vendor accesses use I've addressed the fragility concern in db1e512 by:
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:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
patchSimpleHtmlDomPlaceholders()private static methodThis change is