Skip to content

Fix QPXML::pi($prefix) always returning NULL - #87

Merged
jakejackson1 merged 2 commits into
GravityPDF:mainfrom
samipuro:fix/qpxml-pi-prefix
Oct 25, 2026
Merged

jakejackson1 merged 2 commits into
GravityPDF:mainfrom
samipuro:fix/qpxml-pi-prefix

Conversation

@samipuro

Copy link
Copy Markdown
Contributor

Pull Request type

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: #79

pi($prefix) compares against tagName, which DOMProcessingInstruction does not have, so it always returns NULL with an "Undefined property" warning.

What is the new behavior?

  • pi($prefix) compares against the processing instruction's target and returns the matching instruction
  • A processing instruction written with pi($prefix, $text) can be read back by the same prefix

Does this introduce a breaking change?

  • Yes
  • No

Other information

Adds QPXMLTest::testProcessingInstructionByPrefix and a CHANGELOG entry.

@samipuro
samipuro force-pushed the fix/qpxml-pi-prefix branch from bc49eac to 57d4105 Compare October 25, 2026 13:46
@samipuro

Copy link
Copy Markdown
Contributor Author

FYI, the failing checks are not caused by this change.

Upstream runners now get ubuntu-26.04 for ubuntu-latest, and unchanged main fails there with the same 27 failures and 1 error (mostly Issue65Test, plus testFailedParseHTMLElement): https://github.com/samipuro/querypath/actions/runs/40200191915

The only change in that run is runs-on: ubuntu-26.04. This branch passes on ubuntu-24.04 on all PHP versions: https://github.com/samipuro/querypath/actions/runs/40200202244

The same applies to #88 and #89 🙂

@codecov

codecov Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.89%. Comparing base (7ffe1aa) to head (87a7ffa).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main      #87      +/-   ##
============================================
+ Coverage     89.84%   89.89%   +0.05%     
  Complexity     1430     1430              
============================================
  Files            26       26              
  Lines          3189     3186       -3     
============================================
- Hits           2865     2864       -1     
+ Misses          324      322       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Read the existing round-trip test back by prefix instead of repeating it,
and describe the fix by what pi() returns.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jakejackson1

Copy link
Copy Markdown
Member

@samipuro thanks so much for the contributions! The CI is fixed up now and I'll get your PRs merged. 👍

@jakejackson1
jakejackson1 merged commit 462a75d into GravityPDF:main Oct 25, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants