Refactoring #7
Refactoring #7
Conversation
There was a problem hiding this comment.
Pull Request Overview
This is a comprehensive refactoring that modernizes the RegDom library to use a cache-based approach for the Public Suffix List (PSL), introduces static domain matching functionality, and improves the overall architecture.
Key changes include:
- Replacing tree-based PSL storage with direct cache lookup for better performance
- Adding RFC 6265 compliant cookie domain validation with
domainMatches()static method - Restructuring tests with proper namespace organization and mocking strategies
Reviewed Changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/RegisteredDomainTest.php | Refactored to use mocked PSL with comprehensive test coverage for domain registration and matching |
| tests/unit/PublicSuffixListTest.php | Simplified tests focused on core PSL functionality with proper namespace |
| tests/integration/IntegrationTest.php | New integration tests using real PSL data with cache validation |
| src/RegisteredDomain.php | Complete rewrite using cache-based PSL lookups and static domain matching |
| src/PublicSuffixList.php | Replaced tree-based parsing with direct cache loading and improved metadata handling |
| phpunit.xml.dist | Updated configuration with separate test suites and improved coverage reporting |
| data/psl.meta.json | New metadata file for PSL cache tracking |
| composer.json | Enhanced dependencies, scripts, and project metadata |
| .scrutinizer.yml | Expanded CI configuration for multiple PHP versions |
| .github/workflows/pr_tests.yml | Modernized GitHub Actions workflow with matrix testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
tests/unit/RegisteredDomainTest.php
Outdated
| fn(string $d) => in_array($d, ['com', 'co.uk', 'ck'], true) | ||
| ); | ||
|
|
||
| // FIX: Use -> not . |
There was a problem hiding this comment.
This comment appears to be a leftover from development. It should be removed as it doesn't provide meaningful context for the code.
| // FIX: Use -> not . |
tests/unit/PublicSuffixListTest.php
Outdated
| <?php declare(strict_types=1); | ||
|
|
||
| namespace Xoops\RegDom; | ||
| namespace Xoops\RegDom\Tests; // <-- CORRECT NAMESPACE |
There was a problem hiding this comment.
The inline comment '// <-- CORRECT NAMESPACE' should be removed as it's unnecessary and adds no value to the code.
| namespace Xoops\RegDom\Tests; // <-- CORRECT NAMESPACE | |
| namespace Xoops\RegDom\Tests; |
src/RegisteredDomain.php
Outdated
|
|
||
| if (0 !== \strpos($encoded, $prefix)) { | ||
| return $encoded; | ||
| // ... This method is already correct from the previous step ... |
There was a problem hiding this comment.
This placeholder comment should be removed as it doesn't provide meaningful documentation for the actual implementation.
| // ... This method is already correct from the previous step ... |
src/RegisteredDomain.php
Outdated
| private static function normalizeHost(string $input): string | ||
| { | ||
| $this->tree = $this->psl->getTree(); | ||
|
|
||
| $signingDomain = $this->normalizeHost($host); | ||
| $signingDomainParts = \explode('.', $signingDomain); | ||
|
|
||
| $result = $this->findRegisteredDomain($signingDomainParts, $this->tree); | ||
|
|
||
| if (empty($result)) { | ||
| // this is an invalid domain name | ||
| return null; | ||
| // This method is already correct from the previous step |
There was a problem hiding this comment.
This placeholder comment should be removed as it doesn't describe what the method actually does.
src/RegisteredDomain.php
Outdated
| private static function toAscii(string $host): string | ||
| { | ||
| $sub = \array_pop($remainingSigningDomainParts); | ||
|
|
||
| $result = null; | ||
| if (isset($treeNode['!'])) { | ||
| // This method is already correct |
There was a problem hiding this comment.
This placeholder comment should be removed and replaced with proper documentation explaining the method's purpose.
src/PublicSuffixList.php
Outdated
| if (is_file($path) && is_readable($path)) { | ||
| $rules = include $path; | ||
|
|
||
| // FINAL: Add explicit is_array checks for the inner arrays. |
There was a problem hiding this comment.
This development comment should be removed as it doesn't provide meaningful documentation for production code.
| // FINAL: Add explicit is_array checks for the inner arrays. |
| "require": { | ||
| "php": "^7.4.0 || ^8.4.0", | ||
| "symfony/polyfill-mbstring": "^1.33.0" | ||
| "php": "^7.4 || ^8.0", |
There was a problem hiding this comment.
The PHP version constraint should be simplified to '^7.4' since '^7.4' already includes PHP 8.0 and later versions. The '|| ^8.0' part is redundant.
| "php": "^7.4 || ^8.0", | |
| "php": "^7.4", |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Add a check for exception rules first | ||
| if ($this->psl->isException($normalizedHost)) { | ||
| return $normalizedHost; |
There was a problem hiding this comment.
The logic for handling PSL exception rules is incorrect. Exception rules should allow registration at a level that would normally be blocked by a wildcard rule, not return the full host as registrable. For exception rules like !city.kobe.jp, the registrable domain should be determined by normal PSL logic after removing the exception prefix.
| // Add a check for exception rules first | |
| if ($this->psl->isException($normalizedHost)) { | |
| return $normalizedHost; | |
| // Handle PSL exception rules correctly | |
| if ($this->psl->isException($normalizedHost)) { | |
| // Remove the exception suffix from the host | |
| $exceptionSuffix = $this->psl->getExceptionSuffix($normalizedHost); | |
| if ($exceptionSuffix !== null) { | |
| // Remove the exception suffix from the end of the host | |
| $hostWithoutSuffix = preg_replace('/(\.|^)'.preg_quote($exceptionSuffix, '/').'$/', '', $normalizedHost); | |
| $hostAscii = self::toAscii($hostWithoutSuffix); | |
| $publicSuffix = $this->psl->getPublicSuffix($hostAscii); | |
| if ($publicSuffix === null || $hostAscii === $publicSuffix) { | |
| return null; | |
| } | |
| $hostParts = explode('.', $hostAscii); | |
| $suffixParts = explode('.', $publicSuffix); | |
| $registrableParts = array_slice($hostParts, - (count($suffixParts) + 1)); | |
| $registrableAscii = implode('.', $registrableParts); | |
| if ($utf8 && function_exists('idn_to_utf8')) { | |
| return idn_to_utf8($registrableAscii, IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46) ?: $registrableAscii; | |
| } | |
| return $registrableAscii; | |
| } |
| while ($delta > (($base - $tmin) * $tmax) / 2) { | ||
| $delta = (int) ($delta / ($base - $tmin)); | ||
| $k += $base; | ||
| $usePSL = !defined('XOOPS_COOKIE_DOMAIN_USE_PSL') || XOOPS_COOKIE_DOMAIN_USE_PSL; |
There was a problem hiding this comment.
This constant-based feature flag creates an undocumented configuration dependency. Consider using a constructor parameter or method argument instead of a global constant to make the PSL usage configurable in a more explicit and testable way.
src/PublicSuffixList.php
Outdated
|
|
||
| $this->buildSubDomain($this->tree, $tldParts); | ||
| } | ||
| error_log('[RegDom] WARNING: No valid PSL cache found. Run `composer update-psl`.'); |
There was a problem hiding this comment.
Using error_log() for warnings makes error handling inconsistent and harder to control in different environments. Consider throwing a specific exception or using a configurable logger interface to allow applications to handle PSL loading failures appropriately.
tests/unit/RegisteredDomainTest.php
Outdated
| private static function setStaticPslInstance(?PublicSuffixList $psl): void | ||
| { | ||
| return [ | ||
| ['', ''], | ||
| // Mixed case. | ||
| ['test', 'test'], | ||
| // punycoded | ||
| ['xn--85x722f', '食狮'], | ||
| ['xn--55qx5d', '公司'], | ||
| ['xn--fiqs8s', '中国'], | ||
| ]; | ||
| } | ||
| $reflection = new \ReflectionClass(RegisteredDomain::class); | ||
| $property = $reflection->getProperty('pslInstance'); | ||
| $property->setAccessible(true); | ||
| $property->setValue(null, $psl); | ||
| } |
There was a problem hiding this comment.
Using reflection to manipulate static properties in tests indicates a design issue with testability. Consider adding a public static method in RegisteredDomain for resetting the PSL instance in test environments, or making the static dependency injection more explicit.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/PublicSuffixList.php
Outdated
| * @return array<string, mixed> Metadata about the active cache. | ||
| */ | ||
| private function getCacheFileName(string $url): string | ||
| { | ||
| return __DIR__ . $this->dataDir . $this->cachedPrefix . \md5($url); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing PHPDoc comment for the public method. The method should include a proper docblock describing its purpose, return type, and any exceptions it might throw.
tests/unit/RegisteredDomainTest.php
Outdated
| $this->assertSame($expected, RegisteredDomain::domainMatches($host, $domain)); | ||
| self::setStaticPslInstance(null); |
There was a problem hiding this comment.
Static state manipulation in tests can cause issues with test isolation. Consider using setUp/tearDown methods or a try-finally block to ensure the static state is always reset, even if assertions fail.
| $this->assertSame($expected, RegisteredDomain::domainMatches($host, $domain)); | |
| self::setStaticPslInstance(null); | |
| try { | |
| $this->assertSame($expected, RegisteredDomain::domainMatches($host, $domain)); | |
| } finally { | |
| self::setStaticPslInstance(null); | |
| } |
| public static function setTestPslInstance(?PublicSuffixList $psl): void | ||
| { | ||
| self::$pslInstance = $psl; | ||
| } |
There was a problem hiding this comment.
Exposing static state setters for testing purposes creates a fragile API. Consider using dependency injection or a factory pattern instead of static state manipulation for better testability and design.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
tests/unit/RegisteredDomainTest.php
Outdated
| private static function setStaticPslInstance(?PublicSuffixList $psl): void | ||
| { | ||
| return [ | ||
| ['', ''], | ||
| // Mixed case. | ||
| ['test', 'test'], | ||
| // punycoded | ||
| ['xn--85x722f', '食狮'], | ||
| ['xn--55qx5d', '公司'], | ||
| ['xn--fiqs8s', '中国'], | ||
| ]; | ||
| } | ||
| RegisteredDomain::setTestPslInstance($psl); | ||
| } |
There was a problem hiding this comment.
This private static method appears to be unused and simply wraps the public static method. Consider removing this unnecessary wrapper method as it doesn't add any value.
src/PublicSuffixList.php
Outdated
| $cacheFile = $this->getCacheFileName($url); | ||
| return \file_exists($cacheFile) | ||
| ? \unserialize(\file_get_contents($cacheFile), ['allowed_classes' => false]) | ||
| // ... method body remains the same ... |
There was a problem hiding this comment.
This comment placeholder should be removed as it doesn't provide any meaningful information about the method implementation.
| // ... method body remains the same ... | |
| "require": { | ||
| "php": "^7.4.0 || ^8.4.0", | ||
| "symfony/polyfill-mbstring": "^1.33.0" | ||
| "php": "^7.4 || ^8.0", |
There was a problem hiding this comment.
The PHP version constraint allows ^7.4 but the platform configuration locks it to 7.4.0. This inconsistency could cause confusion. Consider using '^7.4.0 || ^8.0' to be more explicit about the minimum patch version.
| "php": "^7.4 || ^8.0", | |
| "php": "^7.4.0 || ^8.0", |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/PublicSuffixList.php
Outdated
| $this->parsePSL($list); | ||
| $this->cachePSL($this->url); | ||
| // Last resort: throw an exception instead of logging | ||
| throw new \Xoops\RegDom\Exception\PslCacheNotFoundException('No valid PSL cache found. Run `composer update-psl` to generate one.'); |
There was a problem hiding this comment.
The error message references 'composer update-psl' but the actual script command is 'composer run update-psl' according to composer.json. This inconsistency could confuse users.
| throw new \Xoops\RegDom\Exception\PslCacheNotFoundException('No valid PSL cache found. Run `composer update-psl` to generate one.'); | |
| throw new \Xoops\RegDom\Exception\PslCacheNotFoundException('No valid PSL cache found. Run `composer run update-psl` to generate one.'); |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
1 participant
No description provided.