Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests: Fix libxml >= 2.12 compatibility #797

Merged
merged 3 commits into from
Jan 25, 2025

Conversation

asispts
Copy link
Contributor

@asispts asispts commented Jan 25, 2025

Description

I've updated the tests to support both libxml < 2.12 and >= 2.12.

Suggested changelog entry

Related issues/external references

Fixes #767

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

@jrfnl
Copy link
Member

jrfnl commented Jan 25, 2025

Thanks for working on this @asispts. Quick question: why did you change the regex delimiter from a backtick to # ? Is that necessary for libxml 2.12 or just a personal preference ?

@asispts
Copy link
Contributor Author

asispts commented Jan 25, 2025

It's just a personal preference, and because It's hard to see "`" in the regex string. I'll change it back to "`" if needed.

@jrfnl
Copy link
Member

jrfnl commented Jan 25, 2025

It's just a personal preference, and because It's hard to see "" in the regex string. I'll change it back to "" if needed.

Yes please. I'd like to evaluate the PR on the changes actually needed for libxml 2.12 and not get distracted/confused by unrelated changes.

@asispts
Copy link
Contributor Author

asispts commented Jan 25, 2025

Done

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asispts Thank you very much for working on this.
While it would have been nice to have a test with a ruleset which would consistently display three errors on both libxml < 2.12 as well as >= 2.12, I can live with this. The underlying code handling errors from libxml is still being tested well enough to safeguard its functionality.

Thank you @asispts for investigating and unblocking this issue.

@remicollet This PR should mean that once PHPCS 3.12.0 has been released, you can remove the previously added @requires ... tag from the test docblock in the Fedora test setup. (will be confirmed before release via PR #798)

@jrfnl
Copy link
Member

jrfnl commented Jan 25, 2025

@asispts In the changelog, this project likes to give credit where credit is due and that means it normally contains the full name + handle of the contributor. If that's okay with you, would you mind sharing your name ? If not, I'll just use your GitHub handle. Okay ?

@jrfnl jrfnl merged commit 55b35bc into PHPCSStandards:master Jan 25, 2025
45 checks passed
@asispts asispts deleted the bugfix/issue-767 branch January 26, 2025 03:23
@asispts
Copy link
Contributor Author

asispts commented Jan 26, 2025

@jrfnl Yeah, it’s "Asis Pattisahusiwa"

@jrfnl
Copy link
Member

jrfnl commented Jan 26, 2025

@asispts Thanks. Noted.

@asispts asispts mentioned this pull request Jan 29, 2025
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests: make testBrokenRulesetMultiError() test compatible with LibXML 2.12+
2 participants