Conversation
…ization - Replace pr_tests.yml with unified ci.yml (matrix PHP 7.4–8.4, experimental 8.5) - Add Codecov coverage upload on PHP 8.3 row - Rewrite .scrutinizer.yml: run composer ci on all PHP versions, remove external_code_coverage - Modernize .gitignore (PHPUnit cache, PSL data, local config overrides) - Add .editorconfig, .github/dependabot.yml, copilot-instructions.md - Update composer.json: add scripts, funding, support, config sections - Improve phpstan.neon and phpunit.xml.dist configuration - Add PslCacheNotFoundException test, update existing tests - Remove legacy .travis.yml, .phpcs.xml.dist - Code improvements in PublicSuffixList and RegisteredDomain Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CLAUDE.md and docs/tasks.md are local development aids that should not be committed to the repository. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add qodana.yaml: PHP 7.4, qodana.recommended profile, exclude vendor/data/build - Add .github/workflows/qodana.yml: runs on push to master and PRs - ci.yml: treat PHP 8.5 as stable (remove experimental flag) - .scrutinizer.yml: analysis-only (remove per-PHP build nodes, covered by GitHub Actions) - README.md: add XOOPS logo banner - Add qodana.yaml to .gitattributes export-ignore Requires QODANA_TOKEN secret in GitHub repository settings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- renovate.json: auto-merge patches (3-day wait), group minor+patch, dashboard approval for majors, manage GitHub Actions versions - .coderabbit.yaml: assertive profile, PHP 7.4 compatibility checks, PHPCS/PHPStan/gitleaks enabled, path-specific review instructions - .gitattributes: add renovate.json and .coderabbit.yaml to export-ignore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR modernizes RegDom’s tooling/CI and improves PSL handling, cache metadata, and domain normalization, along with updated bundled PSL data and expanded test coverage.
Changes:
- Enhance PSL cache loading/validation and domain normalization (ports, trailing dots, bracketed IPv6), plus new
PslCacheNotFoundException. - Refresh bundled Public Suffix List cache/meta and add/update unit/integration tests accordingly.
- Overhaul developer tooling and CI configuration (GitHub Actions CI, Qodana config, Renovate/Dependabot, PHPStan/PHPUnit config, docs).
Reviewed changes
Copilot reviewed 26 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/PublicSuffixList.php |
Adds cache validation (size/type checks), static normalization, richer metadata (incl. needs_update), and throws a dedicated exception when cache is missing/invalid. |
src/RegisteredDomain.php |
Improves host normalization (URL parsing detection via str_contains, strips ports/trailing dots, bracketed IPv6) and adds XOOPS header. |
src/Exception/PslCacheNotFoundException.php |
Introduces a dedicated exception type for missing/invalid PSL cache. |
tests/unit/PublicSuffixListTest.php |
Adds tests for empty/IP inputs, exception checks, metadata keys, and normalization behavior. |
tests/unit/RegisteredDomainTest.php |
Extends registered-domain provider with trailing-dot/port/IPv6 cases and uses str_ends_with in suffix matching. |
tests/unit/PslCacheNotFoundExceptionTest.php |
Adds unit coverage for the new exception class. |
tests/integration/IntegrationTest.php |
(Existing) Ensures real PSL cache behaviors; impacted by cache/metadata changes. |
data/psl.cache.php |
Updates the bundled PSL cache contents (new/removed rules). |
data/psl.meta.json |
Updates PSL metadata timestamps/headers. |
bin/update-psl.php |
Adds strict types, autoload probing, PHP 8 string helpers, opcache invalidation, and simplifies temp cleanup. |
bin/reloadpsl |
Simplifies to a wrapper around bin/update-psl.php. |
composer.json |
Updates polyfills/dev tools, adjusts autoload-dev mappings, and changes lint/fix/analyse scripts. |
phpunit.xml.dist |
Restructures PHPUnit config (switches to <source> and removes Clover output config). |
phpstan.neon |
Narrows analysis scope to src, tweaks settings. |
.github/workflows/ci.yml |
Replaces prior CI workflow with a matrix across PHP versions + a coverage job (Codecov upload). |
.github/workflows/qodana.yml |
Adds Qodana scanning workflow. |
.github/workflows/pr_tests.yml |
Removed (superseded by new CI workflow). |
qodana.yaml |
Adds Qodana configuration for PHP. |
renovate.json |
Adds Renovate configuration. |
.github/dependabot.yml |
Adds Dependabot configuration. |
README.md |
Rewrites/expands docs (usage, RFC 6265 cookie validation, update workflows, dev commands). |
.gitignore |
Adds ignores for build artifacts, PHPUnit cache, downloaded PSL source, local configs, etc. |
.gitattributes |
Adds EOL normalization rules and adjusts export-ignore list. |
.editorconfig |
Adds editor settings for consistent formatting. |
.coderabbit.yaml |
Adds CodeRabbit review configuration emphasizing PHP 7.4 compatibility. |
.github/copilot-instructions.md |
Adds repo-specific Copilot guidance (layout, compatibility, conventions). |
.github/xoops-copilot-template.md |
Adds a reusable XOOPS Copilot instructions template. |
.scrutinizer.yml |
Simplifies Scrutinizer config and expands excluded paths. |
.travis.yml |
Removed legacy Travis configuration. |
.phpcs.xml.dist |
Removed the tracked PHPCS ruleset file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "test": "phpunit --colors=always", | ||
| "phpunit": "phpunit --colors=always", | ||
| "lint": "phpcs", | ||
| "analyse": "phpstan analyse --level=max src", | ||
| "fix": "phpcbf --standard=PSR12 src", | ||
| "analyse": "phpstan analyse", | ||
| "fix": "phpcbf", |
There was a problem hiding this comment.
composer ci runs phpcs/phpcbf, but this PR removes the tracked PHPCS ruleset file and the scripts no longer specify a standard or paths. With no phpcs.xml(.dist)/.phpcs.xml(.dist) in the repo, PHPCS will fall back to its defaults (and may scan unintended directories), which will make CI unreliable. Add a tracked PHPCS ruleset (e.g., restore .phpcs.xml.dist) or update the scripts to pass --standard=PSR12 and explicit target paths (e.g., src, tests).
| <source> | ||
| <include> | ||
| <directory suffix=".php">./src</directory> | ||
| </include> | ||
| <report> | ||
| <clover outputFile="build/logs/clover.xml"/> | ||
| </report> | ||
| </coverage> | ||
|
|
||
| </source> |
There was a problem hiding this comment.
The PHPUnit config removes the Clover coverage report generation, but the CI workflow still uploads build/logs/clover.xml on coverage runs. As-is, the coverage file won’t be produced, so the upload step becomes ineffective. Either re-enable Clover report output in the config, or generate it via CLI in the coverage job (e.g., --coverage-clover build/logs/clover.xml).
| <phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd" | ||
| bootstrap="vendor/autoload.php" | ||
| colors="true" | ||
| failOnRisky="true" | ||
| failOnWarning="true"> |
There was a problem hiding this comment.
phpunit.xml.dist now uses the <source> element, but composer.json still allows PHPUnit ^9.6. PHPUnit 9.x configurations typically use <coverage>/<filter> instead, and schema validation can fail when the config doesn’t match the installed major version. Either pin PHPUnit to a version that supports this config, or keep/split configs so PHPUnit 9 remains supported.
- Add sonar-project.properties: org=xoops, projectKey=XOOPS_RegDom, PHP 7.4, coverage from clover.xml - Add .github/workflows/sonarcloud.yml: runs tests with coverage then SonarCloud scan on push to master and PRs - ci.yml: add separate coverage report generation step - qodana.yml: fix checkout ref to fall back to github.sha on push - Fix typo in test method name (Hands → Handles) - Add sonar-project.properties to .gitattributes export-ignore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
CI Workflows & Quality Scans .github/workflows/ci.yml, .github/workflows/sonarcloud.yml, .github/workflows/qodana.yml, .github/workflows/pr_tests.yml (removed) |
Adds matrix CI, SonarCloud and Qodana workflows; removes legacy pr_tests workflow. |
Dependency Automation .github/dependabot.yml, renovate.json |
Adds Dependabot for Composer/Actions and Renovate rules for grouping/automerge. |
Code review / Bot configs .coderabbit.yaml, qodana.yaml, sonar-project.properties |
Introduces CodeRabbit review profile and Qodana/Sonar config files (scan/bootstrap/excludes). |
Editor & Repo Settings .editorconfig, .gitattributes, .gitignore |
Adds editor formatting rules, enforces LF for many file types, expands export-ignore and ignore entries. |
Static Analysis / Linting phpstan.neon, .phpcs.xml.dist (removed), .scrutinizer.yml |
Narrows PHPStan scope to src, removes legacy PHPCS config, consolidates Scrutinizer excluded paths. |
CI Tooling & Templates .github/copilot-instructions.md, .github/xoops-copilot-template.md |
Adds contributor guidance, project layout, coding conventions, build/test instructions, and PR checklist. |
Composer / Packaging composer.json |
Updates polyfill deps, adds test autoload mappings, adjusts scripts, and suggests ext-intl for IDN support. |
PSL Data & Metadata data/psl.cache.php, data/psl.meta.json |
Refreshes PSL cache timestamp/etag and expands/updates NORMAL/WILDCARD/EXCEPTION lists and metadata. |
Core Code Changes src/PublicSuffixList.php, src/RegisteredDomain.php, src/Exception/PslCacheNotFoundException.php |
Makes normalizeDomain static; hardens cache loading (size/readability checks, throws PslCacheNotFoundException); replaces legacy string checks with str_contains/str_ends_with; adds throws annotations and header/license add. |
Scripts bin/reloadpsl, bin/update-psl.php |
Adds declare(strict_types=1), delegates reload to update-psl.php, uses str_starts_with for parsing, invalidates opcache after atomic cache writes, simplifies write-failure handling, and loads Composer autoload. |
Tests & PHPUnit config phpunit.xml.dist, tests/unit/* |
Simplifies PHPUnit coverage config and adds unit tests for PslCacheNotFoundException, PublicSuffixList edge cases, and RegisteredDomain scenarios. |
Documentation / README README.md |
Comprehensive README rewrite: purpose, requirements, installation, usage examples, configuration options, and development commands. |
Tooling / CI Config Files qodana.yaml, qodana.yaml, sonar-project.properties, phpstan.neon |
Adds Qodana config, Sonar project properties, and tightens PHPStan configuration. |
Removed Files .travis.yml, .phpcs.xml.dist |
Removes legacy Travis CI config and PHPCS ruleset file. |
Estimated Code Review Effort
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 33.33% 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 'Feature/enhancements' is too vague and generic, using non-descriptive terms that fail to convey meaningful information about the changeset. It does not summarize the main changes. | Replace with a specific, concise title that describes the primary change, such as 'Add CI/CD workflows and configuration files for enhanced development tooling' or 'Configure PHP CodeRabbit, GitHub Actions, and quality analysis tools'. |
✅ 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 unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In @.coderabbit.yaml:
- Around line 34-47: The tools block is currently a top-level key; per the
CodeRabbit schema it must be nested under the reviews key. Move the entire
"tools:" mapping (including phpcs, phpstan, gitleaks, markdownlint, yamllint,
github-checks and its timeout_ms) one indentation level deeper so it becomes a
child of "reviews:", ensuring YAML indentation is correct and no duplicate
"reviews" entries are created.
In @.github/copilot-instructions.md:
- Around line 9-26: Replace the opening code fence for the directory tree block
with a fenced code block tagged for language "text" (i.e. change ``` to ```text)
for the block starting with "src/ # Library source
code" in .github/copilot-instructions.md so the tree is fenced as ```text ...
```, which will satisfy the MD040 lint rule and improve readability.
In @.github/workflows/ci.yml:
- Line 7: Update the CI workflow to pin the runner image by replacing the
runs-on value "ubuntu-latest" with "ubuntu-22.04" in the GitHub Actions workflow
(the runs-on: ubuntu-latest entry in .github/workflows/ci.yml) so the job uses
the stable Ubuntu 22.04 runner instead of the floating latest image.
In @.github/workflows/qodana.yml:
- Around line 22-25: The Qodana job should skip when the token is not available;
set the job-level environment variable QODANA_TOKEN to use the secret (env:
QODANA_TOKEN: ${{ secrets.QODANA_TOKEN }}) and add an if guard on the job (if:
env.QODANA_TOKEN != '') so the "Qodana Scan" job is skipped for forked PRs where
the secret is missing; keep the existing uses: JetBrains/qodana-action@v2025.2
and just add the env binding plus the if condition referencing env.QODANA_TOKEN.
In @.github/workflows/sonarcloud.yml:
- Around line 34-35: The workflow step "Run tests with coverage" is calling the
global phpunit binary; update that step to use the Composer-installed binary to
ensure CI uses the project version by changing the command from `phpunit ...` to
the vendored binary path (e.g., ./vendor/bin/phpunit --colors=always
--coverage-clover build/logs/clover.xml) so the GitHub Actions job uses the
Composer-provided PHPUnit binary.
In @.github/xoops-copilot-template.md:
- Around line 18-57: Update the markdown fenced code blocks to include language
tags and surrounding blank lines to satisfy markdownlint (MD040/MD031): add a
language tag (e.g., ```text) to the directory tree fence and ensure there is a
blank line before and after each code fence (including the PHP header block
fence ```php), so the directory listing and the XOOPS copyright header block are
both fenced with an explicit language and have blank lines separating them from
surrounding text.
In `@bin/update-psl.php`:
- Around line 16-22: After the autoloader discovery loop, add a PHP 7.4
compatibility fallback for str_starts_with so the script won’t fatal when the
Composer autoloader (and symfony/polyfill-php80) is missing; check
function_exists('str_starts_with') and, if absent, define a polyfill
implementation (using strpos === 0 semantics) so later calls to
str_starts_with() (used near lines calling str_starts_with) are safe even if the
autoloader wasn't loaded.
In `@qodana.yaml`:
- Around line 3-9: Replace the non-deterministic Docker image tag in the linter
setting to a fixed Qodana PHP version: change the value of the linter field
(currently "jetbrains/qodana-php:latest") to a pinned tag such as
"jetbrains/qodana-php:2025.3" (or 2025.2) to ensure reproducible analysis while
leaving the php.version entry ("php: version: \"7.4\"") unchanged.
In `@README.md`:
- Around line 5-7: Update the README description to hyphenate the compound
adjective: change the phrase "Object oriented PHP library" to "Object-oriented
PHP library" (look for the initial header/summary line containing "Object
oriented PHP library for querying Mozilla's Public Suffix List...") so static
analysis and grammar rules correctly treat the compound adjective.
- Around line 1-2: Add a top-level heading above the repository logo to satisfy
markdownlint: move or insert an H1 line (e.g., "# Project Title" or descriptive
project name) before the existing image markdown reference "" so the README
starts with a top-level heading followed by the logo/badges.
- Around line 45-55: Add a short note to the README explaining the dual
static/instance API: state that RegisteredDomain::domainMatches(...) is a
convenience static method intended for RFC 6265 cookie-domain checks and will
internally instantiate RegisteredDomain when PSL checks are needed, while
methods like getRegisteredDomain() require creating a RegisteredDomain instance
(new RegisteredDomain(...)) and are meant for domain extraction/manipulation;
include guidance on when to prefer the static utility (quick cookie checks)
versus the instance API (reusable parsed domain, repeated operations or access
to other instance methods).
In `@sonar-project.properties`:
- Around line 5-13: The sonar configuration contradicts itself by defining
sonar.tests=tests but then excluding that same path via
sonar.test.exclusions=tests/**; remove the sonar.test.exclusions entry so that
sonar.tests controls the test scope (keep sonar.sources, sonar.tests,
sonar.php.* and sonar.exclusions as-is), ensuring tests are correctly recognized
and the Clover report import remains unchanged.
In `@src/PublicSuffixList.php`:
- Around line 63-66: The code silently skips cache files when $fileSize is false
or outside hardcoded thresholds; update the PublicSuffixList logic to report
diagnostics and/or make thresholds configurable: instead of just "continue" in
the block checking $fileSize and $path, emit a clear warning (use trigger_error
or the class logger if available) that includes $path, the actual $fileSize (or
failure), and the min/max thresholds, and optionally replace the magic numbers
with class constants or constructor parameters (e.g.,
$minCacheSize/$maxCacheSize) so callers can adjust limits; keep the existing
continue behavior after logging unless a config flag instructs stricter
handling.
In `@src/RegisteredDomain.php`:
- Line 116: The use of PHP 8's str_contains breaks PHP 7.4 support; update the
expression that sets $host in RegisteredDomain (the ternary using
str_contains($input, '/')) to use strpos($input, '/') !== false instead so it
remains PHP 7.4-compatible when computing $host from $input and calling
parse_url.
In `@tests/unit/RegisteredDomainTest.php`:
- Line 61: Replace the PHP 8-only call to str_ends_with($h, $suffix) with a PHP
7.4-compatible check: ensure empty-suffix behavior is preserved and avoid
negative-length issues by using ($suffix === '' || (strlen($suffix) <=
strlen($h) && substr($h, -strlen($suffix)) === $suffix)). Update the conditional
where str_ends_with($h, $suffix) appears (in the RegisteredDomainTest logic
using variables $h and $suffix) to use this expression.
| tools: | ||
| phpcs: | ||
| enabled: true | ||
| phpstan: | ||
| enabled: true | ||
| gitleaks: | ||
| enabled: true | ||
| markdownlint: | ||
| enabled: true | ||
| yamllint: | ||
| enabled: true | ||
| github-checks: | ||
| enabled: true | ||
| timeout_ms: 90000 |
There was a problem hiding this comment.
Move tools configuration under reviews section.
Per the CodeRabbit schema, tools is a property of reviews, not a root-level key. The current placement may cause the tool configuration to be ignored.
🔧 Proposed fix
chat:
auto_reply: true
-tools:
- phpcs:
- enabled: true
- phpstan:
- enabled: true
- gitleaks:
- enabled: true
- markdownlint:
- enabled: true
- yamllint:
- enabled: true
- github-checks:
- enabled: true
- timeout_ms: 90000
+reviews:
+ profile: "assertive"
+ request_changes_workflow: true
+ high_level_summary: true
+ poem: false
+ review_status: true
+ collapse_walkthrough: false
+ auto_review:
+ enabled: true
+ drafts: false
+ path_instructions:
+ - path: "src/**/*.php"
+ instructions: >-
+ Review for PHP 7.4 compatibility (no union types, named arguments,
+ match expressions, enums, readonly, constructor promotion, fibers,
+ intersection types, never return type, or first-class callable syntax).
+ Check for proper use of declare(strict_types=1) and PSR-12 code style.
+ - path: "tests/**/*.php"
+ instructions: >-
+ Review test code for proper assertions, test isolation, and edge
+ case coverage. Tests must work across PHPUnit 9.6, 10, and 11.
+ tools:
+ phpcs:
+ enabled: true
+ phpstan:
+ enabled: true
+ gitleaks:
+ enabled: true
+ markdownlint:
+ enabled: true
+ yamllint:
+ enabled: true
+ github-checks:
+ enabled: true
+ timeout_ms: 90000
+
+chat:
+ auto_reply: trueOr more simply, just move the tools: block to be indented under reviews:.
🤖 Prompt for AI Agents
In @.coderabbit.yaml around lines 34 - 47, The tools block is currently a
top-level key; per the CodeRabbit schema it must be nested under the reviews
key. Move the entire "tools:" mapping (including phpcs, phpstan, gitleaks,
markdownlint, yamllint, github-checks and its timeout_ms) one indentation level
deeper so it becomes a child of "reviews:", ensuring YAML indentation is correct
and no duplicate "reviews" entries are created.
| ``` | ||
| src/ # Library source code | ||
| RegisteredDomain.php # Main class: extracts registrable domains, validates cookie domains | ||
| PublicSuffixList.php # PSL cache loader and query engine (NORMAL/WILDCARD/EXCEPTION rules) | ||
| Exception/ | ||
| PslCacheNotFoundException.php # Thrown when no valid PSL cache is found | ||
| bin/ | ||
| update-psl.php # Downloads and caches the Mozilla PSL (atomic writes, HTTP conditional) | ||
| reloadpsl # Shell wrapper to force PSL reload | ||
| data/ | ||
| psl.cache.php # Bundled PSL cache (PHP array with NORMAL/WILDCARD/EXCEPTION keys) | ||
| psl.meta.json # HTTP metadata (ETag, Last-Modified) for conditional downloads | ||
| public_suffix_list.dat # Raw PSL source (reference copy) | ||
| tests/ | ||
| unit/ # Isolated unit tests (mocked PSL) | ||
| integration/ # Integration tests (real PSL cache, requires intl extension) | ||
| docs/ # Documentation and improvement roadmaps | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the directory tree fence.
This avoids the MD040 lint warning and improves readability.
Suggested fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| src/ # Library source code | |
| RegisteredDomain.php # Main class: extracts registrable domains, validates cookie domains | |
| PublicSuffixList.php # PSL cache loader and query engine (NORMAL/WILDCARD/EXCEPTION rules) | |
| Exception/ | |
| PslCacheNotFoundException.php # Thrown when no valid PSL cache is found | |
| bin/ | |
| update-psl.php # Downloads and caches the Mozilla PSL (atomic writes, HTTP conditional) | |
| reloadpsl # Shell wrapper to force PSL reload | |
| data/ | |
| psl.cache.php # Bundled PSL cache (PHP array with NORMAL/WILDCARD/EXCEPTION keys) | |
| psl.meta.json # HTTP metadata (ETag, Last-Modified) for conditional downloads | |
| public_suffix_list.dat # Raw PSL source (reference copy) | |
| tests/ | |
| unit/ # Isolated unit tests (mocked PSL) | |
| integration/ # Integration tests (real PSL cache, requires intl extension) | |
| docs/ # Documentation and improvement roadmaps | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @.github/copilot-instructions.md around lines 9 - 26, Replace the opening
code fence for the directory tree block with a fenced code block tagged for
language "text" (i.e. change ``` to ```text) for the block starting with "src/
# Library source code" in .github/copilot-instructions.md so the tree is fenced
as ```text ... ```, which will satisfy the MD040 lint rule and improve
readability.
| jobs: | ||
| tests: | ||
| name: PHP ${{ matrix.php }}${{ matrix.deps && format(' / {0}', matrix.deps) || '' }}${{ matrix.coverage && ' / Coverage' || '' }} | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
PHP 7.4 support GitHub Actions ubuntu-latest 2024 2025
💡 Result:
ubuntu-latestswitched to Ubuntu 24.04 with a rollout that started Dec 5, 2024 and completed Jan 17, 2025. As of that migration,ubuntu-latest==ubuntu-24.04. [1], [2]- PHP 7.4 is not available “out of the box” on
ubuntu-latest(24.04). The Ubuntu 24.04 runner image lists PHP 8.3.* (not 7.4). [1] - If you still need PHP 7.4 in GitHub Actions in 2024/2025+, install it during the workflow using
shivammathur/setup-php@v2, which supports setting up PHP 7.4 on GitHub-hosted runners. [3]
Sources:
[1] actions/runner-images announcement issue “Ubuntu-latest workflows will use Ubuntu-24.04 image” (#10636)
[2] actions/runner-images README (available images / label mapping)
[3] GitHub Marketplace: shivammathur/setup-php action (PHP version support incl. 7.4)
🌐 Web query:
shivammathur/setup-php PHP 7.4 ubuntu-24.04 compatibility
💡 Result:
Yes—shivammathur/setup-php can set up PHP 7.4 on GitHub-hosted ubuntu-24.04 runners (the action lists PHP 5.3–8.6 as supported on GitHub-hosted runners, which includes 7.4) [1]. The project wiki also documents PHP module sets on Ubuntu 24.04 across many PHP versions, indicating the action’s Ubuntu 24.04 support isn’t limited to only the OS default PHP [2].
However, installing PHP 7.4 directly on a plain Ubuntu 24.04 system via APT (e.g., Ondřej Surý packages/PPA) can fail due to dependency conflicts, so “works on Ubuntu 24.04” depends on using the action vs. using distro packages [3]. Practically: if you must run 7.4, prefer setup-php (or a pinned container image); otherwise consider moving CI to PHP ≥ 8.1 or an older runner like ubuntu-22.04.
Sources: [1] [2] [3]
🌐 Web query:
GitHub Actions ubuntu-22.04 ubuntu-latest maintenance support lifecycle
💡 Result:
- What
ubuntu-latestmeans (today):ubuntu-latestcurrently points to Ubuntu 24.04 on GitHub-hosted runners. (github.com) - Is
ubuntu-22.04still supported? Yes—ubuntu-22.04is still a standard GitHub-hosted runner label and is one of the currently supported Ubuntu GA images. (docs.github.com) - GitHub’s lifecycle policy (OS images):
- GitHub supports at most 2 GA images + 1 beta at a time.
- When a new OS image becomes GA, GitHub starts deprecating the oldest image label (with advance notice and “brownout” periods), and then retires it. (github.com)
- No published end-of-support date for
ubuntu-22.04(found here): I don’t see an announced deprecation/retirement date forubuntu-22.04in the official runner-images repo content retrieved above; GitHub typically announces it when the next Ubuntu GA image causes it to become the “oldest supported” image. (github.com) - Tooling maintenance inside images: Tool/versions are “typically removed 6 months after they’re deprecated or reach EOL,” so even while
ubuntu-22.04remains supported, specific preinstalled tool versions can disappear on that cadence. (github.com)
Practical guidance: pin the OS explicitly (ubuntu-22.04 or ubuntu-24.04) if you need stability; use ubuntu-latest if you want to follow the newest GA image as GitHub moves the label. (github.com)
Citations:
- 1: https://github.com/actions/runner-images?utm_source=openai
- 2: https://docs.github.com/en/actions/reference/github-hosted-runners-reference?utm_source=openai
- 3: https://github.com/actions/runner-images?utm_source=openai
- 4: https://github.com/actions/runner-images?utm_source=openai
- 5: https://github.com/actions/runner-images?utm_source=openai
- 6: https://github.com/actions/runner-images?utm_source=openai
🏁 Script executed:
cd .github/workflows && cat -n ci.ymlRepository: XOOPS/RegDom
Length of output: 3245
Consider pinning the runner image for consistent CI performance.
With ubuntu-latest now running Ubuntu 24.04, PHP 7.4 is not preinstalled but shivammathur/setup-php handles it automatically. Pinning to ubuntu-22.04 would avoid compiling PHP from source during CI, providing minor performance gains and longer tool availability stability.
Optional suggestion
- runs-on: ubuntu-latest
+ runs-on: ubuntu-22.04📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| runs-on: ubuntu-latest | |
| runs-on: ubuntu-22.04 |
🤖 Prompt for AI Agents
In @.github/workflows/ci.yml at line 7, Update the CI workflow to pin the runner
image by replacing the runs-on value "ubuntu-latest" with "ubuntu-22.04" in the
GitHub Actions workflow (the runs-on: ubuntu-latest entry in
.github/workflows/ci.yml) so the job uses the stable Ubuntu 22.04 runner instead
of the floating latest image.
| - name: Qodana Scan | ||
| uses: JetBrains/qodana-action@v2025.2 | ||
| env: | ||
| QODANA_TOKEN: ${{ secrets.QODANA_TOKEN }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Actions secrets availability forked pull_request workflows
💡 Result:
pull_requestworkflows triggered by PRs from forks: Repository/environment/org secrets are not provided to the runner. The only exception isGITHUB_TOKEN, which is read-only for fork PRs. [1]- Public repos: Fork PR workflows may also require maintainer approval to run (commonly for first-time contributors), but approval does not change the “no secrets for forks” rule. [1][4]
- Private/internal repos: Admins can optionally enable policies to send write tokens and even send secrets to fork PR workflows, and/or require approval before those fork-PR workflow runs execute. [2][3]
pull_request_target: Runs in the base repo context (base branch workflow), and can access secrets (and typically has elevated permissions) even when triggered by a fork PR—so it must be used with extreme caution and you generally should not check out / run untrusted PR code in that workflow. [1][5]- Dependabot PRs: Treated like fork PRs—
GITHUB_TOKENis read-only and Actions secrets are not available (use Dependabot secrets if needed). [1][6]
Sources: [1] GitHub Docs – Events that trigger workflows; [2] GitHub Docs – Enterprise policies for Actions (fork PR workflows); [3] GitHub Docs – Managing Actions settings for a repo (fork policies); [4] GitHub Docs – Approving workflow runs from forks; [5] GitHub Well-Architected – Avoid pull_request_target; [6] GitHub Changelog – Dependabot PR workflow restrictions.
🌐 Web query:
GitHub Actions if condition check secret token not empty syntax
💡 Result:
GitHub Actions secrets can’t be referenced directly in an if:. Put the secret into an env var, then test that env var for non-empty. Also, unset/unavailable secrets evaluate to an empty string. [1]
Step-level condition (most common)
steps:
- name: Do something only if token is set
env:
MY_TOKEN: ${{ secrets.MY_TOKEN }}
if: ${{ env.MY_TOKEN != '' }}
run: echo "Token is present"[1]
Job-level env + step conditions
jobs:
build:
runs-on: ubuntu-latest
env:
MY_TOKEN: ${{ secrets.MY_TOKEN }}
steps:
- if: ${{ env.MY_TOKEN != '' }}
run: echo "Token is present"[1]
Note: for pull_request workflows from forks, secrets (except GITHUB_TOKEN) are not provided, so your env var will be '' and the condition will be false. [1][2]
Sources: GitHub Docs on using secrets and conditionals [1]; GitHub Actions toolkit issue describing secrets becoming empty when unavailable (e.g., forks) [2].
Skip Qodana when QODANA_TOKEN isn't available (fork PRs).
Secrets are not passed to forked pull_request workflows, so the job will fail for external contributors. Add a guard to skip when the token is missing—secrets cannot be referenced directly in if: conditions, so use an env variable as the intermediary.
🧩 Suggested change
jobs:
qodana:
+ if: ${{ env.QODANA_TOKEN != '' }}
runs-on: ubuntu-latest
env:
QODANA_TOKEN: ${{ secrets.QODANA_TOKEN }}🤖 Prompt for AI Agents
In @.github/workflows/qodana.yml around lines 22 - 25, The Qodana job should
skip when the token is not available; set the job-level environment variable
QODANA_TOKEN to use the secret (env: QODANA_TOKEN: ${{ secrets.QODANA_TOKEN }})
and add an if guard on the job (if: env.QODANA_TOKEN != '') so the "Qodana Scan"
job is skipped for forked PRs where the secret is missing; keep the existing
uses: JetBrains/qodana-action@v2025.2 and just add the env binding plus the if
condition referencing env.QODANA_TOKEN.
| ### Cookie Domain Validation (RFC 6265) | ||
|
|
||
| `echo null === $regdom->getRegisteredDomain('co.uk');` | ||
| Outputs: | ||
| > 1 | ||
| ```php | ||
| use Xoops\RegDom\RegisteredDomain; | ||
|
|
||
| ## script bin\reloadpsl | ||
| This script can be used to load a fresh copy of the PSL. It may be useful | ||
| in cron job, or other scripted updates. | ||
| // Validates if a cookie domain is appropriate for a given host | ||
| RegisteredDomain::domainMatches('www.example.com', 'example.com'); // true | ||
| RegisteredDomain::domainMatches('example.com', 'com'); // false (public suffix) | ||
| RegisteredDomain::domainMatches('google.com', 'facebook.com'); // false (cross-domain) | ||
| RegisteredDomain::domainMatches('192.168.1.1', '192.168.1.1'); // false (IP addresses) | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Find PHP files containing the RegisteredDomain class
find . -type f -name "*.php" | xargs grep -l "class RegisteredDomain"Repository: XOOPS/RegDom
Length of output: 122
🏁 Script executed:
# Look for domainMatches method definition and its signature
rg -n "function domainMatches|public.*domainMatches" --type=php -B2 -A8Repository: XOOPS/RegDom
Length of output: 1701
🏁 Script executed:
# Check the entire RegisteredDomain class structure
rg -n "class RegisteredDomain" --type=php -A30Repository: XOOPS/RegDom
Length of output: 4119
🏁 Script executed:
# Get the full RegisteredDomain class with docblocks
head -n 120 ./src/RegisteredDomain.phpRepository: XOOPS/RegDom
Length of output: 4425
🏁 Script executed:
# Check if README.md has any introduction explaining the dual API
head -n 60 ./README.md | tail -n 30Repository: XOOPS/RegDom
Length of output: 1047
Consider adding documentation explaining the dual static/instance API design.
The RegisteredDomain class intentionally provides both patterns: domainMatches() as a static utility method for RFC 6265 cookie domain validation, and instance methods like getRegisteredDomain() for domain extraction. While the README examples naturally separate these approaches, adding explicit guidance on when to use each would clarify the API contract—particularly noting that domainMatches() internally instantiates the class when PSL validation is required, whereas getRegisteredDomain() expects explicit instantiation.
🤖 Prompt for AI Agents
In `@README.md` around lines 45 - 55, Add a short note to the README explaining
the dual static/instance API: state that RegisteredDomain::domainMatches(...) is
a convenience static method intended for RFC 6265 cookie-domain checks and will
internally instantiate RegisteredDomain when PSL checks are needed, while
methods like getRegisteredDomain() require creating a RegisteredDomain instance
(new RegisteredDomain(...)) and are meant for domain extraction/manipulation;
include guidance on when to prefer the static utility (quick cookie checks)
versus the instance API (reusable parsed domain, repeated operations or access
to other instance methods).
| $fileSize = filesize($path); | ||
| if ($fileSize === false || $fileSize < 100000 || $fileSize > 10000000) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Avoid silently skipping caches on size thresholds.
A valid (custom or future-larger) cache can be ignored without diagnostics, leading to unexpected fallback or exceptions.
Suggested fix
$fileSize = filesize($path);
if ($fileSize === false || $fileSize < 100000 || $fileSize > 10000000) {
+ trigger_error('PSL cache file size out of expected range: ' . basename($path), E_USER_WARNING);
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $fileSize = filesize($path); | |
| if ($fileSize === false || $fileSize < 100000 || $fileSize > 10000000) { | |
| continue; | |
| } | |
| $fileSize = filesize($path); | |
| if ($fileSize === false || $fileSize < 100000 || $fileSize > 10000000) { | |
| trigger_error('PSL cache file size out of expected range: ' . basename($path), E_USER_WARNING); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In `@src/PublicSuffixList.php` around lines 63 - 66, The code silently skips cache
files when $fileSize is false or outside hardcoded thresholds; update the
PublicSuffixList logic to report diagnostics and/or make thresholds
configurable: instead of just "continue" in the block checking $fileSize and
$path, emit a clear warning (use trigger_error or the class logger if available)
that includes $path, the actual $fileSize (or failure), and the min/max
thresholds, and optionally replace the magic numbers with class constants or
constructor parameters (e.g., $minCacheSize/$maxCacheSize) so callers can adjust
limits; keep the existing continue behavior after logging unless a config flag
instructs stricter handling.
| private static function normalizeHost(string $input): string | ||
| { | ||
| $host = (strpos($input, '/') !== false) ? parse_url($input, PHP_URL_HOST) : $input; | ||
| $host = str_contains($input, '/') ? parse_url($input, PHP_URL_HOST) : $input; |
There was a problem hiding this comment.
str_contains() breaks PHP 7.4 compatibility — use strpos() instead.
str_contains() was introduced in PHP 8.0. Since this library targets PHP 7.4+, this will cause a fatal error on PHP 7.4 installations.
🔧 PHP 7.4-compatible fix
- $host = str_contains($input, '/') ? parse_url($input, PHP_URL_HOST) : $input;
+ $host = strpos($input, '/') !== false ? parse_url($input, PHP_URL_HOST) : $input;As per coding guidelines: "Review for PHP 7.4 compatibility (no union types, named arguments, match expressions, enums, readonly, constructor promotion, fibers, intersection types, never return type, or first-class callable syntax)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $host = str_contains($input, '/') ? parse_url($input, PHP_URL_HOST) : $input; | |
| $host = strpos($input, '/') !== false ? parse_url($input, PHP_URL_HOST) : $input; |
🤖 Prompt for AI Agents
In `@src/RegisteredDomain.php` at line 116, The use of PHP 8's str_contains breaks
PHP 7.4 support; update the expression that sets $host in RegisteredDomain (the
ternary using str_contains($input, '/')) to use strpos($input, '/') !== false
instead so it remains PHP 7.4-compatible when computing $host from $input and
calling parse_url.
| $longestMatch = null; | ||
| foreach ($suffixes as $suffix => $result) { | ||
| if (substr($h, -strlen($suffix)) === $suffix) { | ||
| if (str_ends_with($h, $suffix)) { |
There was a problem hiding this comment.
Critical: str_ends_with() breaks PHP 7.4 compatibility.
The str_ends_with() function was introduced in PHP 8.0. Since this library supports PHP 7.4 through 8.5, this will cause a fatal error on PHP 7.4 installations.
🐛 Proposed fix for PHP 7.4 compatibility
- if (str_ends_with($h, $suffix)) {
+ if (substr($h, -strlen($suffix)) === $suffix) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (str_ends_with($h, $suffix)) { | |
| if (substr($h, -strlen($suffix)) === $suffix) { |
🤖 Prompt for AI Agents
In `@tests/unit/RegisteredDomainTest.php` at line 61, Replace the PHP 8-only call
to str_ends_with($h, $suffix) with a PHP 7.4-compatible check: ensure
empty-suffix behavior is preserved and avoid negative-length issues by using
($suffix === '' || (strlen($suffix) <= strlen($h) && substr($h,
-strlen($suffix)) === $suffix)). Update the conditional where str_ends_with($h,
$suffix) appears (in the RegisteredDomainTest logic using variables $h and
$suffix) to use this expression.
- Use ./vendor/bin/phpunit instead of bare phpunit in ci.yml and sonarcloud.yml (bare phpunit not on PATH in GitHub Actions) - Remove sonar.test.exclusions=tests/** which prevented SonarCloud from indexing test files and collecting test metrics Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 29-85: Replace mutable action tags with pinned commit SHAs for
each external action use to prevent supply-chain risk: locate the action usages
actions/checkout@v5, shivammathur/setup-php@v2, actions/cache@v4 and
codecov/codecov-action@v5 in the workflow and change the version tags to their
corresponding full commit SHA (e.g., actions/checkout@<full-sha>) by finding the
latest tested commit for each action repository, update the workflow to those
SHAs, and run a CI test to ensure compatibility.
In @.github/workflows/sonarcloud.yml:
- Around line 14-38: Replace the three mutable action tags with their pinned
commit SHAs: change actions/checkout@v5, shivammathur/setup-php@v2, and
SonarSource/sonarqube-scan-action@v7 to use full 40-character commit SHAs (one
SHA per action) while preserving the human-readable version as an inline
comment; update the workflow entries for the checkout step, the Setup PHP step,
and the SonarCloud Scan step to use the exact SHAs from each official repo and
verify each SHA before committing.
- qodana.yaml: pin linter to jetbrains/qodana-php:2025.2 (was :latest) - sonarcloud.yml: pin all actions to full SHA hashes (security best practice) - sonarcloud.yml: only run on upstream repo (if: github.repository) - sonarcloud.yml: skip scan step gracefully if SONAR_TOKEN not set Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Pin actions/checkout, shivammathur/setup-php, actions/cache, codecov/codecov-action, JetBrains/qodana-action to commit SHA hashes to prevent supply-chain attacks - Fix SonarCloud token check from env.SONAR_TOKEN to secrets.SONAR_TOKEN (env context not available at step-level if condition) - Add repository guard to Qodana workflow (skip on forks) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 75-80: The CI is running tests twice because the "Run all CI
checks" step executes composer ci and the "Generate coverage report" step runs
phpunit again when matrix.coverage is true; update the "Run all CI checks" step
to only run when coverage is false (add an if: condition using matrix.coverage,
e.g. if: ${{ matrix.coverage == false }}) so that the composer ci step is
skipped for coverage matrix entries, or alternatively modify the composer ci
script to accept a COVERAGE flag and generate the coverage report inside
composer ci when matrix.coverage is true (use the matrix.coverage variable to
drive either behavior).
In @.github/workflows/sonarcloud.yml:
- Around line 38-42: The step-level `if:` uses an invalid `secrets` context and
will break the workflow; change the logic to check an environment variable
instead: set SONAR_TOKEN in the step's env (already present) and replace the
`if: ${{ secrets.SONAR_TOKEN != '' }}` condition with a check against the env
var (e.g., `if: env.SONAR_TOKEN != ''`) on the "SonarCloud Scan" step so the
`uses: SonarSource/sonarqube-scan-action@...` runs only when SONAR_TOKEN is
populated.
| - name: Run all CI checks | ||
| run: composer ci | ||
|
|
||
| - name: Generate coverage report | ||
| if: ${{ matrix.coverage }} | ||
| run: ./vendor/bin/phpunit --colors=always --coverage-clover build/logs/clover.xml |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Coverage matrix entry runs tests twice.
The coverage matrix entry (php: 8.3, coverage: true) executes composer ci (line 76), which likely includes PHPUnit, then runs PHPUnit again with coverage (line 80). This doubles test execution time for coverage builds.
Consider either:
- Skipping
composer ciwhen coverage is enabled, or - Integrating coverage generation into the
composer ciscript conditionally.
♻️ Option 1: Skip ci script for coverage runs
- name: Run all CI checks
+ if: ${{ !matrix.coverage }}
run: composer ci
- name: Generate coverage report
if: ${{ matrix.coverage }}
- run: ./vendor/bin/phpunit --colors=always --coverage-clover build/logs/clover.xml
+ run: |
+ composer ci
+ ./vendor/bin/phpunit --colors=always --coverage-clover build/logs/clover.xmlOr if composer ci already runs PHPUnit:
- name: Run all CI checks
+ if: ${{ !matrix.coverage }}
run: composer ci
- name: Generate coverage report
if: ${{ matrix.coverage }}
run: ./vendor/bin/phpunit --colors=always --coverage-clover build/logs/clover.xml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run all CI checks | |
| run: composer ci | |
| - name: Generate coverage report | |
| if: ${{ matrix.coverage }} | |
| run: ./vendor/bin/phpunit --colors=always --coverage-clover build/logs/clover.xml | |
| - name: Run all CI checks | |
| if: ${{ !matrix.coverage }} | |
| run: composer ci | |
| - name: Generate coverage report | |
| if: ${{ matrix.coverage }} | |
| run: | | |
| composer ci | |
| ./vendor/bin/phpunit --colors=always --coverage-clover build/logs/clover.xml |
| - name: Run all CI checks | |
| run: composer ci | |
| - name: Generate coverage report | |
| if: ${{ matrix.coverage }} | |
| run: ./vendor/bin/phpunit --colors=always --coverage-clover build/logs/clover.xml | |
| - name: Run all CI checks | |
| if: ${{ !matrix.coverage }} | |
| run: composer ci | |
| - name: Generate coverage report | |
| if: ${{ matrix.coverage }} | |
| run: ./vendor/bin/phpunit --colors=always --coverage-clover build/logs/clover.xml |
🤖 Prompt for AI Agents
In @.github/workflows/ci.yml around lines 75 - 80, The CI is running tests twice
because the "Run all CI checks" step executes composer ci and the "Generate
coverage report" step runs phpunit again when matrix.coverage is true; update
the "Run all CI checks" step to only run when coverage is false (add an if:
condition using matrix.coverage, e.g. if: ${{ matrix.coverage == false }}) so
that the composer ci step is skipped for coverage matrix entries, or
alternatively modify the composer ci script to accept a COVERAGE flag and
generate the coverage report inside composer ci when matrix.coverage is true
(use the matrix.coverage variable to drive either behavior).
| - name: SonarCloud Scan | ||
| if: ${{ secrets.SONAR_TOKEN != '' }} | ||
| uses: SonarSource/sonarqube-scan-action@a31c9398be7ace6bbfaf30c0bd5d415f843d45e9 # v7 | ||
| env: | ||
| SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} |
There was a problem hiding this comment.
Invalid secrets context in if: condition — will cause workflow parse error.
The secrets context is not available in step-level if: expressions. As flagged by actionlint, only env, github, inputs, job, matrix, needs, runner, steps, strategy, and vars contexts are permitted. Use the environment variable intermediary pattern instead.
🔧 Proposed fix
jobs:
sonarcloud:
name: SonarCloud Analysis
runs-on: ubuntu-latest
if: github.repository == 'XOOPS/RegDom'
+ env:
+ SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
steps:
# ... other steps unchanged ...
- name: SonarCloud Scan
- if: ${{ secrets.SONAR_TOKEN != '' }}
+ if: env.SONAR_TOKEN != ''
uses: SonarSource/sonarqube-scan-action@a31c9398be7ace6bbfaf30c0bd5d415f843d45e9 # v7
env:
SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}🧰 Tools
🪛 actionlint (1.7.10)
[error] 39-39: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🤖 Prompt for AI Agents
In @.github/workflows/sonarcloud.yml around lines 38 - 42, The step-level `if:`
uses an invalid `secrets` context and will break the workflow; change the logic
to check an environment variable instead: set SONAR_TOKEN in the step's env
(already present) and replace the `if: ${{ secrets.SONAR_TOKEN != '' }}`
condition with a check against the env var (e.g., `if: env.SONAR_TOKEN != ''`)
on the "SonarCloud Scan" step so the `uses:
SonarSource/sonarqube-scan-action@...` runs only when SONAR_TOKEN is populated.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 32 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Licensed under Apache License 2.0 or Mozilla Public License 2.0 (dual-licensed). | ||
| See [LICENSE.txt](LICENSE.txt) for details. |
There was a problem hiding this comment.
README claims the project is “dual-licensed” under Apache 2.0 or MPL 2.0 and points to LICENSE.txt, but LICENSE.txt currently contains the Apache 2.0 text only. If MPL-2.0 applies only to the bundled PSL data/cache, clarify that distinction (e.g., library code Apache-2.0; PSL data under MPL-2.0) and/or include the appropriate MPL license notice/file so the README matches the repository contents.
| Licensed under Apache License 2.0 or Mozilla Public License 2.0 (dual-licensed). | |
| See [LICENSE.txt](LICENSE.txt) for details. | |
| The PHP library code in this repository is licensed under the Apache License 2.0. | |
| See [LICENSE.txt](LICENSE.txt) for the full Apache 2.0 license text. | |
| The bundled Public Suffix List data/cache is derived from Mozilla's Public Suffix List | |
| and is available under the Mozilla Public License 2.0 (MPL-2.0). For details, see | |
| https://www.mozilla.org/en-US/MPL/2.0/ and https://publicsuffix.org/. |
| .scrutinizer.yml export-ignore | ||
| .travis.yml export-ignore | ||
| phpcs.xml.dist export-ignore | ||
| phpcs.xml export-ignore |
There was a problem hiding this comment.
.gitattributes marks phpcs.xml as export-ignore, but this repo doesn’t contain a phpcs.xml file (and this PR removes the previous PHPCS ruleset). Either add the intended PHPCS config file with the correct name or remove/fix this export-ignore entry to avoid confusion about the expected ruleset filename.
| phpcs.xml export-ignore |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 32 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| <coverage processUncoveredFiles="true"> | ||
| <source> | ||
| <include> | ||
| <directory suffix=".php">./src</directory> | ||
| </include> | ||
| <report> | ||
| <clover outputFile="build/logs/clover.xml"/> | ||
| </report> | ||
| </coverage> | ||
|
|
||
| </source> | ||
| </phpunit> |
There was a problem hiding this comment.
The PHPUnit configuration switches to <source> (PHPUnit 10+ schema), but composer.json allows PHPUnit 9.6 (needed for PHP 7.4). PHPUnit 9 will fail to load/validate this config against its XSD. To keep PHP 7.4 support, either revert to the PHPUnit 9 schema (<coverage>/<filter>), or split configs per major version and select via CI/Composer.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 32 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function getPublicSuffix(string $domain): ?string | ||
| { | ||
| $domain = $this->normalizeDomain($domain); | ||
| $domain = self::normalizeDomain($domain); |
There was a problem hiding this comment.
getPublicSuffix() appears to implement PSL exception handling incorrectly: when an exception rule matches, it should return the public suffix after removing the leftmost label of the exception (e.g., for sub.city.kawasaki.jp the public suffix should be kawasaki.jp), but the current exception branch returns a suffix that includes additional labels. This will produce incorrect registrable domains for subdomains under exception rules. Adjust the exception branch to return the suffix starting at $i + 1 and add a test case like sub.city.kawasaki.jp (and sub.www.ck) to prevent regressions.
| private static function normalizeHost(string $input): string | ||
| { | ||
| $host = (strpos($input, '/') !== false) ? parse_url($input, PHP_URL_HOST) : $input; | ||
| $host = str_contains($input, '/') ? parse_url($input, PHP_URL_HOST) : $input; |
There was a problem hiding this comment.
normalizeHost() strips brackets from IPv6 literals and then applies preg_replace('/:\d+$/', ...), which will mistakenly treat the last hextet of an IPv6 address like ::1 as a port and truncate it (e.g., [::1] becomes :). This can cause IPv6 addresses to bypass the later FILTER_VALIDATE_IP checks in domainMatches() and be treated as normal domains. Update the port-stripping logic to only remove ports from non-IPv6 hosts (or handle the bracketed IPv6 + optional port form explicitly, e.g. [::1]:443).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 32 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'sch.id' => true, | ||
| 'web.id' => true, | ||
| 'ᬩᬮᬶ.id' => true, | ||
| 'ie' => true, | ||
| 'gov.ie' => true, | ||
| 'il' => true, |
There was a problem hiding this comment.
This cache now contains Unicode PSL rules (e.g. ᬩᬮᬶ.id, ישראל). With ext-intl enabled, PublicSuffixList::normalizeDomain() converts domains to ASCII/punycode before lookup, so these Unicode keys will never be found and PSL matching for IDN suffixes will be wrong. The cache generator should store rules in the same normalized form used at runtime (typically punycode) to keep behavior consistent across environments.
| public function getPublicSuffix(string $domain): ?string | ||
| { | ||
| $domain = $this->normalizeDomain($domain); | ||
| $domain = self::normalizeDomain($domain); | ||
| if ($domain === '' || filter_var($domain, FILTER_VALIDATE_IP)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
In getPublicSuffix(), PSL exception handling later in the method appears incorrect: when a $testSuffix matches an exception rule (e.g. www.ck), the public suffix should be the exception with the leftmost label removed (e.g. ck), but the current logic returns one label more (which can yield the full domain for a.www.ck and null for www.ck). This will break registered-domain extraction for exception-based suffixes; adjust the exception branch and add a test for www.ck / a.www.ck.


Summary by CodeRabbit
New Features
Data
Bug Fixes
Tests
Documentation