fix(dbip): include file.php before wp_tempnam() in cron context#183
Merged
parhumm merged 4 commits intodevelopmentfrom Mar 13, 2026
Merged
fix(dbip): include file.php before wp_tempnam() in cron context#183parhumm merged 4 commits intodevelopmentfrom
parhumm merged 4 commits intodevelopmentfrom
Conversation
Add ensureWpFileApiLoaded() to AbstractGeoIPProvider and use it in both DbIpProvider and MaxmindGeoIPProvider. Broaden catch blocks from \Exception to \Throwable in cron callback and admin AJAX handlers to prevent PHP Error instances from fataling requests. Add E2E regression tests verifying the include guard in non-admin context and \Throwable catch behavior. Closes #180
📝 WalkthroughWalkthroughBroadened error handling to catch Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as WP-Cron
participant Service as GeolocationService
participant Provider as DbIpProvider / MaxmindProvider
participant WPFiles as WP File API
participant Remote as Remote DB Host
Cron->>Service: trigger updateDatabase()
Service->>Provider: updateDatabase()
Provider->>WPFiles: ensureWpFileApiLoaded() %% rgba(50,150,250,0.5)
WPFiles-->>Provider: wp_tempnam / WP_Filesystem available
Provider->>Remote: download DB archive
Remote-->>Provider: DB stream
Provider->>WPFiles: write DB via WP_Filesystem
Provider-->>Service: success or throw (caught as Throwable)
Service-->>Cron: complete (errors logged/handled)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cookie-law-info and other plugins may load wp-admin/includes/file.php on frontend requests. Use test.skip() instead of hard-fail so the include guard test is inconclusive rather than falsely failing.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wp-slimstat.php (1)
1265-1275:⚠️ Potential issue | 🟠 MajorMove
GeolocationServiceconstruction inside thetry.
catch ( \Throwable )only protectsupdateDatabase()right now. If\SlimStat\Services\Geolocation\GeolocationService::__construct()or provider initialization throws, the cron callback still fatals before it reaches the new handler.Suggested fix
- $geographicProvider = new \SlimStat\Services\Geolocation\GeolocationService($provider, []); - try { + $geographicProvider = new \SlimStat\Services\Geolocation\GeolocationService($provider, []); $geographicProvider->updateDatabase(); // Set the last update time update_option('slimstat_last_geoip_dl', time());Based on learnings: Use try/catch for risky code; never fatal error in admin or front-end; gracefully handle failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wp-slimstat.php` around lines 1265 - 1275, The GeolocationService instance creation is currently outside the try/catch so constructor exceptions aren't caught; move the construction of \SlimStat\Services\Geolocation\GeolocationService($provider, []) into the try block that calls $geographicProvider->updateDatabase(), and only call update_option('slimstat_last_geoip_dl', time()) on successful update; keep the catch(\Throwable $e) and use wp_slimstat::log('Geolocation database update failed: ' . $e->getMessage(), 'error') to record any construction or update errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin/index.php`:
- Around line 2103-2105: Replace returning the raw throwable message in the
catch block by logging the exception server-side and responding with a generic
localized failure string; specifically, in the catch (\Throwable $exception)
block where wp_send_json_error($exception->getMessage()) is used, call your
logging mechanism (error_log or the plugin’s logger) with the full $exception
(message, file, line, stack) and then call wp_send_json_error( __('An error
occurred while processing your request.', 'your-text-domain') ) or similar
localized, non-sensitive text; make the same change for the other catch block
that also uses wp_send_json_error with the exception message.
In `@tests/e2e/helpers/cron-frontend-shim-mu-plugin.php`:
- Around line 13-83: This MU-plugin exposes public test endpoints and must be
no-op unless the site is explicitly in the E2E test environment; add a single
early guard at the top of the file that returns immediately unless a dedicated
test flag is present (for example check defined('SLIMSTAT_E2E_TESTING') &&
SLIMSTAT_E2E_TESTING === true or an environment variable like getenv('E2E') ===
'1'). Ensure the guard runs before any add_action calls so the handlers for
wp_ajax_test_get_geoip_nonce, the init pre_http_request injection, and the
template_redirect test_dbip_cron logic never get registered on non-test sites.
In `@tests/e2e/helpers/setup.ts`:
- Around line 207-217: The restoreGeoipTimestamp helper currently hardcodes the
wp_options table and sets autoload='no', which breaks prefixed tables, multisite
blog tables, and preserves only option_value; change restoreGeoipTimestamp (and
its companion clearGeoipTimestamp) to restore the entire saved row instead of
inserting a fixed row: capture and persist the full savedGeoipRow (including
option_name, option_value, autoload and the actual table/prefix or blog-specific
table) and on restore use that saved row to perform a WordPress-level write or a
parametrized INSERT/UPDATE against the original table name/prefix (or call the
WP option API if available) so the original autoload and table placement are
preserved; continue to set savedGeoipTimestamp/row back to undefined after
restore.
In `@tests/e2e/issue-180-dbip-cron-tempnam.spec.ts`:
- Around line 50-61: The test currently only checks that wp_tempnam wasn't
preloaded and that no error mentions "wp_tempnam" or "undefined function", but
it never asserts the provider path actually succeeded; update the test to assert
the positive success flag by adding an assertion on body.success (e.g.,
expect(body.success).toBe(true) or truthy) after the precondition check (where
variable body is available) and before/alongside the existing error checks so
the test fails if the provider returned a non-success result like
"provider_disabled".
---
Outside diff comments:
In `@wp-slimstat.php`:
- Around line 1265-1275: The GeolocationService instance creation is currently
outside the try/catch so constructor exceptions aren't caught; move the
construction of \SlimStat\Services\Geolocation\GeolocationService($provider, [])
into the try block that calls $geographicProvider->updateDatabase(), and only
call update_option('slimstat_last_geoip_dl', time()) on successful update; keep
the catch(\Throwable $e) and use wp_slimstat::log('Geolocation database update
failed: ' . $e->getMessage(), 'error') to record any construction or update
errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4802db14-6921-4e45-9778-3cd36d777fba
📒 Files selected for processing (8)
admin/index.phpsrc/Services/Geolocation/AbstractGeoIPProvider.phpsrc/Services/Geolocation/Provider/DbIpProvider.phpsrc/Services/Geolocation/Provider/MaxmindGeoIPProvider.phptests/e2e/helpers/cron-frontend-shim-mu-plugin.phptests/e2e/helpers/setup.tstests/e2e/issue-180-dbip-cron-tempnam.spec.tswp-slimstat.php
Contributor
Author
E2E Test Results — Issue #180 RegressionEnvironment: Local by Flywheel, PHP 8.4, WordPress (latest), WP SlimStat Issue-180 Specific Tests
Summary
Changes Verified
|
Contributor
Author
Full E2E Suite Results54 passed, 1 skipped, 0 failed (12.1m) The only skip is Test 1 from the new issue-180 spec (cookie-law-info pre-loads file.php on frontend). All 54 other tests — including existing tracking, consent, transport, upgrade safety, visit ID, and geolocation specs — pass without regression. No changes needed. |
…elpers - admin/index.php: log throwable server-side, return generic localized message - wp-slimstat.php: move GeolocationService constructor inside try/catch - cron-frontend-shim: add SLIMSTAT_E2E_TESTING guard for safety - setup.ts: extract injectWpConfigLine() shared helper, preserve autoload - spec: assert positive success in Test 1, stronger oracle in Test 3
- Add wporg-support-issues.spec.ts: 6 tests verifying soolee (GDPR/IP), kindnessville (geolocation Unknown), and admin stability - Fix issue-180 Test 1: deactivate cookie-law-info via option_active_plugins filter so file.php isn't pre-loaded in provider test - Fix geoip-ajax-loop flakes: increase timeouts, use domcontentloaded, relax Test 2 threshold to ≤2 for slow servers - Add getLatestStatWithIp helper for IP column assertions
Contributor
Author
E2E QA Report: wp.org Support Issues Verification61/61 tests passed (0 failed, 0 skipped) — 13.5 min on Local by Flywheel Support Issues Status
New Tests Added
Issue-180 Tests (all passing, none skipped)
Additional Fixes
Full Suite Results
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/e2e/helpers/cron-frontend-shim-mu-plugin.php (2)
1-13: Add the standard GPL header to this new PHP helper.This is a new PHP file, but it only has a descriptive block right now. Please prepend the repository’s GPLv2+ header here.
As per coding guidelines, "Add GPL license headers to all new PHP files; ensure all code/assets are GPLv2+ or GPL-compatible".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpers/cron-frontend-shim-mu-plugin.php` around lines 1 - 13, Prepend the repository’s standard GPLv2+ header to the very top of the new PHP file (cron-frontend-shim-mu-plugin.php) before the existing <?php tag so the file includes the required license boilerplate; keep the existing descriptive block and code intact (the SLIMSTAT_E2E_TESTING guard and any handlers) and ensure the header explicitly states GPLv2 or later (matching the project’s standard wording and copyright/year/owner placeholders).
81-99: Return a sentinel that proves the injected\Erroractually ran.
wp_slimstat::wp_slimstat_update_geoip_database()already swallows its own\Throwable, so this branch also reports success when the callback exits before the remote-request path. Without a flag from thepre_http_requestfilter, the cron regression test can false-pass without ever exercising the new catch block.Suggested change
} elseif ($mode === 'callback_throwable') { + $threw_simulated_error = false; // Test 2: Cron callback with forced \Error — tests \Throwable catch // Stub HTTP to throw \Error (not \Exception) — simulates engine-level failure - add_filter('pre_http_request', function () { + add_filter('pre_http_request', function () use (&$threw_simulated_error) { + $threw_simulated_error = true; throw new \Error('test_simulated_php_error'); }, 1); try { \wp_slimstat::wp_slimstat_update_geoip_database(); - echo json_encode(['success' => true, 'error' => null, - 'escaped_catch' => false, + echo json_encode(['success' => $threw_simulated_error, 'error' => null, + 'threw_simulated_error' => $threw_simulated_error, + 'escaped_catch' => false, 'had_wp_tempnam_before' => $had_wp_tempnam_before]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpers/cron-frontend-shim-mu-plugin.php` around lines 81 - 99, The pre_http_request filter injection should set a visible sentinel before throwing so the test can prove the injected \Error ran: inside the add_filter callback used in the 'callback_throwable' branch (the anonymous function passed to add_filter), assign a clear marker (e.g. $GLOBALS['pre_http_request_sentinel'] = 'callback_throwable_executed') and then throw the \Error as currently done; leave the throw in place so wp_slimstat::wp_slimstat_update_geoip_database() still receives the \Error, and the surrounding catch/echo logic can read $GLOBALS['pre_http_request_sentinel'] to determine the callback actually executed.tests/e2e/geoip-ajax-loop.spec.ts (1)
60-75: Keep Test 2 proving suppression on the second load.Allowing
log.length <= 2makes this pass even when the second admin load schedules its own fallback because the first request has not settled yet. That turns the check into “bounded by page loads,” which Test 3 already covers. Prefer waiting until the first run has settled, then keep the bound at1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/geoip-ajax-loop.spec.ts` around lines 60 - 75, Ensure the test truly verifies suppression on the second load by waiting for the first AJAX to finish before navigating to the second page and then asserting at most one AJAX call; replace the heuristic waits around page.goto('/wp-admin/', { waitUntil: 'domcontentloaded' }) and page.waitForTimeout(8000) with a deterministic wait for the first run to settle (for example poll for the presence/value that indicates completion such as slimstat_last_geoip_dl or use a longer explicit wait) and then keep the final assertion on readAjaxLog() as expect(log.length).toBeLessThanOrEqual(1) so Test 2 only passes when the second load is actually suppressed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/geoip-ajax-loop.spec.ts`:
- Around line 60-75: Ensure the test truly verifies suppression on the second
load by waiting for the first AJAX to finish before navigating to the second
page and then asserting at most one AJAX call; replace the heuristic waits
around page.goto('/wp-admin/', { waitUntil: 'domcontentloaded' }) and
page.waitForTimeout(8000) with a deterministic wait for the first run to settle
(for example poll for the presence/value that indicates completion such as
slimstat_last_geoip_dl or use a longer explicit wait) and then keep the final
assertion on readAjaxLog() as expect(log.length).toBeLessThanOrEqual(1) so Test
2 only passes when the second load is actually suppressed.
In `@tests/e2e/helpers/cron-frontend-shim-mu-plugin.php`:
- Around line 1-13: Prepend the repository’s standard GPLv2+ header to the very
top of the new PHP file (cron-frontend-shim-mu-plugin.php) before the existing
<?php tag so the file includes the required license boilerplate; keep the
existing descriptive block and code intact (the SLIMSTAT_E2E_TESTING guard and
any handlers) and ensure the header explicitly states GPLv2 or later (matching
the project’s standard wording and copyright/year/owner placeholders).
- Around line 81-99: The pre_http_request filter injection should set a visible
sentinel before throwing so the test can prove the injected \Error ran: inside
the add_filter callback used in the 'callback_throwable' branch (the anonymous
function passed to add_filter), assign a clear marker (e.g.
$GLOBALS['pre_http_request_sentinel'] = 'callback_throwable_executed') and then
throw the \Error as currently done; leave the throw in place so
wp_slimstat::wp_slimstat_update_geoip_database() still receives the \Error, and
the surrounding catch/echo logic can read $GLOBALS['pre_http_request_sentinel']
to determine the callback actually executed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d3fa485-2de3-4748-955a-62f68f1a5064
📒 Files selected for processing (4)
tests/e2e/geoip-ajax-loop.spec.tstests/e2e/helpers/cron-frontend-shim-mu-plugin.phptests/e2e/helpers/setup.tstests/e2e/wporg-support-issues.spec.ts
parhumm
commented
Mar 13, 2026
| $geographicProvider->updateDatabase(); | ||
|
|
||
| // Set the last update time | ||
| update_option('slimstat_last_geoip_dl', time()); |
Contributor
Author
There was a problem hiding this comment.
High risk: GeolocationService::updateDatabase() returns false on download/setup failures, but this cron path still updates slimstat_last_geoip_dl unconditionally. That means a transient network/filesystem failure marks the GeoIP DB as current and suppresses retries until the next monthly window, leaving the site on a stale database after a failed run. The admin AJAX path already avoids this by only updating the timestamp when $ok is truthy; the cron callback should do the same.
3 tasks
6 tasks
Merged
4 tasks
Merged
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
ensureWpFileApiLoaded()toAbstractGeoIPProvider— loadswp-admin/includes/file.phpwhenwp_tempnam()is undefined. BothDbIpProviderandMaxmindGeoIPProvidernow use this shared method instead of inline guards.catch (\Exception)→catch (\Throwable)in the cron callback (wp-slimstat.php:1273) and admin AJAX handlers (admin/index.php:2103, 2132) to prevent PHPErrorinstances from fataling requests.template_redirectshim (non-admin context) with HTTP stubbing:wp_tempnam()is available\Erroris caught by\Throwablecatch\Errorreturns JSON error with sentinel, not fatalCloses #180
Test plan
issue-180-dbip-cron-tempnam.spec.tspasses all 3 testsdbip, confirm no PHP fatal in error log after admin page load🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests