Fix enhance api requests #309
Conversation
📝 WalkthroughWalkthroughMoves dependency/hook loading behind the provider setup check; standardizes Closte API responses to arrays; updates Enhance provider to use organization- and website-scoped endpoints/fields; adds Gridpane configuration constants; adds defensive WPE_API availability checks in WP Engine provider. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🧹 Nitpick comments (1)
inc/integrations/host-providers/class-closte-host-provider.php (1)
108-108: Fix the domain error comparison.The comparison is reversed—it should compare the response error against the expected string, not the other way around. While functionally equivalent in this case, the current Yoda condition reduces readability.
🔎 Suggested fix
- } elseif (isset($domain_response['error']) && 'Invalid or empty domain: ' . $domain === $domain_response['error']) { + } elseif (isset($domain_response['error']) && $domain_response['error'] === 'Invalid or empty domain: ' . $domain) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
inc/integrations/host-providers/class-base-host-provider.phpinc/integrations/host-providers/class-closte-host-provider.phpinc/integrations/host-providers/class-enhance-host-provider.phpinc/integrations/host-providers/class-gridpane-host-provider.phpinc/integrations/host-providers/class-wpengine-host-provider.php
🧰 Additional context used
🧬 Code graph analysis (3)
inc/integrations/host-providers/class-enhance-host-provider.php (1)
inc/functions/helper.php (1)
wu_get_isset(66-73)
inc/integrations/host-providers/class-closte-host-provider.php (1)
inc/functions/helper.php (1)
wu_log_add(208-211)
inc/integrations/host-providers/class-base-host-provider.php (2)
inc/integrations/host-providers/class-wpengine-host-provider.php (1)
load_dependencies(89-97)inc/class-sunrise.php (1)
load_dependencies(106-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Plugin for Testing
- GitHub Check: cypress (8.1, chrome)
- GitHub Check: cypress (8.2, chrome)
🔇 Additional comments (9)
inc/integrations/host-providers/class-base-host-provider.php (1)
103-113: LGTM! Improved setup flow logic.Moving dependency loading and hook registration into the
is_setup()guard ensures they only execute when the provider is properly configured. This prevents potential issues from loading dependencies or registering hooks when required constants are missing.inc/integrations/host-providers/class-gridpane-host-provider.php (1)
65-67: LGTM! Constants list now matches actual usage.These constants are already referenced in the implementation (lines 116, 148-149, 168-169), so adding them to the required constants list ensures proper setup validation. This prevents runtime errors when these constants are undefined.
inc/integrations/host-providers/class-wpengine-host-provider.php (3)
108-110: LGTM! Defensive guard against missing WPE_API class.This check prevents fatal errors if
load_dependencies()fails to load the WPE_API class (e.g., whenWPE_PLUGIN_DIRis undefined or the file is unreadable). The early return gracefully handles the missing dependency.
129-131: LGTM! Consistent defensive guard.Applies the same protection as in
on_add_domain, ensuring safe operation when the WPE_API class is unavailable.
205-207: LGTM! Improved error reporting for test connection.Unlike the domain methods that silently return,
test_connectionappropriately provides user feedback via JSON error response when the WPE_API class is missing.inc/integrations/host-providers/class-closte-host-provider.php (1)
218-218: LGTM! Standardized return types to arrays.Consolidating return values to consistently use arrays improves error handling predictability and makes the API easier to consume. The docblock updates accurately reflect the implementation.
Also applies to: 265-265, 271-274, 279-282, 325-329, 347-351, 355-358
inc/integrations/host-providers/class-enhance-host-provider.php (3)
103-103: LGTM! Improved UX for API token field.Adding
autocomplete="new-password"prevents browsers from auto-filling the API token field with saved passwords, which is appropriate for bearer tokens.
181-187: LGTM! Domain lookup updated for new API response shape.The field name changes (
domain→domainName,id→domainId) are consistent with the response field change inon_add_domain. The logic correctly handles cases where the domain is not found.
262-263: LGTM! Improved error handling and documentation.The error message now includes the actual API error from the response, and the docblock accurately reflects that
$datacan be either an array or a string (as demonstrated by the domain addition changes).Also applies to: 272-274
inc/integrations/host-providers/class-enhance-host-provider.php
Outdated
Show resolved
Hide resolved
inc/integrations/host-providers/class-enhance-host-provider.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @inc/integrations/host-providers/class-enhance-host-provider.php:
- Around line 303-308: The docblock for send_enhance_api_request incorrectly
lists $data as array|string while the implementation always JSON-encodes the
payload (json_encode used when preparing the request), which would double-encode
if a JSON string were passed; fix by updating the docblock to @param array $data
to reflect actual usage (or alternatively implement conditional encoding if
string support is desired), and ensure the function signature and
examples/comments consistently treat $data as an array.
🧹 Nitpick comments (1)
inc/integrations/host-providers/class-enhance-host-provider.php (1)
286-296: Improve error diagnostics for failed connection tests.When the connection test fails (no
idin response), the error message defaults to'Unknown error'if theerrorkey is missing. However, thesend_enhance_api_requestmethod returns structured error data with keys likeresponse_codeandresponse_body. Consider usingwp_json_encode($response)or accessing these fields to provide more detailed diagnostic information.♻️ Enhanced error message
} else { // Translators: %s the full error message. - $error = new \WP_Error('connection-failed', sprintf(__('Failed to connect to Enhance API: %s', 'ultimate-multisite'), $response['error'] ?? 'Unknown error')); + $error = new \WP_Error('connection-failed', sprintf(__('Failed to connect to Enhance API: %s', 'ultimate-multisite'), $response['error'] ?? wp_json_encode($response))); wp_send_json_error($error); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
inc/integrations/host-providers/class-enhance-host-provider.php
🧰 Additional context used
🧬 Code graph analysis (1)
inc/integrations/host-providers/class-enhance-host-provider.php (1)
inc/functions/helper.php (2)
wu_log_add(208-211)wu_get_isset(66-73)
🔇 Additional comments (5)
inc/integrations/host-providers/class-enhance-host-provider.php (5)
65-70: LGTM! API scope migration properly reflected.The migration from server-based to organization/website-based constants aligns with the Enhance API endpoint changes throughout the file.
102-120: LGTM! Security improvement with autocomplete attribute.The addition of
autocomplete='new-password'for the API token field prevents browsers from inadvertently auto-filling sensitive credentials, which is a good security practice.
135-146: LGTM! Proper validation with informative logging.The validation checks for both
org_idandwebsite_idwith descriptive log messages provide clear diagnostics if the configuration is incomplete.
148-168: LGTM! Robust success detection.The dual success condition checking for both
idandsuccessflag provides flexibility in handling different API response formats.
182-225: LGTM! Correct two-step deletion flow.The implementation correctly lists domains first to obtain the domain ID before deletion, with appropriate handling for the case where the domain may already be removed.
| * @param string $endpoint API endpoint (relative to base URL). | ||
| * @param string $method HTTP method (GET, POST, DELETE, etc.). | ||
| * @param array|string $data Request body data (for POST/PUT/PATCH). | ||
| * @return array|object | ||
| */ | ||
| public function send_enhance_api_request($endpoint, $method = 'GET', $data = []) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to send_enhance_api_request with potential string arguments
# First, let's see all calls to this method
rg -n 'send_enhance_api_request\s*\(' --type=php
echo "---"
# Search specifically for calls with string literals
rg -n "send_enhance_api_request\s*\([^)]*['\"]" --type=phpRepository: Multisite-Ultimate/ultimate-multisite
Length of output: 847
🏁 Script executed:
# Also check the implementation around the encoding line
cat -n inc/integrations/host-providers/class-enhance-host-provider.php | sed -n '335,350p'Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 692
🏁 Script executed:
# Let's check what arguments are actually passed in each call
rg -n -A 5 'send_enhance_api_request\(' inc/integrations/host-providers/class-enhance-host-provider.php | head -50Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 1184
🏁 Script executed:
# Also check the docblock definition to confirm the @param line
cat -n inc/integrations/host-providers/class-enhance-host-provider.php | sed -n '300,310p'Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 660
Resolve docblock/implementation mismatch for $data parameter.
The docblock indicates @param array|string $data (line 305), but the implementation always JSON-encodes the data (line 341), which would double-encode if a JSON string is passed. The function signature defaults to an empty array ($data = []).
Current usage in this file and throughout the codebase only passes arrays as the $data argument. Either:
- Revert the docblock to
@param array $datato match the actual implementation, or - Add conditional encoding to handle pre-encoded string inputs properly
Reverting the docblock is the appropriate fix unless string support is intentionally planned.
🤖 Prompt for AI Agents
In @inc/integrations/host-providers/class-enhance-host-provider.php around lines
303 - 308, The docblock for send_enhance_api_request incorrectly lists $data as
array|string while the implementation always JSON-encodes the payload
(json_encode used when preparing the request), which would double-encode if a
JSON string were passed; fix by updating the docblock to @param array $data to
reflect actual usage (or alternatively implement conditional encoding if string
support is desired), and ensure the function signature and examples/comments
consistently treat $data as an array.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
inc/integrations/host-providers/class-enhance-host-provider.php (2)
192-239: Well-implemented domain removal with proper lookup logic.The implementation correctly handles the two-step process (list domains → find ID → delete), with appropriate error handling when the domain is not found.
♻️ Optional: Extract validation logic to reduce duplication
The org_id/website_id validation pattern is repeated across
on_add_domain,on_remove_domain, andtest_connection. Consider extracting to a private helper method:/** * Validates and retrieves org and website IDs. * * @return array|null Returns ['org_id' => string, 'website_id' => string] or null if invalid. */ private function get_validated_ids() { $org_id = defined('WU_ENHANCE_ORG_ID') ? WU_ENHANCE_ORG_ID : ''; $website_id = defined('WU_ENHANCE_WEBSITE_ID') ? WU_ENHANCE_WEBSITE_ID : ''; if (empty($org_id)) { wu_log_add('integration-enhance', 'Organization ID not configured'); return null; } if (empty($website_id)) { wu_log_add('integration-enhance', 'Website ID not configured'); return null; } return [ 'org_id' => $org_id, 'website_id' => $website_id, ]; }Then use it in methods:
$ids = $this->get_validated_ids(); if ($ids === null) { return; } $org_id = $ids['org_id']; $website_id = $ids['website_id'];
277-311: Test connection logic is sound.Properly validates configuration before attempting API connectivity test, and uses an appropriate endpoint to verify access to the specified organization and website.
Consider including the HTTP response code in the error message for easier troubleshooting:
} else { // Translators: %s the full error message. - $error = new \WP_Error('connection-failed', sprintf(__('Failed to connect to Enhance API: %s', 'ultimate-multisite'), $response['error'] ?? 'Unknown error')); + $error_msg = $response['error'] ?? 'Unknown error'; + if (isset($response['response_code'])) { + $error_msg .= sprintf(' (HTTP %d)', $response['response_code']); + } + $error = new \WP_Error('connection-failed', sprintf(__('Failed to connect to Enhance API: %s', 'ultimate-multisite'), $error_msg)); wp_send_json_error($error); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
inc/integrations/host-providers/class-enhance-host-provider.php
🧰 Additional context used
🧬 Code graph analysis (1)
inc/integrations/host-providers/class-enhance-host-provider.php (1)
inc/functions/helper.php (2)
wu_log_add(208-211)wu_get_isset(66-73)
🔇 Additional comments (4)
inc/integrations/host-providers/class-enhance-host-provider.php (4)
65-70: LGTM! Clean migration to organization/website-scoped configuration.The replacement of
WU_ENHANCE_SERVER_IDwithWU_ENHANCE_ORG_IDandWU_ENHANCE_WEBSITE_IDaligns well with the new API endpoint structure used throughout the file.
102-134: Excellent UX improvements in field definitions.The enhanced field configuration provides clear guidance for users:
- Security best practice with
autocomplete='new-password'prevents credential leakage- Documentation links help users understand where to find credentials
- UUID format examples in placeholders and descriptions guide correct input
- HTML
idattributes enable better frontend integration
145-182: Solid implementation with good defensive coding.The domain addition flow properly validates required configuration, uses the new org/website-scoped endpoint, and handles multiple response formats gracefully. The detailed logging will aid troubleshooting.
317-322: Change the$dataparameter type fromarray|stringtoarrayonly.All calls to this function pass arrays (line 168) or omit the parameter entirely (lines 211, 233, 296), using the default
[]. The implementation useswp_json_encode($data), which would double-encode a string input. The docblock incorrectly suggests strings are supported; they are not.
🔨 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: |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
inc/integrations/host-providers/class-closte-host-provider.php (2)
214-249: Initialize$ssl_responsebefore the loop.The variable
$ssl_response(line 224) is assigned within the loop but used afterward (line 239) without prior initialization. While the$ssl_endpointsarray currently contains entries guaranteeing loop execution, this creates a potential undefined variable risk if the array were ever empty or modified.🛡️ Defensive initialization
$ssl_endpoints = [ '/ssl/install', '/installssl', '/ssl', '/certificate/install', ]; + $ssl_response = ['success' => false, 'error' => 'No SSL endpoints attempted']; + foreach ($ssl_endpoints as $endpoint) {
305-352: Fix return type inconsistency: WP_Error object violates array return type.Line 352 returns a
WP_Errorobject when the HTTP request fails, but the docblock (line 303) specifies the function returnsarray. This type inconsistency could cause runtime errors for callers expecting array access.🐛 Fix to return array consistently
// Log response details if (is_wp_error($response)) { wu_log_add('integration-closte', sprintf('API request failed: %s', $response->get_error_message())); - return $response; + return [ + 'success' => false, + 'error' => $response->get_error_message(), + ]; }
🧹 Nitpick comments (1)
inc/integrations/host-providers/class-closte-host-provider.php (1)
146-156: Consider consolidating duplicate domain validation checks.Lines 146 and 152 both check for domain validation errors using slightly different approaches:
- Line 146: Exact match comparison
- Line 152: Substring check with
str_containsWhile the logic appears correct, having two similar checks for the same error pattern could be confusing.
♻️ Potential consolidation
Consider extracting the domain validation check into a helper method for clarity:
private function is_invalid_domain_error($response, $domain) { return isset($response['error']) && str_contains($response['error'], 'Invalid or empty domain'); }Then simplify the conditional logic:
- } elseif (isset($domain_response['error']) && 'Invalid or empty domain: ' . $domain === $domain_response['error']) { + } elseif ($this->is_invalid_domain_error($domain_response, $domain)) { wu_log_add('integration-closte', sprintf('Domain %s rejected by Closte API as invalid. This may be expected for Closte subdomains or internal domains.', $domain)); } else { wu_log_add('integration-closte', sprintf('Failed to add domain %s. Response: %s', $domain, wp_json_encode($domain_response))); // Only try SSL if it's not a domain validation error - if (! isset($domain_response['error']) || ! str_contains($domain_response['error'], 'Invalid or empty domain')) { + if (! $this->is_invalid_domain_error($domain_response, $domain)) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
inc/integrations/host-providers/class-closte-host-provider.php
🧰 Additional context used
🧬 Code graph analysis (1)
inc/integrations/host-providers/class-closte-host-provider.php (1)
inc/functions/helper.php (1)
wu_log_add(208-211)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: PHP 8.2
- GitHub Check: cypress (8.1, chrome)
- GitHub Check: Build Plugin for Testing
- GitHub Check: cypress (8.2, chrome)
🔇 Additional comments (2)
inc/integrations/host-providers/class-closte-host-provider.php (2)
256-270: LGTM! Return type standardization improves consistency.The docblock change from
array|objecttoarrayaligns with the PR's goal of standardizing Closte API responses, making the return type more predictable for callers.
305-397: Excellent standardization of API error responses.The consistent array structure for all error returns (lines 309-312, 317-320, 363-367, 385-389, 393-396) improves predictability and makes error handling more robust. The uniform
['success' => false, 'error' => message]pattern is clear and maintainable.
Summary by CodeRabbit
Bug Fixes
Improvements
New Features
✏️ Tip: You can customize this high-level summary in your review settings.