perf: Optimize geolocation with local MaxMind database and caching#361
perf: Optimize geolocation with local MaxMind database and caching#361superdav42 wants to merge 1 commit intomainfrom
Conversation
Performance improvements: - Use WooCommerce's bundled MaxMind database reader (eliminates external API calls) - Add in-memory static cache for same-request lookups - Add object cache integration for cross-request caching - Improve IP detection for CloudFlare, proxies, and load balancers - Reuse MaxMind Reader instance across lookups - Fall back gracefully when database is unavailable This eliminates the 1+ second delays caused by external HTTP calls to geolocation APIs (ipinfo.io, ip-api.com) on every page load. Key changes: - get_ip_address(): Now checks CF-Connecting-IP, True-Client-IP headers - geolocate_ip(): Uses multi-layer caching (memory -> object cache -> db) - geolocate_via_db(): Reuses MaxMind Reader, uses WooCommerce's database - get_external_ip_address(): Deprecated, no longer makes external calls - Added clear_cache() method for cache invalidation
📝 WalkthroughWalkthroughThe Geolocation class has been completely reimplemented to use local MaxMind database lookups instead of external API calls. The new version introduces in-request and persistent caching layers, request-scoped database reader management, robust IP extraction from HTTP headers, and graceful initialization and shutdown hooks while maintaining a minimal public API surface. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Request
participant GeolocService as Geolocation Service
participant Cache as Object Cache
participant DB as MaxMind DB Reader
Client->>GeolocService: geolocate_ip(ip_address)
activate GeolocService
GeolocService->>Cache: Check in-memory cache
alt Cache hit
Cache-->>GeolocService: Return cached result
else Cache miss
GeolocService->>GeolocService: Validate & normalize IP
GeolocService->>Cache: Check persistent cache
alt Persistent cache hit
Cache-->>GeolocService: Return cached result
else Persistent cache miss
GeolocService->>DB: Query MaxMind DB
activate DB
DB-->>GeolocService: Country code
deactivate DB
GeolocService->>Cache: Store in persistent cache (TTL: 24h)
end
GeolocService->>GeolocService: Store in in-memory cache
end
deactivate GeolocService
GeolocService-->>Client: Return geolocation array
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
inc/class-geolocation.php (1)
399-424: Remove or use these unused private methods.Static analysis correctly identifies that
supports_geolite2()andis_geolocation_enabled()are never called. Since they're private, they cannot be used externally.Options:
- Remove them if not needed
- Use them where appropriate (e.g.,
supports_geolite2()could guard the DB lookup ingeolocate_via_db())- If intended for future use, add a
@todoannotation explaining the plan♻️ Option 1: Remove unused methods
- /** - * Check if server supports MaxMind GeoLite2 Reader. - * - * `@return` bool - */ - private static function supports_geolite2(): bool - { - // Check if WooCommerce's MaxMind reader is available - if (class_exists('MaxMind\Db\Reader')) { - return true; - } - - // Check if WooCommerce is installed with the reader - if (defined('WC_ABSPATH')) { - $reader_path = WC_ABSPATH . 'vendor/maxmind-db/reader/src/MaxMind/Db/Reader.php'; - return file_exists($reader_path); - } - - return false; - } - - /** - * Check if geolocation is enabled. - * - * `@param` string $current_settings Current geolocation settings. - * `@return` bool - */ - private static function is_geolocation_enabled(string $current_settings): bool - { - return in_array($current_settings, ['geolocation', 'geolocation_ajax'], true); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inc/class-geolocation.php` around lines 399 - 424, These two private methods, supports_geolite2() and is_geolocation_enabled(), are never called; either remove them or wire them into existing logic: if you don't need them, delete both methods; otherwise call supports_geolite2() from geolocate_via_db() to guard MaxMind DB lookups instead of duplicating checks, and replace any ad-hoc geolocation setting checks with is_geolocation_enabled($current_settings) where the code currently tests for 'geolocation'/'geolocation_ajax'; if you intend to keep them for future work, add a `@todo` explaining planned usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inc/class-geolocation.php`:
- Around line 301-313: The code currently requires the MaxMind Reader file
($wc_reader) before loading the Composer autoloader ($autoload), which can cause
missing-dependency errors; change the order so that when WC_ABSPATH vendor files
exist you first check for and require_once the autoload file ($autoload) and
then require_once the Reader.php ($wc_reader) if still needed, preserving the
existing checks around defined('WC_ABSPATH') and file_exists and keeping the
class_exists('MaxMind\\Db\\Reader') guard.
- Around line 28-41: Move the opening brace for the class Geolocation to the
same line as the class declaration, replace space indentation with tabs inside
the class docblocks/members (affecting the docblock and constant declarations
around CACHE_GROUP and CACHE_TTL), and replace the hardcoded TTL value 86400
with the DAY_IN_SECONDS constant (i.e., set CACHE_TTL = DAY_IN_SECONDS) so
formatting and readability conform to the pipeline rules.
- Around line 242-246: The 'state' field is left empty in the $result array
returned by geolocate_via_db(); update geolocate_via_db() to extract the first
MaxMind subdivision ISO code when present (check
isset($data['subdivisions'][0]['iso_code'])), set $state =
strtoupper(sanitize_text_field(...)) and use that value for the 'state' key in
the $result array, falling back to an empty string if not available.
---
Nitpick comments:
In `@inc/class-geolocation.php`:
- Around line 399-424: These two private methods, supports_geolite2() and
is_geolocation_enabled(), are never called; either remove them or wire them into
existing logic: if you don't need them, delete both methods; otherwise call
supports_geolite2() from geolocate_via_db() to guard MaxMind DB lookups instead
of duplicating checks, and replace any ad-hoc geolocation setting checks with
is_geolocation_enabled($current_settings) where the code currently tests for
'geolocation'/'geolocation_ajax'; if you intend to keep them for future work,
add a `@todo` explaining planned usage.
| class Geolocation | ||
| { | ||
|
|
||
| /** | ||
| * Cache group for geolocation data. | ||
| */ | ||
| const CACHE_GROUP = 'wu_geolocation'; | ||
|
|
||
| /** | ||
| * Cache TTL in seconds (24 hours). | ||
| */ | ||
| const CACHE_TTL = 86400; // DAY_IN_SECONDS | ||
|
|
||
| /** |
There was a problem hiding this comment.
Fix formatting issues flagged by pipeline.
The code quality checks are failing due to formatting violations:
- Line 29: Opening brace should be on the same line as the class declaration
- Lines 31-41: Spaces are used for indentation instead of tabs
Additionally, consider using DAY_IN_SECONDS constant instead of the hardcoded 86400 for better readability.
🔧 Proposed fix
-class Geolocation
-{
-
- /**
- * Cache group for geolocation data.
- */
- const CACHE_GROUP = 'wu_geolocation';
-
- /**
- * Cache TTL in seconds (24 hours).
- */
- const CACHE_TTL = 86400; // DAY_IN_SECONDS
+class Geolocation {
+
+ /**
+ * Cache group for geolocation data.
+ */
+ const CACHE_GROUP = 'wu_geolocation';
+
+ /**
+ * Cache TTL in seconds (24 hours).
+ */
+ const CACHE_TTL = DAY_IN_SECONDS;🧰 Tools
🪛 GitHub Check: Code Quality Checks
[failure] 41-41:
Tabs must be used to indent lines; spaces are not allowed
[failure] 39-39:
Tabs must be used to indent lines; spaces are not allowed
[failure] 38-38:
Tabs must be used to indent lines; spaces are not allowed
[failure] 37-37:
Tabs must be used to indent lines; spaces are not allowed
[failure] 36-36:
Tabs must be used to indent lines; spaces are not allowed
[failure] 34-34:
Tabs must be used to indent lines; spaces are not allowed
[failure] 33-33:
Tabs must be used to indent lines; spaces are not allowed
[failure] 32-32:
Tabs must be used to indent lines; spaces are not allowed
[failure] 31-31:
Tabs must be used to indent lines; spaces are not allowed
[failure] 29-29:
Opening brace should be on the same line as the declaration for class Geolocation
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/class-geolocation.php` around lines 28 - 41, Move the opening brace for
the class Geolocation to the same line as the class declaration, replace space
indentation with tabs inside the class docblocks/members (affecting the docblock
and constant declarations around CACHE_GROUP and CACHE_TTL), and replace the
hardcoded TTL value 86400 with the DAY_IN_SECONDS constant (i.e., set CACHE_TTL
= DAY_IN_SECONDS) so formatting and readability conform to the pipeline rules.
| $result = [ | ||
| 'ip' => $ip_address, | ||
| 'country' => is_string($country_code) ? $country_code : '', | ||
| 'state' => '', | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how state data is used across the codebase
rg -n "ip_state|'state'" --type php -C 2 | head -80Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 4540
🏁 Script executed:
# Find class-customer.php and search for ip_state usage
fd -e php | xargs rg -l "ip_state" | head -20Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 189
🏁 Script executed:
# Search more broadly for ip_state in the codebase
rg "ip_state" -n --type php | head -40Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 342
🏁 Script executed:
# Find the geolocate_via_db method implementation
rg "geolocate_via_db" -n --type php -A 30 | head -80Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 4243
🏁 Script executed:
# Check for MaxMind API/database handling and subdivision data
rg "subdivisions|iso_code" -n --type php | head -30Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 290
🏁 Script executed:
rg "private static function geolocate_via_db" -A 50 --type php inc/class-geolocation.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 1955
🏁 Script executed:
# Also check the full MaxMind usage to see what data is available
rg "self::\$reader" -B 3 -A 15 --type php inc/class-geolocation.php | head -80Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 63
State field is always empty and should be populated from MaxMind subdivision data.
The 'state' field is hardcoded to an empty string, but it's actively used throughout the codebase:
- Stored in customer metadata (
class-customer.phpline 558) - Displayed in the admin interface (
class-customer-edit-admin-page.phpline 1264) - Used for filtering by state (
countries.phpline 492)
The geolocate_via_db() method retrieves the full MaxMind data but only extracts the country code. Extract the subdivision/state code by accessing $data['subdivisions'][0]['iso_code'] when available and return it in the result array:
$state = '';
if (isset($data['subdivisions'][0]['iso_code'])) {
$state = strtoupper(\sanitize_text_field($data['subdivisions'][0]['iso_code']));
}Then populate the 'state' field in the result array instead of hardcoding an empty string.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/class-geolocation.php` around lines 242 - 246, The 'state' field is left
empty in the $result array returned by geolocate_via_db(); update
geolocate_via_db() to extract the first MaxMind subdivision ISO code when
present (check isset($data['subdivisions'][0]['iso_code'])), set $state =
strtoupper(sanitize_text_field(...)) and use that value for the 'state' key in
the $result array, falling back to an empty string if not available.
| if (!class_exists('MaxMind\Db\Reader')) { | ||
| if (defined('WC_ABSPATH')) { | ||
| $wc_reader = WC_ABSPATH . 'vendor/maxmind-db/reader/src/MaxMind/Db/Reader.php'; | ||
| if (file_exists($wc_reader)) { | ||
| require_once $wc_reader; | ||
| // Also need to load the dependencies | ||
| $autoload = WC_ABSPATH . 'vendor/autoload.php'; | ||
| if (file_exists($autoload)) { | ||
| require_once $autoload; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Autoload order may cause issues - load autoload.php first.
The current order requires Reader.php directly (line 305) before loading autoload.php (lines 307-310). The MaxMind Reader class has dependencies (like InvalidDatabaseException) that need to be autoloaded. If those aren't available when Reader.php is parsed, it could cause a fatal error.
🐛 Proposed fix - load autoload first
if (defined('WC_ABSPATH')) {
- $wc_reader = WC_ABSPATH . 'vendor/maxmind-db/reader/src/MaxMind/Db/Reader.php';
- if (file_exists($wc_reader)) {
- require_once $wc_reader;
- // Also need to load the dependencies
- $autoload = WC_ABSPATH . 'vendor/autoload.php';
- if (file_exists($autoload)) {
- require_once $autoload;
- }
+ $autoload = WC_ABSPATH . 'vendor/autoload.php';
+ if (file_exists($autoload)) {
+ require_once $autoload;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/class-geolocation.php` around lines 301 - 313, The code currently
requires the MaxMind Reader file ($wc_reader) before loading the Composer
autoloader ($autoload), which can cause missing-dependency errors; change the
order so that when WC_ABSPATH vendor files exist you first check for and
require_once the autoload file ($autoload) and then require_once the Reader.php
($wc_reader) if still needed, preserving the existing checks around
defined('WC_ABSPATH') and file_exists and keeping the
class_exists('MaxMind\\Db\\Reader') guard.
1 participant
Summary
This PR dramatically improves geolocation performance by eliminating external HTTP calls that were causing 1+ second delays on every page load.
Problem
The existing geolocation implementation made external HTTP calls to services like
ipinfo.ioandip-api.comon every request. SPX profiling showed:WP_Ultimo\Geolocation::geolocate_ip- 1,117ms (2 external API calls)WP_Ultimo\Geolocation::get_external_ip_address- 645ms (external IP lookup)Solution
1. Use Local MaxMind Database
2. Multi-Layer Caching
3. Improved IP Detection
Headers are now checked in order of trust:
CF-Connecting-IP(Cloudflare)True-Client-IP(Cloudflare Enterprise / Akamai)X-Real-IP(Nginx proxy)X-Forwarded-For(Standard proxy - first IP only)REMOTE_ADDR(Direct connection)4. Performance Optimizations
HTTP_CF_IPCOUNTRY) is used when availableBreaking Changes
get_external_ip_address()no longer makes external API calls (kept for backwards compatibility)wu_geolocation_ip_lookup_apisfilter returning non-empty array)Testing
Performance Impact
Summary by CodeRabbit
Release Notes
New Features
Performance
Bug Fixes