Conversation
… always be enabled/disabled.
# Conflicts: # src/RewriteUrl.php
…easier for future popular scripts to be automatically excluded.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
composer.json (1)
24-25: Coverage script is not shell-portable as written.Line 25 uses POSIX-style
XDEBUG_MODE=coverage ..., which won’t run under Windowscmd. A more portable Composer-script form is to set Xdebug mode via PHP args.Portable alternative
- "XDEBUG_MODE=coverage ./vendor/bin/phpunit --coverage-clover ./build/clover.xml" + "@php -d xdebug.mode=coverage ./vendor/bin/phpunit --coverage-clover ./build/clover.xml"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` around lines 24 - 25, The "coverage" Composer script currently uses a POSIX env assignment ("XDEBUG_MODE=coverage ...") which is not Windows cmd portable; update the "coverage" script entry (the "coverage" key in composer.json) to invoke PHP with a PHP INI override instead, e.g. run php -d xdebug.mode=coverage to set Xdebug mode and then call ./vendor/bin/phpunit --coverage-clover ./build/clover.xml so the command works on both Unix and Windows shells.src/Download.php (1)
89-92: Redundant normalization - but hey, redundancy is the spice of defensive programming!taps keyboard thoughtfully
I notice this protocol-relative URL normalization (Lines 90-92) is also performed in
download_file()andparse_font_faces()before calling this method. It's not harmful - just a bit like wearing both a belt AND suspenders. Which, come to think of it, is perfectly sensible if you really don't want your trousers to fall down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Download.php` around lines 89 - 92, Remove the redundant protocol-relative URL normalization (the str_starts_with($url, '//') check and 'https:' prefixing) from this method since download_file() and parse_font_faces() already perform it; either delete that block here or extract a single helper (e.g., normalize_protocol_relative_url) and call it from callers to centralize behavior, leaving download_file() and parse_font_faces() as the single normalization points to avoid duplicate logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Around line 14-28: The scripts "lint", "unit", "integration", and "coverage"
reference vendor binaries that won't exist because the "require-dev" section is
empty; update require-dev to include the dev packages used by those scripts
(e.g., php-parallel-lint/php-parallel-lint for the "lint" script,
phpunit/phpunit for "unit" and "integration", and an appropriate coverage tool
such as phpunit/php-code-coverage or ext-xdebug/phpdbg for "coverage") so
composer install will install those binaries; also consider adjusting the
"coverage" invocation (currently using XDEBUG_MODE=coverage
./vendor/bin/phpunit) to a cross-platform approach if Windows support is
required.
In `@src/Admin/Settings/Manage.php`:
- Around line 328-335: The checkbox is receiving a boolean because
Manage::test_mode() passes GDPRESS_TEST_MODE === 'on' to do_checkbox(), but
do_checkbox() (in Builder.php) expects the raw string (it checks $checked ===
'on'); change the third argument in Manage::test_mode() to pass
GDPRESS_TEST_MODE directly (not the === comparison) so do_checkbox() can
correctly detect the 'on' string and render the checkbox checked.
In `@src/Download.php`:
- Around line 190-192: In download_file(), the copy($tmp, $path . $filename)
call must have its return value checked like the earlier block: test if
copy(...) returns true, and on failure log an error (including $tmp and
$path.$filename) and return false (or otherwise signal failure) before
unlinking; reference the download_file() function and the variables $tmp, $path
and $filename to locate and update the logic so the code doesn't proceed as if
the font was written when copy() failed.
---
Nitpick comments:
In `@composer.json`:
- Around line 24-25: The "coverage" Composer script currently uses a POSIX env
assignment ("XDEBUG_MODE=coverage ...") which is not Windows cmd portable;
update the "coverage" script entry (the "coverage" key in composer.json) to
invoke PHP with a PHP INI override instead, e.g. run php -d xdebug.mode=coverage
to set Xdebug mode and then call ./vendor/bin/phpunit --coverage-clover
./build/clover.xml so the command works on both Unix and Windows shells.
In `@src/Download.php`:
- Around line 89-92: Remove the redundant protocol-relative URL normalization
(the str_starts_with($url, '//') check and 'https:' prefixing) from this method
since download_file() and parse_font_faces() already perform it; either delete
that block here or extract a single helper (e.g.,
normalize_protocol_relative_url) and call it from callers to centralize
behavior, leaving download_file() and parse_font_faces() as the single
normalization points to avoid duplicate logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3ef7e39-bdaa-4582-9718-bb1ca41fabf2
📒 Files selected for processing (6)
composer.jsonsrc/Admin/Settings.phpsrc/Admin/Settings/Builder.phpsrc/Admin/Settings/Help.phpsrc/Admin/Settings/Manage.phpsrc/Download.php
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
composer.json (2)
3-11: Move test namespaces fromautoloadtoautoload-dev.Right, tiny snag: at Line 6 through Line 10, test-only namespaces are being shipped in production autoloading. Better keep only
GDPRess\\inautoloadand move test mappings toautoload-dev.Proposed refactor
"autoload": { "psr-4": { - "GDPRess\\": "src/", - "GDPRess\\Tests\\Integration\\": "tests/integration/", - "GDPRess\\Tests\\Mocks\\": "tests/mocks/", - "GDPRess\\Tests\\Unit\\": "tests/unit/", - "GDPRess\\Tests\\": "tests/" + "GDPRess\\": "src/" } }, + "autoload-dev": { + "psr-4": { + "GDPRess\\Tests\\": "tests/" + } + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` around lines 3 - 11, The composer.json currently exposes test-only PSR-4 mappings in "autoload"; update composer.json so that only the production namespace "GDPRess\\\\" remains under "autoload" and move all test-only namespaces (the PSR-4 entries you added for tests) into "autoload-dev" under PSR-4; after editing, run composer dump-autoload / composer validate to ensure the JSON and autoload mappings are correct and that tests are loaded only in dev.
19-21: Use Composer’s binary proxy forparallel-lint.At Line 20, the script reaches into vendor internals. Much safer to call the proxy in
vendor/bin, otherwise updates may break the path.Proposed refactor
- "@php ./vendor/php-parallel-lint/php-parallel-lint/parallel-lint . -e php --show-deprecated --exclude vendor --exclude .git" + "@php ./vendor/bin/parallel-lint . -e php --show-deprecated --exclude vendor --exclude .git"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` around lines 19 - 21, The composer.json scripts currently call the package internals rather than the Composer binary proxy; update the script that invokes the linter so it runs vendor/bin/parallel-lint (or simply "vendor/bin/parallel-lint" from the scripts section) instead of reaching into vendor/composer/... internals—locate the script entry referencing parallel-lint in composer.json and replace the direct vendor internals path with the vendor/bin proxy to ensure future updates won't break the path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@composer.json`:
- Around line 3-11: The composer.json currently exposes test-only PSR-4 mappings
in "autoload"; update composer.json so that only the production namespace
"GDPRess\\\\" remains under "autoload" and move all test-only namespaces (the
PSR-4 entries you added for tests) into "autoload-dev" under PSR-4; after
editing, run composer dump-autoload / composer validate to ensure the JSON and
autoload mappings are correct and that tests are loaded only in dev.
- Around line 19-21: The composer.json scripts currently call the package
internals rather than the Composer binary proxy; update the script that invokes
the linter so it runs vendor/bin/parallel-lint (or simply
"vendor/bin/parallel-lint" from the scripts section) instead of reaching into
vendor/composer/... internals—locate the script entry referencing parallel-lint
in composer.json and replace the direct vendor internals path with the
vendor/bin proxy to ensure future updates won't break the path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f10d3502-ad34-4476-8268-a2d8a6e0c9a8
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.gitignorecomposer.jsonsrc/Download.phpvendor/composer/InstalledVersions.phpvendor/composer/autoload_classmap.phpvendor/composer/autoload_files.phpvendor/composer/autoload_psr4.phpvendor/composer/autoload_real.phpvendor/composer/autoload_static.phpvendor/composer/installed.jsonvendor/composer/installed.php
✅ Files skipped from review due to trivial changes (2)
- vendor/composer/autoload_files.php
- vendor/composer/autoload_psr4.php
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- src/Download.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Helper.php (1)
22-26:⚠️ Potential issue | 🟡 MinorGuard empty local paths before
file_exists().Line 25 and Line 152 still evaluate
file_exists()when path resolution returns''. That turns an ordinary cache miss into PHP warnings.Possible fix
public static function get_local_url( $url, $type, $bypass = false ) { $local_path = self::get_local_path( $url, $type ); + + if ( ! $local_path ) { + return ''; + } if ( ! file_exists( $local_path ) && ! $bypass ) { return ''; } @@ public static function get_local_url_google_font( $filename, $bypass = false ) { $rel_path = self::sanitize_relative_path( "/$filename/google-fonts.css" ); if ( ! $rel_path ) { return ''; } - if ( ! file_exists( self::get_local_path_google_font( $filename ) ) && ! $bypass ) { + $local_path = self::get_local_path_google_font( $filename ); + + if ( ! $local_path ) { + return ''; + } + + if ( ! file_exists( $local_path ) && ! $bypass ) { return ''; }Also applies to: 145-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helper.php` around lines 22 - 26, The code calls file_exists() on the result of self::get_local_path() without guarding against an empty string which raises PHP warnings; update get_local_url to first check if $local_path is non-empty (e.g., === '' or empty()) and return '' before calling file_exists(), and apply the same guard to the other occurrence that checks file_exists() against the output of self::get_local_path (the block around lines referencing the same logic), ensuring all file_exists() calls only run when the resolved local path is a non-empty string.src/Admin/Settings/Manage.php (1)
133-135:⚠️ Potential issue | 🟡 MinorUse the Google Fonts cache helpers for all Google Fonts requests.
Line 133 only switches helpers when
$request_type === 'google_fonts', butsrc/Download.php, Lines 33-39 store anyHelper::is_google_fonts_request()URL withget_local_*_google_font()instead. Small Google Fonts requests andupsellcases will therefore look undownloaded here even when the file exists.Possible fix
- $local_url = $request_type === 'google_fonts' ? Helper::get_local_url_google_font( $request['name'] ) : Helper::get_local_url( $request['href'], $type ); - $downloaded = $request_type === 'google_fonts' ? file_exists( Helper::get_local_path_google_font( $request['name'] ) ) : file_exists( Helper::get_local_path( $request['href'], - $type ) ); + $is_google_fonts = Helper::is_google_fonts_request( $request['href'] ); + $local_url = $is_google_fonts ? Helper::get_local_url_google_font( $request['name'] ) : Helper::get_local_url( $request['href'], $type ); + $local_path = $is_google_fonts ? Helper::get_local_path_google_font( $request['name'] ) : Helper::get_local_path( $request['href'], $type ); + $downloaded = $local_path && file_exists( $local_path );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Admin/Settings/Manage.php` around lines 133 - 135, The branch deciding which cache helper to use currently checks $request_type === 'google_fonts' but downloads are determined by Helper::is_google_fonts_request(), causing mismatches; update the condition in the Manage.php logic to call Helper::is_google_fonts_request(...) (passing the appropriate value from $request, e.g. $request['href'] or $request['name'] as used elsewhere) and then use Helper::get_local_url_google_font / Helper::get_local_path_google_font when that helper returns true, otherwise fall back to Helper::get_local_url / Helper::get_local_path so cached Google Fonts are correctly detected.src/Download.php (1)
260-266:⚠️ Potential issue | 🟠 MajorClamp
../resolution at the source origin.I’m afraid Line 261 can still trim
https://cdn.example.com/css/main.cssdown tohttps:/orhttps:when the relative path climbs above root. Browsers clamp at the origin, so those font downloads will fail.Possible fix
+ $parts = parse_url( $source ); + $origin = isset( $parts['scheme'], $parts['host'] ) + ? $parts['scheme'] . '://' . $parts['host'] . ( isset( $parts['port'] ) ? ':' . $parts['port'] : '' ) + : ''; + for ( $i = 0; $i <= $folder_depth; $i ++ ) { - $url_to_insert = substr( $url_to_insert, 0, strrpos( $url_to_insert, '/' ) ); + $slash = strrpos( $url_to_insert, '/' ); + + if ( $slash === false || ( $origin && $slash < strlen( $origin ) ) ) { + $url_to_insert = $origin ?: $url_to_insert; + break; + } + + $url_to_insert = substr( $url_to_insert, 0, $slash ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Download.php` around lines 260 - 266, The loop that trims $url_to_insert by folder depth can strip past the origin (e.g. turning https://cdn.example.com/... into https:/), so modify the logic in the Download class where $url_to_insert, $folder_depth and $rel_url are handled: compute the origin (scheme + '://' + host and include port if present) using parse_url, then in the for loop that does substr/strrpos stop trimming once $url_to_insert would be equal to or shorter than that origin (or when strrpos returns a position <= strlen($origin)); finally build the return value by joining the clamped $url_to_insert and the sanitized $path so relative climbs never go above the origin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Download.php`:
- Around line 164-166: The bug is that you strip the query from $filename then
compute $dir from the already-stripped name, producing a $dir containing the
query (e.g. ".../?v=1") so replace_abs_urls() can't match the original URL; fix
by deriving $dir from the URL without the query (or by parsing the URL path
first) before removing the query portion for $filename — e.g. compute the
directory using the URL path (using parse_url() or strtok($url, '?')) and only
then strip the query to set $filename, keeping Helper::get_local_path and
replace_abs_urls() working with the original path form.
---
Duplicate comments:
In `@src/Admin/Settings/Manage.php`:
- Around line 133-135: The branch deciding which cache helper to use currently
checks $request_type === 'google_fonts' but downloads are determined by
Helper::is_google_fonts_request(), causing mismatches; update the condition in
the Manage.php logic to call Helper::is_google_fonts_request(...) (passing the
appropriate value from $request, e.g. $request['href'] or $request['name'] as
used elsewhere) and then use Helper::get_local_url_google_font /
Helper::get_local_path_google_font when that helper returns true, otherwise fall
back to Helper::get_local_url / Helper::get_local_path so cached Google Fonts
are correctly detected.
In `@src/Download.php`:
- Around line 260-266: The loop that trims $url_to_insert by folder depth can
strip past the origin (e.g. turning https://cdn.example.com/... into https:/),
so modify the logic in the Download class where $url_to_insert, $folder_depth
and $rel_url are handled: compute the origin (scheme + '://' + host and include
port if present) using parse_url, then in the for loop that does substr/strrpos
stop trimming once $url_to_insert would be equal to or shorter than that origin
(or when strrpos returns a position <= strlen($origin)); finally build the
return value by joining the clamped $url_to_insert and the sanitized $path so
relative climbs never go above the origin.
In `@src/Helper.php`:
- Around line 22-26: The code calls file_exists() on the result of
self::get_local_path() without guarding against an empty string which raises PHP
warnings; update get_local_url to first check if $local_path is non-empty (e.g.,
=== '' or empty()) and return '' before calling file_exists(), and apply the
same guard to the other occurrence that checks file_exists() against the output
of self::get_local_path (the block around lines referencing the same logic),
ensuring all file_exists() calls only run when the resolved local path is a
non-empty string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93721e8d-4bc6-4c92-8af8-50edf5194481
📒 Files selected for processing (5)
composer.jsonsrc/Admin/Settings/Builder.phpsrc/Admin/Settings/Manage.phpsrc/Download.phpsrc/Helper.php
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/Download.php (2)
154-166:⚠️ Potential issue | 🟠 MajorOh dear, the rewrite key no longer matches the CSS source.
Line 155 turns
//...intohttps://..., and Lines 164-165 derive$dirafter stripping the query from$filename. By Line 185,replace_abs_urls()is asked to replace a string that never appears in$contents, so protocol-relative fonts and?v=absolute URLs stay remote.🛠️ Suggested fix
- foreach ( $urls as $url ) { + foreach ( $urls as $url ) { + $matched_url = $url; + // Normalize protocol-relative URLs. if ( str_starts_with( $url, '//' ) ) { $url = 'https:' . $url; } @@ - [ $filename ] = explode( '?', basename( $url ) ); - $dir = str_replace( $filename, '', $url ); + $basename = basename( $url ); + $matched_base = basename( $matched_url ); + [ $filename ] = explode( '?', $basename, 2 ); + $dir = substr( $url, 0, -strlen( $basename ) ); + $rewrite_dir = substr( $matched_url, 0, -strlen( $matched_base ) ); $path = Helper::get_local_path( $dir, 'css' ); @@ - if ( ! $is_rel_url ) { - $contents = $this->replace_abs_urls( $contents, $dir ); + if ( ! $is_rel_url ) { + $contents = $this->replace_abs_urls( $contents, $rewrite_dir ); }Also applies to: 184-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Download.php` around lines 154 - 166, The rewrite uses a mutated $url (protocol-normalized and query-stripped) so replace_abs_urls() never matches the original source string; restore and use the original URL when computing replacements: capture $orig_url = $url before normalizing with str_starts_with and before stripping the query (keep using is_rel_url and get_abs_url for path resolution), use basename/explode/Helper::get_local_path on the cleaned URL to compute $path/$dir/$filename but pass $orig_url (and a normalized protocol-relative variant) into replace_abs_urls() so protocol-relative URLs and ?v= query variants are matched and replaced correctly; update calls to replace_abs_urls() to reference $orig_url (and its protocol-normalized counterpart) rather than the post-processed $url.
250-267:⚠️ Potential issue | 🟠 MajorRight,
../resolution still walks straight past the origin.Lines 261-263 keep trimming until
https://cdn.example.com/...becomeshttps:/and then''. Above-root traversal should clamp at the source origin, otherwise font downloads can be built from malformed URLs.🛠️ Suggested fix
- $folder_depth = substr_count( $rel_url, '../' ); - $url_to_insert = $source; - - /** - * Remove everything after the last occurrence of a forward slash ('/'); - * - * $i = 0: Filename - * 1: First level subdirectory, i.e. '../' - * 2: 2nd level subdirectory, i.e. '../../' - * 3: Etc. - */ - for ( $i = 0; $i <= $folder_depth; $i ++ ) { - $url_to_insert = substr( $url_to_insert, 0, strrpos( $url_to_insert, '/' ) ); - } - - $path = ltrim( $rel_url, './' ); - - return $url_to_insert . '/' . $path; + $parts = wp_parse_url( $source ); + + if ( ! isset( $parts['scheme'], $parts['host'], $parts['path'] ) ) { + return $rel_url; + } + + $segments = array_values( + array_filter( + explode( '/', trim( dirname( $parts['path'] ), '/' ) ), + 'strlen' + ) + ); + + foreach ( explode( '/', $rel_url ) as $segment ) { + if ( $segment === '' || $segment === '.' ) { + continue; + } + + if ( $segment === '..' ) { + array_pop( $segments ); + continue; + } + + $segments[] = $segment; + } + + $origin = $parts['scheme'] . '://' . $parts['host'] . ( isset( $parts['port'] ) ? ':' . $parts['port'] : '' ); + + return $origin . '/' . implode( '/', $segments );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Download.php` around lines 250 - 267, The loop that trims $url_to_insert using strrpos allows above-root traversal to strip past the origin (e.g. turning https://cdn.example.com/... into invalid strings); modify the logic in the block using $folder_depth and $url_to_insert so you clamp trimming at the source origin: compute the origin from $source (e.g. scheme + '://' + host, or parse_url($source) components) and stop the for-loop (or break) when $url_to_insert equals the origin or when strrpos($url_to_insert, '/') falls inside the origin boundary; ensure you also handle cases where strrpos returns false to avoid further substr calls, then continue building $path with ltrim($rel_url, './') and return origin + '/' + $path.src/Helper.php (1)
79-81:⚠️ Potential issue | 🟠 MajorGood lord, this cache-dir check still accepts dot-segment escapes.
Line 136 is only a prefix check, and
wp_normalize_path()does not collapse... Because Line 79 concatenates raw$type,get_local_path( $url, '../outside' )still passes this guard and can point outsideGDPRESS_CACHE_ABSPATH.🛠️ Suggested fix
public static function get_local_path( $url, $type ) { if ( ! isset( $url ) ) { return ''; } + + $type = trim( (string) $type, '/' ); + + if ( $type === '' || preg_match( '#(^|/)\.\.?(?:/|$)#', $type ) ) { + return ''; + } $path = wp_parse_url( $url, PHP_URL_PATH ); @@ private static function is_within_cache_dir( $path ) { - $path = wp_normalize_path( $path ); - $cache_dir_path = wp_normalize_path( GDPRESS_CACHE_ABSPATH ); - - return str_starts_with( $path, $cache_dir_path ); + $path = wp_normalize_path( $path ); + $cache_dir_path = trailingslashit( wp_normalize_path( GDPRESS_CACHE_ABSPATH ) ); + + if ( ! str_starts_with( $path, $cache_dir_path ) ) { + return false; + } + + $relative = ltrim( substr( $path, strlen( $cache_dir_path ) ), '/' ); + + return preg_match( '#(^|/)\.\.(?:/|$)#', $relative ) !== 1; }Also applies to: 132-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helper.php` around lines 79 - 81, The concatenation of $type and $rel_path into $full_path allows dot-segment escapes (e.g., "../") because wp_normalize_path doesn't collapse “..”, so is_within_cache_dir is bypassed; fix by resolving dot-segments before checking: build the candidate path, normalize it (wp_normalize_path), then collapse dot-segments (or call realpath on the intended directory when possible) to produce a canonical path and pass that to is_within_cache_dir; update the code paths where $full_path is formed (the block using $full_path, get_local_path, and is_within_cache_dir) so the check uses the canonical/resolved path rather than a simple prefix check.
🧹 Nitpick comments (1)
tests/bootstrap.php (1)
2-6: Update the copied package annotation.I’m afraid Line 5 still says
Host_Webfonts_Local, which looks borrowed from another plugin. Please point it at this package so docs and IDE metadata stay accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bootstrap.php` around lines 2 - 6, Update the PHPDoc `@package` annotation in the PHPUnit bootstrap PHPDoc block by replacing the incorrect placeholder "Host_Webfonts_Local" with the actual package name for this project (i.e., the plugin’s real package identifier) so IDE metadata and generated docs are accurate; locate the PHPDoc block at the top of tests/bootstrap.php and change the `@package` value accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/install-wp-tests.sh`:
- Around line 107-113: The script currently skips exporting test assets if
$WP_TESTS_DIR exists, which fails when the directory is present but missing
files; instead check for the actual test assets (e.g.,
$WP_TESTS_DIR/includes/functions.php and the data directory) and only skip
export when those exist. Modify the block using WP_TESTS_DIR and WP_TESTS_TAG
to: ensure the directory exists (mkdir -p), and conditionally svn export the
includes and data paths when the specific assets (includes/functions.php and
$WP_TESTS_DIR/data) are missing or incomplete so partial installs are repaired
on re-run.
- Around line 115-124: The script currently tests for wp-tests-config.php in the
current directory, but then writes to "$WP_TESTS_DIR"/wp-tests-config.php,
causing false positives; update the existence check to look for
"$WP_TESTS_DIR/wp-tests-config.php" (and ensure the path is quoted) before
downloading and editing the sample, so the block that manipulates WP_CORE_DIR
and writes to "$WP_TESTS_DIR"/wp-tests-config.php only runs when that target
file is missing; reference the WP_TESTS_DIR and WP_CORE_DIR variables and the
wp-tests-config.php filename when making this change.
---
Duplicate comments:
In `@src/Download.php`:
- Around line 154-166: The rewrite uses a mutated $url (protocol-normalized and
query-stripped) so replace_abs_urls() never matches the original source string;
restore and use the original URL when computing replacements: capture $orig_url
= $url before normalizing with str_starts_with and before stripping the query
(keep using is_rel_url and get_abs_url for path resolution), use
basename/explode/Helper::get_local_path on the cleaned URL to compute
$path/$dir/$filename but pass $orig_url (and a normalized protocol-relative
variant) into replace_abs_urls() so protocol-relative URLs and ?v= query
variants are matched and replaced correctly; update calls to replace_abs_urls()
to reference $orig_url (and its protocol-normalized counterpart) rather than the
post-processed $url.
- Around line 250-267: The loop that trims $url_to_insert using strrpos allows
above-root traversal to strip past the origin (e.g. turning
https://cdn.example.com/... into invalid strings); modify the logic in the block
using $folder_depth and $url_to_insert so you clamp trimming at the source
origin: compute the origin from $source (e.g. scheme + '://' + host, or
parse_url($source) components) and stop the for-loop (or break) when
$url_to_insert equals the origin or when strrpos($url_to_insert, '/') falls
inside the origin boundary; ensure you also handle cases where strrpos returns
false to avoid further substr calls, then continue building $path with
ltrim($rel_url, './') and return origin + '/' + $path.
In `@src/Helper.php`:
- Around line 79-81: The concatenation of $type and $rel_path into $full_path
allows dot-segment escapes (e.g., "../") because wp_normalize_path doesn't
collapse “..”, so is_within_cache_dir is bypassed; fix by resolving dot-segments
before checking: build the candidate path, normalize it (wp_normalize_path),
then collapse dot-segments (or call realpath on the intended directory when
possible) to produce a canonical path and pass that to is_within_cache_dir;
update the code paths where $full_path is formed (the block using $full_path,
get_local_path, and is_within_cache_dir) so the check uses the
canonical/resolved path rather than a simple prefix check.
---
Nitpick comments:
In `@tests/bootstrap.php`:
- Around line 2-6: Update the PHPDoc `@package` annotation in the PHPUnit
bootstrap PHPDoc block by replacing the incorrect placeholder
"Host_Webfonts_Local" with the actual package name for this project (i.e., the
plugin’s real package identifier) so IDE metadata and generated docs are
accurate; locate the PHPDoc block at the top of tests/bootstrap.php and change
the `@package` value accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f566aeb8-ce56-46b9-b4d1-72261c50e0a6
📒 Files selected for processing (9)
bin/install-wp-tests.shcomposer.jsonphpunit-with-integration.xmlphpunit.xmlsrc/Admin/Settings/Builder.phpsrc/Admin/Settings/Manage.phpsrc/Download.phpsrc/Helper.phptests/bootstrap.php
✅ Files skipped from review due to trivial changes (2)
- phpunit.xml
- phpunit-with-integration.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- composer.json
- src/Admin/Settings/Builder.php
- src/Admin/Settings/Manage.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
vendor/composer/installed.php (1)
1-469: Right, well, this is an auto-generated Composer file, isn't it?Hello! adjusts glasses I see you're committing vendor files to the repository. Now, I'm not saying it's wrong — some projects do this intentionally for deployment purposes or when running in environments without Composer — but traditionally, vendor directories are listed in
.gitignoreand regenerated viacomposer install.If this is deliberate (perhaps for WordPress plugin distribution or similar), then carry on! The packages all look rather splendid — proper testing infrastructure with PHPUnit, Mockery, Brain Monkey, and coding standards tools. Very professional. nods approvingly
If it's NOT deliberate... well, have you tried turning it off and on again? By which I mean, consider adding
vendor/to your.gitignoreand using Composer for dependency management during deployment instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/composer/installed.php` around lines 1 - 469, The PR includes an auto-generated Composer artifact (vendor/composer/installed.php) which should not be committed; remove vendor/composer/installed.php from the commit (or revert the vendor/ folder changes), add vendor/ to .gitignore, commit that change, and then re-run Composer locally (composer install) to regenerate vendor when needed; if you intentionally vendorize for distribution, add a short note in the README explaining why and keep this file out of regular feature commits. Reference file: vendor/composer/installed.php and project root package entry 'daandev/gdpress' to locate the vendor commit.tests/integration/DownloadTest.php (1)
19-33: Consider using PHPUnit's data provider annotation!taps chin thoughtfully
Right, so you're manually iterating over the data provider results inside the test method. That works, but PHPUnit has a lovely built-in
@dataProviderannotation that would give you individual test results for each case. If one fails, you'd see exactly which one without the custom failure message.This is more of a stylistic preference, really. Your approach works. But the native way is rather elegant!
💡 Using `@dataProvider` annotation
/** * `@covers` \GDPRess\Download::get_abs_url + * `@dataProvider` get_abs_url_provider */ - public function test_get_abs_url() { + public function test_get_abs_url( $rel_url, $source, $expected ) { $method = new ReflectionMethod( Download::class, 'get_abs_url' ); $method->setAccessible( true ); - foreach ( $this->get_abs_url_provider() as $name => $data ) { - $this->assertEquals( $data[2], $method->invoke( $this->download, $data[0], $data[1] ), "Failed test: $name" ); - } + $this->assertEquals( $expected, $method->invoke( $this->download, $rel_url, $source ) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/DownloadTest.php` around lines 19 - 33, Replace the manual iteration and ReflectionMethod-based invocation in test_get_abs_url with PHPUnit's `@dataProvider` mechanism: change test_get_abs_url to accept the parameters provided by get_abs_url_provider, add the docblock annotation `@dataProvider` get_abs_url_provider, and inside the test assert the expected value against calling Download::get_abs_url (use the class instance $this->download or make the method accessible via Reflection only if strictly necessary); remove the foreach loop and the ReflectionMethod invocation so each provider case becomes its own PHPUnit test case.tests/TestCase.php (1)
17-33: A minor observation about ABSPATH, if I may...pushes glasses up nose
Right, so on line 22, you're defining
ABSPATHastrue. Now, in the actual WordPress environment,ABSPATHis the absolute path to the WordPress directory — a string, not a boolean. For the purpose of passingdefined('ABSPATH') || exit;guards, this works fine. However, if any test code tries to useABSPATHas an actual path (likeABSPATH . 'wp-admin/...'), you'll get1wp-admin/...becausetrueis cast to string"1".For BrainMonkey unit tests that mock WordPress functions, this is probably fine. But do keep it in mind!
💡 Optional: Use a proper path value
if ( ! defined( 'ABSPATH' ) ) { - define( 'ABSPATH', true ); + define( 'ABSPATH', dirname( __DIR__ ) . '/' ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/TestCase.php` around lines 17 - 33, The constructor in TestCase sets ABSPATH to boolean true which can break code that expects a filesystem path; change the ABSPATH define inside __construct to a realistic path string (e.g. use __DIR__ or GDPRESS_TESTS_ROOT to build an absolute path) instead of true so concatenation like ABSPATH . 'wp-admin/...' works; update the define( 'ABSPATH', ... ) call and ensure GDPRESS_TESTS_ROOT remains defined before using it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/DownloadTest.php`:
- Around line 87-92: The test relies on get_abs_url() parsing a
protocol-relative URL ('//example.com/css/style.css') where parse_url() returns
no scheme so the code falls back to get_site_url(), making the expected result
environment-dependent; update the test to be deterministic by either using a
fully-qualified URL with scheme (e.g., 'http://example.com/...') or by computing
the expected value from get_site_url() at runtime instead of hardcoding
'http://example.org', and ensure the test references get_abs_url() and the
branch that checks isset($parts['scheme'], $parts['host']) so it explicitly
verifies fallback behavior without assuming the WP test site's configured URL.
---
Nitpick comments:
In `@tests/integration/DownloadTest.php`:
- Around line 19-33: Replace the manual iteration and ReflectionMethod-based
invocation in test_get_abs_url with PHPUnit's `@dataProvider` mechanism: change
test_get_abs_url to accept the parameters provided by get_abs_url_provider, add
the docblock annotation `@dataProvider` get_abs_url_provider, and inside the test
assert the expected value against calling Download::get_abs_url (use the class
instance $this->download or make the method accessible via Reflection only if
strictly necessary); remove the foreach loop and the ReflectionMethod invocation
so each provider case becomes its own PHPUnit test case.
In `@tests/TestCase.php`:
- Around line 17-33: The constructor in TestCase sets ABSPATH to boolean true
which can break code that expects a filesystem path; change the ABSPATH define
inside __construct to a realistic path string (e.g. use __DIR__ or
GDPRESS_TESTS_ROOT to build an absolute path) instead of true so concatenation
like ABSPATH . 'wp-admin/...' works; update the define( 'ABSPATH', ... ) call
and ensure GDPRESS_TESTS_ROOT remains defined before using it.
In `@vendor/composer/installed.php`:
- Around line 1-469: The PR includes an auto-generated Composer artifact
(vendor/composer/installed.php) which should not be committed; remove
vendor/composer/installed.php from the commit (or revert the vendor/ folder
changes), add vendor/ to .gitignore, commit that change, and then re-run
Composer locally (composer install) to regenerate vendor when needed; if you
intentionally vendorize for distribution, add a short note in the README
explaining why and keep this file out of regular feature commits. Reference
file: vendor/composer/installed.php and project root package entry
'daandev/gdpress' to locate the vendor commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2248a79-017a-4b47-aa95-cce939733425
📒 Files selected for processing (14)
bin/install-wp-tests.shcomposer.jsonphpunit-with-integration.xmlphpunit.xmlsrc/Admin/Settings/Builder.phpsrc/Admin/Settings/Manage.phpsrc/Download.phpsrc/Helper.phptests/TestCase.phptests/bootstrap.phptests/integration/DownloadTest.phpvendor/composer/InstalledVersions.phpvendor/composer/autoload_static.phpvendor/composer/installed.php
✅ Files skipped from review due to trivial changes (2)
- phpunit.xml
- phpunit-with-integration.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/TestCase.php (1)
21-23: Oh! SettingABSPATHtotrueis a bit like using a fire extinguisher as a paperweight, isn't it?Right, so, here's the thing. In WordPress,
ABSPATHis supposed to be the absolute filesystem path to the WordPress installation — a string, you see. But you've set it to booleantrue. Now, if any code tries to do string concatenation likeABSPATH . 'wp-includes/...', you'll get1wp-includes/...which is... well, it's not going to work, is it?For unit tests that mock WordPress entirely, this might be fine. But if any code under test actually uses
ABSPATHas a path, you'll have a bit of a situation on your hands.🛠️ Possible fix
if ( ! defined( 'ABSPATH' ) ) { - define( 'ABSPATH', true ); + define( 'ABSPATH', __DIR__ . '/../' ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/TestCase.php` around lines 21 - 23, The ABSPATH constant in TestCase.php is incorrectly defined as boolean true; change it to a realistic filesystem path string so code that concatenates paths (e.g., using ABSPATH in tests or code under test) behaves correctly. Update the ABSPATH definition in TestCase.php to a string pointing to a suitable directory for tests (for example the project root, tests fixture dir, or system temp dir), ensuring trailing directory separator is included; locate the ABSPATH define in TestCase.php to make this change so functions/classes that rely on ABSPATH receive a proper path string.tests/integration/DownloadTest.php (1)
26-33: Just a thought — have you considered using PHPUnit's@dataProviderannotation?You see, currently you're manually iterating over the data provider arrays in your test methods. This works, but if one case fails, you won't know which specific case failed without reading the error message carefully.
With
@dataProvider, PHPUnit runs each case as a separate test, so you get nice individual pass/fail reporting. It's like having a separate postbox for each letter instead of one big sack.Not critical at all — your tests will work fine. Just a thought!
♻️ Example using `@dataProvider`
/** + * `@dataProvider` get_abs_url_provider * `@covers` \GDPRess\Download::get_abs_url */ - public function test_get_abs_url() { + public function test_get_abs_url( $rel_url, $source, $expected ) { $method = new ReflectionMethod( Download::class, 'get_abs_url' ); $method->setAccessible( true ); - foreach ( $this->get_abs_url_provider() as $name => $data ) { - $this->assertEquals( $data[2], $method->invoke( $this->download, $data[0], $data[1] ), "Failed test: $name" ); - } + $this->assertEquals( $expected, $method->invoke( $this->download, $rel_url, $source ) ); }Also applies to: 108-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/DownloadTest.php` around lines 26 - 33, Refactor the table-driven tests to use PHPUnit's `@dataProvider` instead of manually looping: convert test_get_abs_url() into a data-driven test that accepts parameters from get_abs_url_provider() (use the method name get_abs_url_provider as the provider), declare the provider via the `@dataProvider` annotation on the test method, and update the method signature to accept the input args and expected value so you can call the reflected get_abs_url on $this->download and assertEquals directly; apply the same change to the similar test referenced around lines 108-115 to give each case its own test report.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/DownloadTest.php`:
- Around line 19-21: The overridden setUp() in DownloadTest fails to call
parent::setUp(), so BrainMonkey's test harness isn't initialized; update the
setUp() method to invoke parent::setUp() (call parent::setUp() at the start of
the DownloadTest::setUp() method) before instantiating $this->download = new
Download() so the Yoast\WPTestUtils\BrainMonkey\TestCase initialization runs.
---
Nitpick comments:
In `@tests/integration/DownloadTest.php`:
- Around line 26-33: Refactor the table-driven tests to use PHPUnit's
`@dataProvider` instead of manually looping: convert test_get_abs_url() into a
data-driven test that accepts parameters from get_abs_url_provider() (use the
method name get_abs_url_provider as the provider), declare the provider via the
`@dataProvider` annotation on the test method, and update the method signature to
accept the input args and expected value so you can call the reflected
get_abs_url on $this->download and assertEquals directly; apply the same change
to the similar test referenced around lines 108-115 to give each case its own
test report.
In `@tests/TestCase.php`:
- Around line 21-23: The ABSPATH constant in TestCase.php is incorrectly defined
as boolean true; change it to a realistic filesystem path string so code that
concatenates paths (e.g., using ABSPATH in tests or code under test) behaves
correctly. Update the ABSPATH definition in TestCase.php to a string pointing to
a suitable directory for tests (for example the project root, tests fixture dir,
or system temp dir), ensuring trailing directory separator is included; locate
the ABSPATH define in TestCase.php to make this change so functions/classes that
rely on ABSPATH receive a proper path string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 421d0f7f-b747-41ed-83af-2091f23a604e
📒 Files selected for processing (14)
bin/install-wp-tests.shcomposer.jsonphpunit-with-integration.xmlphpunit.xmlsrc/Admin/Settings/Builder.phpsrc/Admin/Settings/Manage.phpsrc/Download.phpsrc/Helper.phptests/TestCase.phptests/bootstrap.phptests/integration/DownloadTest.phpvendor/composer/InstalledVersions.phpvendor/composer/autoload_static.phpvendor/composer/installed.php
✅ Files skipped from review due to trivial changes (2)
- phpunit.xml
- phpunit-with-integration.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Tests / CI