Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Repo metadata & export /.gitattributes, /.gitignore |
Added export-ignore for CHANGELOG.md; added comment and ignored patches.lock.json. |
CI & Sonar .github/workflows/ci.yml, .github/workflows/sonarcloud.yml |
Adjusted CI permissions and conditional coverage flow; Sonar job reads SONAR_TOKEN from job env and step condition updated. |
Coding standards & test config phpcs.xml, phpunit.xml.dist |
Added PHPCS PSR-12 config for src; disabled line-length rule; removed phpunit XML namespace attrs and added <source> filtering. |
Documentation & changelog CHANGELOG.md, README.md, docs/architecture.md |
New comprehensive CHANGELOG; clarified README license text; added architecture doc describing PSP pipeline, formats, and update flow. |
PSL update tooling bin/update-psl.php |
Added ext-intl detection and rule normalization via idn_to_ascii when available; improved HTTP cache handling (ETag/If-Modified-Since), explicit 304 handling, low-rule-count guard, and safer atomic temp-file cleanup. |
PSL data & metadata data/psl.cache.php, data/psl.meta.json |
Regenerated PSL cache: many Unicode labels replaced with punycode (xn--...); updated generation timestamp, etag, and last_modified. |
Core domain logic src/PublicSuffixList.php, src/RegisteredDomain.php |
Changed EXCEPTION rule handling to derive public suffix from right side of exception label; improved IPv6/bracketed host normalization and conditional port stripping to avoid corrupting IPv6 addresses. |
Tests tests/integration/..., tests/unit/... |
Added integration tests validating bundled PSL (punycode and Unicode flows) and exception cases; updated unit tests to reset static cache between tests, removed/retuned some unit cases, and expanded IPv6 test coverage. |
Misc docs & templates .github/ISSUE_TEMPLATE/bug-report.yml, .github/copilot-instructions.md |
Minor formatting and form option updates. |
Sequence Diagram(s)
sequenceDiagram
participant CLI as CLI (bin/update-psl.php)
participant Intl as ext-intl
participant Parser as Rule Parser
participant Validator as Rule Count Validator
participant Writer as Cache Writer
CLI->>Intl: check availability
alt ext-intl present
Intl-->>CLI: available
CLI->>Parser: parse rules + normalize to punycode
Parser->>Intl: idn_to_ascii conversions
Intl-->>Parser: ascii/punycode labels
Parser-->>CLI: normalized rules
else ext-intl absent
Intl-->>CLI: not available
CLI->>Parser: parse rules (retain Unicode)
Parser-->>CLI: unicode rules
end
CLI->>Validator: validate total rule count
alt valid count (>=1000)
Validator-->>CLI: pass
CLI->>Writer: write cache atomically (temp -> rename)
Writer->>Writer: clean temp on failure
Writer-->>CLI: cache written + metadata updated
else too few rules
Validator-->>CLI: abort update
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
- Feature/enhancements #8 — Overlapping edits to the update script, core domain handling, PSL cache/meta, CI, and tests; likely directly related.
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Title check | ❓ Inconclusive | The title 'updates' is vague and generic, providing no meaningful information about the substantial changes in this pull request. | Replace with a specific title that describes the main change, such as 'Add punycode normalization and PSL cache updates' or 'Update PSL cache with IDN punycode conversions and CI improvements'. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
No actionable comments were generated in the recent review. 🎉
Comment @coderabbitai help to get the list of available commands and usage tips.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
This PR updates RegDom’s PSL handling and related tooling/tests to improve correctness for PSL exception rules, IPv6 host normalization, and IDN (punycode) rule storage—along with CI/config hygiene updates.
Changes:
- Fix PSL exception-rule suffix calculation and add regression tests (unit + integration).
- Improve host normalization around bracketed IPv6 (and add IPv6 test cases).
- Normalize PSL cache rule keys to ASCII/punycode during updates and refresh bundled PSL cache/metadata.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/PublicSuffixList.php |
Fixes PSL exception-rule handling in getPublicSuffix(). |
src/RegisteredDomain.php |
Adjusts normalizeHost() to treat bracketed IPv6 literals specially. |
bin/update-psl.php |
Normalizes PSL rules to punycode when ext-intl is available; safer temp cleanup. |
data/psl.cache.php |
Refreshes bundled PSL cache; converts many IDN entries to punycode keys. |
data/psl.meta.json |
Updates PSL download metadata (ETag/Last-Modified/updated timestamp). |
tests/unit/PublicSuffixListTest.php |
Adds tests for exception rules and IDN public suffix behavior. |
tests/unit/RegisteredDomainTest.php |
Expands exception-rule and IPv6 normalization test coverage. |
tests/integration/IntegrationTest.php |
Adds real-world integration cases for IDN PS and exception subdomains. |
phpunit.xml.dist |
Adjusts PHPUnit config header; uses <source> for coverage filtering. |
phpcs.xml |
Adds a tracked PHPCS ruleset (PSR-12 baseline + exclusions). |
README.md |
Clarifies licensing split: Apache-2.0 for code, MPL-2.0 for PSL data. |
CHANGELOG.md |
Adds a changelog documenting the recent fixes and infra changes. |
.github/workflows/ci.yml |
Avoids running CI checks twice in the coverage matrix entry. |
.github/workflows/sonarcloud.yml |
Moves Sonar token to job env and uses env-based conditional execution. |
.gitattributes |
Adds CHANGELOG.md to export-ignore. |
| $host = str_contains($input, '/') ? parse_url($input, PHP_URL_HOST) : $input; | ||
| if (!is_string($host)) { | ||
| $host = ''; | ||
| } | ||
| $host = trim(mb_strtolower($host, 'UTF-8')); |
There was a problem hiding this comment.
normalizeHost() can still corrupt IPv6 hosts when the input is a URL. For example, parse_url("http://[::1]:443/", PHP_URL_HOST) yields "::1" (no brackets), so the later non-IPv6 "/:\d+$/" stripping will treat the last hextet as a port and truncate the address. Consider detecting unbracketed IPv6 via filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) (or using PHP_URL_PORT when parse_url is used) and skipping the port-stripping regex for IPv6.
| 'IPv6 bracketed' => ['[::1]', null], | ||
| 'IPv6 bracketed with port' => ['[::1]:443', null], | ||
| 'IPv6 full form' => ['[2001:db8::1]', null], | ||
| 'IPv6 full form with port' => ['[2001:db8::1]:8080', null], |
There was a problem hiding this comment.
The new IPv6 normalization cases cover raw bracketed hosts, but they don’t exercise the URL parsing path (normalizeHost() calls parse_url() when the input contains '/'). Please add a provider case like "http://[::1]:443/path" (and/or https) to prevent regressions where parse_url returns an unbracketed IPv6 host ("::1") and the port-stripping logic misbehaves.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@bin/update-psl.php`:
- Around line 102-105: The single-line if/elseif/else statements in the PSL
parsing loop must be converted to PSR-12-compliant blocks with braces: replace
the inline control structures that check empty($line), str_starts_with($line,
'//'), str_starts_with($line, '!'), and str_starts_with($line, '*.') with braced
blocks and keep the same logic that assigns into $rules['EXCEPTION'],
$rules['WILDCARD'], and $rules['NORMAL'] using $normalizeRule and substr so
linting passes.
In `@CHANGELOG.md`:
- Line 11: The Unreleased section mixes US and UK spellings of
"normalization/normalisation"; choose one variant (e.g., use the US
"normalization") and make a consistent replace across the CHANGELOG.md entries
referenced (including the line mentioning bin/update-psl.php and the occurrence
around line 39), and also ensure related mentions of normalizeDomain(),
idn_to_ascii(), and ext-intl remain unchanged except for the spelling swap so
the text is consistent.
- Around line 6-9: The markdown headings (e.g., "## [Unreleased]" and "### Bug
Fixes" shown in the diff) need blank lines before and after them to satisfy
markdownlint rule MD022; update the CHANGELOG.md by inserting a single empty
line above and below each heading in the affected ranges (including the heading
lines shown and the other ranges mentioned: 19-22, 25-33, 36-40, 47-52) so every
heading is separated by a blank line from surrounding content.
In `@phpcs.xml`:
- Around line 10-13: The current phpcs.xml disables the Generic.Files.LineLength
rule using an uncommon <exclude-pattern>*</exclude-pattern>; replace that by
setting the rule's severity to 0 (i.e., update the <rule
ref="Generic.Files.LineLength"> block to include <severity>0</severity> instead
of the <exclude-pattern>) or, if you want to allow longer lines but still
enforce limits, add <properties> with <property name="lineLimit" value="150"/>
and <property name="absoluteLineLimit" value="0"/> inside the same <rule
ref="Generic.Files.LineLength"> element to preserve conventional configuration.
- Around line 21-24: The phpcs.xml contains a redundant exclusion: you already
globally exclude test files using the existing global exclude pattern, so the
specific rule exclusion Squiz.Classes.ClassFileName.NoMatch with
<exclude-pattern>*/tests/*</exclude-pattern> is unnecessary; remove the rule
block referencing Squiz.Classes.ClassFileName.NoMatch (the
<rule>...<exclude-pattern>*/tests/*</exclude-pattern>...</rule> block) to avoid
dead configuration, or alternatively if you actually want to lint tests with
special rules, remove the global test exclusion instead—ensure you only keep one
of the two exclusions (either the global exclude or the rule-specific exclude).
In `@src/RegisteredDomain.php`:
- Around line 122-132: The current non-IPv6 branch in RegisteredDomain.php
strips trailing :port with preg_replace which corrupts bare IPv6 like "::1";
before applying the port-strip (the code around preg_replace('/:\d+$/', '',
$host)), guard with filter_var($host, FILTER_VALIDATE_IP) (or FILTER_VALIDATE_IP
with FILTER_FLAG_IPV6) and skip port-stripping if the host is already a valid IP
address (especially IPv6); update the logic in that function so valid IPs are
left untouched and only non-IP hosts have their trailing :port removed.
In `@tests/unit/PublicSuffixListTest.php`:
- Around line 89-96: The IDN assertions in testIsPublicSuffixWithIdnDomains fail
when ext-intl is missing; add a guard at the start of
PublicSuffixListTest::testIsPublicSuffixWithIdnDomains that checks for the IDN
support (e.g., extension_loaded('intl') or function_exists('idn_to_ascii')) and
calls $this->markTestSkipped with a short message if absent, so the rest of the
assertions against isPublicSuffix (which relies on normalizeDomain behavior)
only run when Unicode→punycode conversion is available.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/PublicSuffixListTest.php (1)
12-15:⚠️ Potential issue | 🟡 MinorMove these tests to
tests/integration/— they validate real PSL data, not isolated behavior.These tests require actual Public Suffix List data to be meaningful (
testGetPublicSuffixWithExceptionRulesverifies PSL exception rules,testIsPublicSuffixvalidates real TLDs and wildcards). Per coding guidelines, unit tests must use mockedPublicSuffixListfor isolation; integration tests use real PSL cache. Since these tests depend on real PSL behavior, they belong intests/integration/alongsideIntegrationTest.phpwherePublicSuffixListis already used with real data.
🤖 Fix all issues with AI agents
In `@docs/architecture.md`:
- Around line 8-20: Update the fenced code block containing the project tree so
it includes a language specifier (e.g., change the opening ``` to ```text) to
satisfy markdown lint rule MD040; locate the block that lists entries like
RegisteredDomain.php, PublicSuffixList.php,
Exception/PslCacheNotFoundException.php, bin/update-psl.php, and
data/psl.cache.php and add the `text` (or `plaintext`) specifier on the opening
fence.
- Around line 153-156: The documentation example incorrectly labels the punycode
form `xn--55qx5d.cn` as "Unicode input"; update the sentence that reads "Unicode
input (e.g. `xn--55qx5d.cn`)" to use the actual Unicode domain `公司.cn` instead,
ensuring the two bullets clearly show punycode (`xn--55qx5d.cn`) vs Unicode
(`公司.cn`) so readers can tell the difference.
- Around line 111-116: The fenced code block containing the PSL example (the
lines starting with "PSL entry: !city.kawasaki.jp", "Exception key:
city.kawasaki.jp", "Public suffix: kawasaki.jp (one label removed)") is
missing a language specifier; update the opening fence to include a language tag
(e.g., change ``` to ```text) so the block matches other examples and syntax
highlighting conventions.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/PublicSuffixListTest.php (2)
19-22:⚠️ Potential issue | 🟠 MajorUnit test is loading the real PSL cache (should be mocked or moved to integration).
Line 21 constructs a realPublicSuffixList, which loads the bundled PSL data and makes these tests integration‑level. Please either move these tests to the integration suite or inject a mocked/fixture ruleset for isolation.As per coding guidelines: Unit tests must use mocked
PublicSuffixListfor isolation; integration tests must use real PSL cache.
15-49:⚠️ Potential issue | 🟡 MinorReset static PSL state in
tearDown()to prevent cross‑test leakage.As per coding guidelines: Always reset static state in
tearDown()(e.g.,RegisteredDomain::setTestPslInstance(null)).🔧 Suggested fix
class PublicSuffixListTest extends TestCase { private PublicSuffixList $psl; protected function setUp(): void { $this->psl = new PublicSuffixList(); } + + protected function tearDown(): void + { + $ref = new \ReflectionProperty(PublicSuffixList::class, 'rules'); + $ref->setAccessible(true); + $ref->setValue(null); + }
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 176: The link reference "[Unreleased]" at the bottom of CHANGELOG.md is
unused; either remove the standalone reference line "[Unreleased]:
https://github.com/XOOPS/RegDom/compare/v2.0.2-beta2...HEAD" or add a
corresponding top-level section header "## [Unreleased]" (and content) that uses
that reference; update the file so there are no orphaned reference definitions
and the link reference is consumed by a matching section.
In `@tests/integration/PublicSuffixListIntegrationTest.php`:
- Around line 18-24: Add a tearDown() that clears the static PSL test instance
to ensure test isolation: after setUp() creates $this->psl (an instance of
PublicSuffixList) call RegisteredDomain::setTestPslInstance(null) in tearDown()
and unset $this->psl so any static state set for tests is reset between runs;
implement tearDown() alongside the existing setUp() to perform these resets.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/integration/PublicSuffixListIntegrationTest.php`:
- Around line 26-32: Add a top-level "use ReflectionProperty;" import and update
the tearDown() code in PublicSuffixListIntegrationTest to instantiate
ReflectionProperty via the imported class (replace new \ReflectionProperty(...)
with new ReflectionProperty(...)); this satisfies PSR-12 imports and removes the
PHPMD warning while keeping the existing logic that resets
PublicSuffixList::$rules in tearDown().
In `@tests/unit/PublicSuffixListTest.php`:
- Around line 24-30: Import the global ReflectionProperty class and update the
tearDown method to use the short class name instead of the fully-qualified name;
specifically add a use statement for ReflectionProperty and change the
ReflectionProperty instantiation in the tearDown that references
PublicSuffixList::class and 'rules' so it uses ReflectionProperty (not
\ReflectionProperty) to match PSR-12 import style while keeping the same
setAccessible and setValue calls.
|
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests