Fix PayPal checkout loop and improve error handling #305
Conversation
## Problem When PayPal returns an error (e.g., error 10002 "Security header is not valid" due to invalid API credentials), the checkout would silently loop back to the /register page instead of showing an error message to the user. ## Root Causes Fixed 1. **get_checkout_details() not checking ACK=Failure**: The function only checked HTTP status code (200 OK) but not the PayPal ACK field. When API credentials were invalid, PayPal would return HTTP 200 with ACK=Failure, which was being treated as a valid response. 2. **confirmation_form() returned errors instead of displaying them**: When an error occurred, the function returned an error string but the calling code used output buffering and ignored return values, so errors were never shown. 3. **Missing validation for required data**: No validation for pending_payment, customer, or membership objects before trying to render the confirmation form. ## Changes ### get_checkout_details() - Added ACK=Failure check to detect PayPal API errors - Returns WP_Error on API failures instead of partial data - Added comprehensive logging for all error conditions ### confirmation_form() - Changed return type from string to void - Uses wp_die() to display errors instead of returning strings - Added validation for pending_payment, customer, and membership - Added error logging ### process_confirmation() - Added proper WP_Error handling for get_checkout_details() response - Improved all error messages with better context - Added logging for all error conditions - Uses proper wp_die() calls with back_link option ### create_recurring_profile() & complete_single_payment() - Added logging for API call start, success, and failure - Added ACK=FailureWithWarning check (not just Failure) - Improved error messages to include PayPal error code and message - Fixed HTTP response code from 401 to 200 for proper back links ## Result Users now see clear error messages instead of being silently redirected. All PayPal API errors are logged for debugging. Fixes #193 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a PayPal REST gateway with OAuth and webhook handlers, introduces a shared Base_PayPal_Gateway, updates the legacy NVP PayPal gateway to extend it with improved error/logging, and tightens URL helper return types in Base_Gateway; also updates gateway registration and webhook hooks in the gateway manager. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Site as WP Site (PayPal_REST_Gateway)
participant PayPal as PayPal API
participant Webhook as PayPal_Webhook_Handler
participant DB as WP Ultimo Storage
User->>Site: Start checkout (create order/subscription)
Site->>PayPal: API request to create Order/Subscription (redirect URL)
PayPal-->>User: Approval page (redirect to PayPal)
User->>PayPal: Approve payment/subscription
PayPal-->>Site: Redirect callback (token / subscription_id)
Site->>PayPal: Confirm order/subscription via API
PayPal-->>Site: Confirmation response (status, transaction ids)
Site->>DB: Persist payment/membership updates
PayPal->>Webhook: Send webhook events (captures, refunds, subscription updates)
Webhook->>Site: Verify and dispatch event to update DB/state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
inc/gateways/class-paypal-gateway.php (1)
848-852: Consider using integer for HTTP response code.The
responseparameter inwp_die()expects an integer. While the string'200'works (WordPress casts it), using an integer200would be more conventional and explicit.This pattern appears in multiple
wp_die()calls throughout the file (lines 848, 862, 885, 906, 927, 1351, 1381, 1477, 1558, 1580, 1658, 1687, 1705, 1724).🔎 Example fix (apply to all occurrences)
- 'response' => '200', + 'response' => 200,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
inc/gateways/class-paypal-gateway.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
inc/gateways/class-paypal-gateway.php
1802-1802: Avoid unused local variables such as '$custom'. (undefined)
(UnusedLocalVariable)
⏰ 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). (5)
- GitHub Check: cypress (8.2, chrome)
- GitHub Check: cypress (8.1, chrome)
- GitHub Check: PHP 8.4
- GitHub Check: Build Plugin for Testing
- GitHub Check: PHP 8.1
🔇 Additional comments (7)
inc/gateways/class-paypal-gateway.php (7)
833-866: LGTM!Good addition of WP_Error handling. The error detection for both
WP_Errorobjects and empty/invalid responses covers the main failure scenarios. Logging withLogLevel::ERRORand usingwp_die()withback_linkprovides clear user feedback and debugging capability.
878-931: LGTM!Consistent error handling for missing required objects (payment, cart, membership, customer). Each failure path now logs the error and provides a user-friendly message with a back link, addressing the original issue where errors were silently swallowed.
1333-1397: LGTM!Comprehensive error handling for
CreateRecurringPaymentsProfile. The changes properly:
- Log before initiating the request
- Handle
WP_Errorfromwp_remote_post- Check for both
failureandfailurewithwarningACK values- Extract and display PayPal error codes/messages
- Log success with transaction details for debugging
1469-1480: LGTM!Good addition of logging for unexpected HTTP responses before the
wp_die()call. This will help diagnose edge cases where PayPal returns non-200 status codes.
1532-1662: LGTM!The
complete_single_paymentmethod now has consistent error handling that mirrorscreate_recurring_profile. The addition of logging at entry, for failures, and on success provides good observability for debugging payment issues.
1668-1738: LGTM!The
confirmation_formmethod now properly validates all required objects before rendering the template:
- Validates checkout details response
- Checks for pending payment existence
- Verifies customer and membership data
The return type change to
voidis appropriate since the method now useswp_die()for errors instead of returning error strings.
1779-1790: LGTM - Core fix for the checkout loop issue.This is the critical fix addressing the root cause from Issue #193. The method now properly checks for
ACK=FailureandACK=FailureWithWarningresponses and returns aWP_Errorinstead of treating the response as valid. This prevents the silent loop back to/register.
| $body['pending_payment'] = $pending_payment; | ||
|
|
||
| $custom = explode('|', (string) $body['PAYMENTREQUEST_0_CUSTOM']); | ||
| $custom = explode('|', (string) wu_get_isset($body, 'PAYMENTREQUEST_0_CUSTOM', '')); |
There was a problem hiding this comment.
Remove unused variable.
The $custom variable is assigned but never used. This appears to be dead code.
🔎 Proposed fix
- $custom = explode('|', (string) wu_get_isset($body, 'PAYMENTREQUEST_0_CUSTOM', ''));
-
return $body;Based on static analysis hint from PHPMD.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $custom = explode('|', (string) wu_get_isset($body, 'PAYMENTREQUEST_0_CUSTOM', '')); | |
| return $body; |
🧰 Tools
🪛 PHPMD (2.15.0)
1802-1802: Avoid unused local variables such as '$custom'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In inc/gateways/class-paypal-gateway.php around line 1802, the variable $custom
is assigned from explode(...) but never used; remove that unused assignment line
to eliminate dead code and satisfy PHPMD static analysis.
- Add Base_PayPal_Gateway abstract class for shared PayPal functionality - Add PayPal_REST_Gateway with modern PayPal REST API implementation - Add PayPal_OAuth_Handler for OAuth 2.0 authentication flow - Add PayPal_Webhook_Handler for webhook event processing - Update Gateway_Manager to register new PayPal REST gateway and use filters - Fix Base_Gateway methods to properly return strings for filter callbacks - Maintain backwards compatibility with legacy PayPal gateway Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🔨 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: 5
🤖 Fix all issues with AI agents
In @inc/gateways/class-base-paypal-gateway.php:
- Around line 164-177: Replace usages of the PHP 8-only function str_starts_with
(e.g., the check in class-base-paypal-gateway.php that tests
$gateway_subscription_id) with a PHP 7.4-compatible check: use
strpos($gateway_subscription_id, 'I-') === 0 (or an equivalent substr/strncmp)
to detect the prefix; apply the same replacement to the other ~28 occurrences
across the codebase or alternatively add a simple polyfill for str_starts_with
if you prefer to keep the original calls, but do not change composer PHP
requirement unless intentionally bumping minimum PHP to 8.0.
In @inc/gateways/class-paypal-oauth-handler.php:
- Around line 724-754: The delete_webhooks_on_disconnect method is calling
$gateway_manager->get_gateway('paypal-rest') which returns gateway metadata, not
an instance; replace that call with the Gateway_Manager method that returns an
actual gateway object (for example use get_gateway_instance('paypal-rest') or
the manager's equivalent instance factory), then verify the returned value is an
instance of PayPal_REST_Gateway before calling set_test_mode() and
delete_webhook(); update the variable name if needed (e.g., $gateway_instance)
and keep the existing error logging and try/catch logic unchanged.
- Around line 688-714: The instanceof check in install_webhook_after_oauth is
wrong because $gateway_manager->get_gateway('paypal-rest') returns metadata, so
replace that call with wu_get_gateway('paypal-rest') to obtain an actual
PayPal_REST_Gateway instance; update the code path that currently assigns
$gateway from Gateway_Manager::get_instance()->get_gateway(...) to call
wu_get_gateway('paypal-rest'), keep the subsequent set_test_mode/install_webhook
logic and the same logging branches, and ensure the instanceof
PayPal_REST_Gateway check now succeeds and the webhook installation is
attempted.
In @inc/gateways/class-paypal-webhook-handler.php:
- Around line 183-187: The current check in class PayPal_Webhook_Handler (when
$webhook_id is empty) returns $this->test_mode which accepts any webhook in test
mode; change this to never auto-accept missing webhook IDs. Replace the line
"return $this->test_mode" so the method logs a clear error and returns false (do
not accept the request), e.g., keep the warning log or escalate to error and
then return false; if you need an explicit opt-in for development, add a boolean
property like $allow_unverified_webhooks_in_test (default false) and only return
true when that property is explicitly true.
- Around line 172-176: The current unconditional bypass in the signature-check
block (when $this->test_mode and WP_DEBUG) is unsafe; change it to only skip
cryptographic verification while still requiring the webhook headers to exist
and be well-formed: keep the $this->test_mode and WP_DEBUG branch but replace
the immediate return true with a header presence check for the PayPal webhook
headers (e.g., 'Paypal-Transmission-Id', 'Paypal-Transmission-Sig',
'Paypal-Transmission-Time', 'Paypal-Cert-Url') and return true only if those
headers are present (otherwise return false and log an error); update the log
call ($this->log('Skipping signature verification in debug mode')) to include
which headers were missing so the verification path in the class (the method
using $this->test_mode and the log call) still defends against spoofed requests
in debug/test environments.
🧹 Nitpick comments (7)
inc/gateways/class-paypal-oauth-handler.php (1)
393-394: Remove unused variables or add TODO for future use.
$consent_statusand$risk_statusare captured from PayPal's OAuth callback but never used. If these are intended for future validation logic, add a TODO comment; otherwise, remove them to reduce dead code.♻️ Suggested fix
- $consent_status = isset($_GET['consentStatus']) && 'true' === $_GET['consentStatus']; - $risk_status = isset($_GET['isEmailConfirmed']) ? sanitize_text_field(wp_unslash($_GET['isEmailConfirmed'])) : ''; + // Note: consentStatus and isEmailConfirmed are available in the callback but not currently used + // They could be used for additional validation if needed in the futureinc/gateways/class-paypal-gateway.php (1)
1793-1793: Remove unused$customvariable.The
$customvariable is parsed from the response but never used.♻️ Suggested fix
- $custom = explode('|', (string) wu_get_isset($body, 'PAYMENTREQUEST_0_CUSTOM', ''));inc/gateways/class-paypal-webhook-handler.php (1)
419-481: Handle potential duplicate payment creation race condition.The check for existing payment on line 449-454 and subsequent payment creation could have a race condition if two webhook deliveries arrive simultaneously. Consider using a database lock or unique constraint.
💡 Consideration
The current flow:
- Check if payment exists by gateway_payment_id
- If not, create payment
If two webhooks arrive simultaneously, both could pass the existence check before either creates the payment. Consider wrapping this in a transaction or using a unique constraint on
gateway_payment_idin the database.inc/gateways/class-paypal-rest-gateway.php (4)
376-395: Remove unused$descriptionvariable.The
$descriptionvariable is created on line 379 but never used in the subscription creation flow.♻️ Suggested fix
protected function create_subscription($payment, $membership, $customer, $cart, $type): void { $currency = strtoupper($payment->get_currency()); - $description = $this->get_subscription_description($cart); // First, create or get the billing plan $plan_id = $this->get_or_create_plan($cart, $currency);
788-788: Unused$customer_idfrom destructuring.The
$customer_idis extracted but not used. If it's not needed, use an underscore or remove it from destructuring.♻️ Suggested fix
- [$payment_id, $membership_id, $customer_id] = $custom_parts; + [$payment_id, $membership_id] = $custom_parts;Apply the same fix on line 880.
Also applies to: 880-880
1262-1313: Inline JavaScript in settings should be extracted.The inline JavaScript for the connect/disconnect buttons works but could be extracted to a separate JS file for better maintainability and to enable proper linting.
993-1009:process_membership_updateis incomplete.The method logs the update request but doesn't actually implement the update logic. PayPal subscription updates are limited, but the method should either attempt what's possible or return an error explaining limitations.
💡 Suggested improvement
public function process_membership_update(&$membership, $customer) { $subscription_id = $membership->get_gateway_subscription_id(); if (empty($subscription_id)) { return new \WP_Error( 'wu_paypal_no_subscription', __('No subscription ID found for this membership.', 'ultimate-multisite') ); } - // Note: PayPal subscription updates are limited - // For significant changes, may need to cancel and recreate - $this->log(sprintf('Membership update requested for subscription: %s', $subscription_id)); + // PayPal REST API has limited subscription update capabilities + // For amount/duration changes, cancel current subscription and create new one + $this->log(sprintf('Membership update requested for subscription: %s - cancelling and requiring new subscription', $subscription_id)); - return true; + // Cancel the current subscription + $this->process_cancellation($membership, $customer); + + // Reset gateway info so a new subscription will be created + $membership->set_gateway_subscription_id(''); + $membership->set_auto_renew(false); + + return true; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
inc/gateways/class-base-gateway.phpinc/gateways/class-base-paypal-gateway.phpinc/gateways/class-paypal-gateway.phpinc/gateways/class-paypal-oauth-handler.phpinc/gateways/class-paypal-rest-gateway.phpinc/gateways/class-paypal-webhook-handler.phpinc/managers/class-gateway-manager.php
🧰 Additional context used
🧬 Code graph analysis (4)
inc/gateways/class-paypal-oauth-handler.php (5)
inc/gateways/class-paypal-rest-gateway.php (2)
init(85-90)is_configured(155-162)inc/gateways/class-paypal-webhook-handler.php (2)
init(45-51)get_api_base_url(59-62)inc/functions/settings.php (2)
wu_get_setting(37-40)wu_save_setting(51-54)inc/gateways/class-base-paypal-gateway.php (2)
get_api_base_url(98-101)is_configured(291-291)inc/functions/helper.php (1)
wu_log_add(208-211)
inc/gateways/class-base-gateway.php (2)
inc/gateways/class-base-paypal-gateway.php (2)
get_payment_url_on_gateway(130-143)get_subscription_url_on_gateway(155-177)inc/gateways/class-base-stripe-gateway.php (3)
get_payment_url_on_gateway(2939-2950)get_subscription_url_on_gateway(2962-2967)get_customer_url_on_gateway(2979-2984)
inc/gateways/class-paypal-gateway.php (2)
inc/gateways/class-base-paypal-gateway.php (2)
is_configured(291-291)get_connection_status(299-299)inc/functions/helper.php (1)
wu_log_add(208-211)
inc/gateways/class-paypal-rest-gateway.php (3)
inc/gateways/class-base-paypal-gateway.php (4)
get_api_base_url(98-101)log(221-224)add_partner_attribution_header(205-210)get_subscription_description(111-118)inc/gateways/class-paypal-oauth-handler.php (2)
init(79-91)get_api_base_url(119-122)inc/gateways/class-paypal-webhook-handler.php (3)
init(45-51)get_api_base_url(59-62)log(574-577)
🪛 GitHub Check: Code Quality Checks
inc/gateways/class-base-gateway.php
[warning] 536-536:
The method parameter $gateway_payment_id is never used
[warning] 551-551:
The method parameter $gateway_subscription_id is never used
[warning] 566-566:
The method parameter $gateway_customer_id is never used
inc/gateways/class-paypal-webhook-handler.php
[warning] 367-367:
It is recommended not to use reserved keyword "resource" as function parameter name. Found: $resource
[warning] 341-341:
It is recommended not to use reserved keyword "resource" as function parameter name. Found: $resource
[warning] 325-325:
It is recommended not to use reserved keyword "resource" as function parameter name. Found: $resource
[warning] 297-297:
It is recommended not to use reserved keyword "resource" as function parameter name. Found: $resource
[warning] 280-280:
It is recommended not to use reserved keyword "resource" as function parameter name. Found: $resource
[warning] 221-221:
base64_encode() can be used to obfuscate code which is strongly discouraged. Please verify that the function is used for benign reasons.
🪛 PHPMD (2.15.0)
inc/gateways/class-paypal-oauth-handler.php
393-393: Avoid unused local variables such as '$consent_status'. (undefined)
(UnusedLocalVariable)
394-394: Avoid unused local variables such as '$risk_status'. (undefined)
(UnusedLocalVariable)
inc/gateways/class-base-gateway.php
536-536: Avoid unused parameters such as '$gateway_payment_id'. (undefined)
(UnusedFormalParameter)
551-551: Avoid unused parameters such as '$gateway_subscription_id'. (undefined)
(UnusedFormalParameter)
566-566: Avoid unused parameters such as '$gateway_customer_id'. (undefined)
(UnusedFormalParameter)
inc/gateways/class-paypal-gateway.php
1793-1793: Avoid unused local variables such as '$custom'. (undefined)
(UnusedLocalVariable)
inc/gateways/class-base-paypal-gateway.php
253-253: Avoid unused parameters such as '$atts'. (undefined)
(UnusedFormalParameter)
253-253: Avoid unused parameters such as '$site'. (undefined)
(UnusedFormalParameter)
inc/gateways/class-paypal-rest-gateway.php
376-376: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
379-379: Avoid unused local variables such as '$description'. (undefined)
(UnusedLocalVariable)
610-610: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
788-788: Avoid unused local variables such as '$customer_id'. (undefined)
(UnusedLocalVariable)
880-880: Avoid unused local variables such as '$customer_id'. (undefined)
(UnusedLocalVariable)
924-924: Avoid unused parameters such as '$customer'. (undefined)
(UnusedFormalParameter)
957-957: Avoid unused parameters such as '$membership'. (undefined)
(UnusedFormalParameter)
957-957: Avoid unused parameters such as '$customer'. (undefined)
(UnusedFormalParameter)
993-993: Avoid unused parameters such as '$customer'. (undefined)
(UnusedFormalParameter)
⏰ 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: cypress (8.2, chrome)
- GitHub Check: Build Plugin for Testing
- GitHub Check: cypress (8.1, chrome)
- GitHub Check: PHP 8.4
🔇 Additional comments (16)
inc/gateways/class-base-gateway.php (1)
534-569: LGTM - Return type declarations align with gateway pattern.These base class stub methods correctly return empty strings and declare explicit return types, matching the established pattern in
Base_Stripe_Gateway. The unused parameters are intentional as these methods are designed to be overridden by child classes.inc/managers/class-gateway-manager.php (3)
417-431: LGTM - PayPal gateway registration and webhook initialization.The new PayPal REST gateway registration with OAuth-based descriptions, the legacy PayPal gateway re-labeling, and the webhook handler initialization are well-structured.
543-547: Appropriate change from actions to filters for URL retrieval.Using
add_filterinstead ofadd_actionis semantically correct here since these hooks are meant to return URL data that can be used by other components. This aligns with the filter-based data retrieval pattern.
556-556: Direct method call is cleaner.Replacing
call_user_func([$gateway, 'fields'])with$gateway->fields()improves readability without changing behavior.inc/gateways/class-paypal-oauth-handler.php (1)
79-91: OAuth handler initialization looks correct.The handler properly loads test mode setting, partner credentials, registers AJAX handlers with nonce verification, and hooks into admin_init for OAuth return handling.
inc/gateways/class-base-paypal-gateway.php (2)
253-283: Filter callback signature is correct.The
$attsand$siteparameters are required by thewu_element_get_site_actionsfilter signature even though they're not used in this implementation. This is standard practice for WordPress filter callbacks.
23-79: Well-structured base class for PayPal gateways.The abstract class provides good shared functionality including:
- Cross-gateway hook compatibility via
$other_ids- Partner attribution tracking via
$bn_code- Common capability declarations for recurring payments
inc/gateways/class-paypal-gateway.php (4)
841-874: Excellent error handling for PayPal API failures.This change directly addresses the PR objective. When
get_checkout_details()returns aWP_Error, the code now properly displays the error viawp_die()instead of silently failing and looping back. The logging and contextual error messages will help with debugging.
1770-1781: Key fix:get_checkout_detailsnow returnsWP_Erroron API failures.This is the core fix for issue #193. Previously, ACK=Failure responses were not properly detected, causing the checkout to loop. Now the method:
- Checks for ACK=Failure or FailureWithWarning
- Logs the error with code and message
- Returns a
WP_Errorthat callers can handle
1378-1392: Good addition ofFailureWithWarninghandling.PayPal can return
FailureWithWarningin addition toFailure. Handling both ensures edge cases are properly caught and displayed to users with the PayPal error code and message.Also applies to: 1560-1574
1661-1729:confirmation_form()now properly validates and handles errors.The method now:
- Returns void (breaking change noted in AI summary)
- Validates pending_payment, customer, and membership exist
- Uses
wp_die()to display errors with back links- Logs errors for debugging
This prevents silent failures and provides clear user feedback.
inc/gateways/class-paypal-webhook-handler.php (2)
280-287: Parameter name$resourceis acceptable.The static analysis warning about
$resourcebeing a reserved keyword is a false positive. Whileresourceis a type in PHP, it's not a reserved keyword and can be used as a parameter name.Also applies to: 297-315, 325-331, 341-357, 367-382, 392-407
70-147: Well-structured webhook processor with comprehensive event handling.The webhook handler properly:
- Validates incoming JSON
- Verifies webhook signatures via PayPal API
- Routes events to appropriate handlers
- Returns appropriate HTTP status codes
inc/gateways/class-paypal-rest-gateway.php (3)
218-273: Good token caching implementation.The access token caching with a 5-minute safety buffer before expiration is a solid pattern. This prevents token expiration mid-request while minimizing unnecessary token refreshes.
506-596: Billing plan caching is efficient.Caching plan IDs via
update_site_optionwith an MD5 hash key based on plan parameters avoids recreating identical plans on PayPal. The verification check for existing plans handles the case where a plan was deleted on PayPal's side.
85-90: Gateway initialization and hooks are well-structured.The init method properly loads test mode and credentials. The hooks method initializes the OAuth handler, registers webhook installation handlers, and sets up admin notices - providing a complete gateway lifecycle.
Also applies to: 132-147
| if (str_starts_with($gateway_subscription_id, 'I-')) { | ||
| // REST API subscription | ||
| return sprintf( | ||
| 'https://www.%spaypal.com/billing/subscriptions/%s', | ||
| $sandbox_prefix, | ||
| $gateway_subscription_id | ||
| ); | ||
| } | ||
|
|
||
| // Legacy NVP recurring payment profile | ||
| $base_url = 'https://www.%spaypal.com/us/cgi-bin/webscr?cmd=_profile-recurring-payments&encrypted_profile_id=%s'; | ||
|
|
||
| return sprintf($base_url, $sandbox_prefix, $gateway_subscription_id); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check composer.json for PHP version requirement
echo "=== composer.json PHP requirement ==="
if [ -f composer.json ]; then
cat composer.json | jq '.require.php // .config."platform"."php" // empty' 2>/dev/null || grep -i '"php"' composer.json
else
echo "No composer.json found"
fi
echo ""
echo "=== Checking README or plugin header for version info ==="
head -50 README.md 2>/dev/null | grep -i "php\|version" || echo "No README found"
echo ""
echo "=== Checking main plugin file for version requirements ==="
fd "*.php" -x grep -l "Requires PHP\|requires_php" {} \; 2>/dev/null | head -5
echo ""
echo "=== Checking if str_starts_with polyfill exists ==="
rg "function str_starts_with|if.*str_starts_with" -A 2 -B 2
echo ""
echo "=== Checking all uses of str_starts_with in codebase ==="
rg "str_starts_with" --type php -nRepository: Multisite-Ultimate/ultimate-multisite
Length of output: 22048
str_starts_with() is incompatible with PHP 7.4.1 (the minimum supported version per composer.json).
The function str_starts_with() (used in lines 164 and 191) requires PHP 8.0+. Since the plugin supports PHP 7.4.1 and no polyfill exists in the codebase, this will cause a fatal error on any PHP 7.4 installation. This affects not only the PayPal gateway but also 28 other files throughout the codebase using the same function.
Either update the minimum PHP version requirement to 8.0+, add a polyfill for str_starts_with(), or replace the function call with PHP 7.4-compatible alternatives (e.g., strpos() === 0).
🤖 Prompt for AI Agents
In @inc/gateways/class-base-paypal-gateway.php around lines 164 - 177, Replace
usages of the PHP 8-only function str_starts_with (e.g., the check in
class-base-paypal-gateway.php that tests $gateway_subscription_id) with a PHP
7.4-compatible check: use strpos($gateway_subscription_id, 'I-') === 0 (or an
equivalent substr/strncmp) to detect the prefix; apply the same replacement to
the other ~28 occurrences across the codebase or alternatively add a simple
polyfill for str_starts_with if you prefer to keep the original calls, but do
not change composer PHP requirement unless intentionally bumping minimum PHP to
8.0.
| protected function install_webhook_after_oauth(string $mode_prefix): void { | ||
|
|
||
| try { | ||
| // Get the PayPal REST gateway instance | ||
| $gateway_manager = \WP_Ultimo\Managers\Gateway_Manager::get_instance(); | ||
| $gateway = $gateway_manager->get_gateway('paypal-rest'); | ||
|
|
||
| if (! $gateway instanceof PayPal_REST_Gateway) { | ||
| wu_log_add('paypal', 'Could not get PayPal REST gateway instance for webhook installation', LogLevel::WARNING); | ||
| return; | ||
| } | ||
|
|
||
| // Ensure the gateway is in the correct mode | ||
| $gateway->set_test_mode('sandbox' === $mode_prefix); | ||
|
|
||
| // Install the webhook | ||
| $result = $gateway->install_webhook(); | ||
|
|
||
| if (true === $result) { | ||
| wu_log_add('paypal', sprintf('Webhook installed successfully for %s mode after OAuth', $mode_prefix)); | ||
| } elseif (is_wp_error($result)) { | ||
| wu_log_add('paypal', sprintf('Failed to install webhook after OAuth: %s', $result->get_error_message()), LogLevel::ERROR); | ||
| } | ||
| } catch (\Exception $e) { | ||
| wu_log_add('paypal', sprintf('Exception installing webhook after OAuth: %s', $e->getMessage()), LogLevel::ERROR); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "function get_gateway|function wu_get_gateway" --type=php -A 15Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 9592
🏁 Script executed:
rg -n "registered_gateways\[" --type=php -B 2 -A 2 | head -50Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 1583
🏁 Script executed:
rg -n "PayPal_REST_Gateway|register_gateway.*paypal" --type=php -A 5Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 3367
Type check should use wu_get_gateway() to get a gateway instance instead of using get_gateway() which returns metadata array.
The instanceof check on line 695 will always fail because $gateway_manager->get_gateway() returns an associative array with gateway metadata, not a PayPal_REST_Gateway instance. Use wu_get_gateway('paypal-rest') instead, which properly instantiates and returns the gateway object. This prevents webhook installation from being silently skipped.
🐛 Proposed fix
protected function install_webhook_after_oauth(string $mode_prefix): void {
try {
- // Get the PayPal REST gateway instance
- $gateway_manager = \WP_Ultimo\Managers\Gateway_Manager::get_instance();
- $gateway = $gateway_manager->get_gateway('paypal-rest');
+ // Get a fresh PayPal REST gateway instance
+ $gateway = wu_get_gateway('paypal-rest');
- if (! $gateway instanceof PayPal_REST_Gateway) {
+ if (! $gateway) {
wu_log_add('paypal', 'Could not get PayPal REST gateway instance for webhook installation', LogLevel::WARNING);
return;
}🤖 Prompt for AI Agents
In @inc/gateways/class-paypal-oauth-handler.php around lines 688 - 714, The
instanceof check in install_webhook_after_oauth is wrong because
$gateway_manager->get_gateway('paypal-rest') returns metadata, so replace that
call with wu_get_gateway('paypal-rest') to obtain an actual PayPal_REST_Gateway
instance; update the code path that currently assigns $gateway from
Gateway_Manager::get_instance()->get_gateway(...) to call
wu_get_gateway('paypal-rest'), keep the subsequent set_test_mode/install_webhook
logic and the same logging branches, and ensure the instanceof
PayPal_REST_Gateway check now succeeds and the webhook installation is
attempted.
| protected function delete_webhooks_on_disconnect(): void { | ||
|
|
||
| try { | ||
| $gateway_manager = \WP_Ultimo\Managers\Gateway_Manager::get_instance(); | ||
| $gateway = $gateway_manager->get_gateway('paypal-rest'); | ||
|
|
||
| if (! $gateway instanceof PayPal_REST_Gateway) { | ||
| return; | ||
| } | ||
|
|
||
| // Try to delete sandbox webhook | ||
| $gateway->set_test_mode(true); | ||
| $result = $gateway->delete_webhook(); | ||
| if (is_wp_error($result)) { | ||
| wu_log_add('paypal', sprintf('Failed to delete sandbox webhook: %s', $result->get_error_message()), LogLevel::WARNING); | ||
| } else { | ||
| wu_log_add('paypal', 'Sandbox webhook deleted during disconnect'); | ||
| } | ||
|
|
||
| // Try to delete live webhook | ||
| $gateway->set_test_mode(false); | ||
| $result = $gateway->delete_webhook(); | ||
| if (is_wp_error($result)) { | ||
| wu_log_add('paypal', sprintf('Failed to delete live webhook: %s', $result->get_error_message()), LogLevel::WARNING); | ||
| } else { | ||
| wu_log_add('paypal', 'Live webhook deleted during disconnect'); | ||
| } | ||
| } catch (\Exception $e) { | ||
| wu_log_add('paypal', sprintf('Exception deleting webhooks during disconnect: %s', $e->getMessage()), LogLevel::WARNING); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue with gateway instance retrieval.
This method has the same issue as install_webhook_after_oauth - $gateway_manager->get_gateway() returns gateway metadata, not a gateway instance.
🐛 Proposed fix
protected function delete_webhooks_on_disconnect(): void {
try {
- $gateway_manager = \WP_Ultimo\Managers\Gateway_Manager::get_instance();
- $gateway = $gateway_manager->get_gateway('paypal-rest');
+ $gateway = wu_get_gateway('paypal-rest');
- if (! $gateway instanceof PayPal_REST_Gateway) {
+ if (! $gateway) {
return;
}🤖 Prompt for AI Agents
In @inc/gateways/class-paypal-oauth-handler.php around lines 724 - 754, The
delete_webhooks_on_disconnect method is calling
$gateway_manager->get_gateway('paypal-rest') which returns gateway metadata, not
an instance; replace that call with the Gateway_Manager method that returns an
actual gateway object (for example use get_gateway_instance('paypal-rest') or
the manager's equivalent instance factory), then verify the returned value is an
instance of PayPal_REST_Gateway before calling set_test_mode() and
delete_webhook(); update the variable name if needed (e.g., $gateway_instance)
and keep the existing error logging and try/catch logic unchanged.
| if ($this->test_mode && defined('WP_DEBUG') && WP_DEBUG) { | ||
| $this->log('Skipping signature verification in debug mode'); | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Security concern: Debug mode bypasses signature verification entirely.
Skipping signature verification when WP_DEBUG is true and in test mode could allow spoofed webhooks in development environments that might still have real data. Consider requiring at least the webhook headers to be present, even if verification is skipped.
🔒 Suggested improvement
// If headers are missing, we can't verify
if (empty($auth_algo) || empty($cert_url) || empty($transmission_id) || empty($transmission_sig) || empty($transmission_time)) {
$this->log('Missing webhook signature headers', LogLevel::WARNING);
- // In development/testing, you might want to skip verification
- if ($this->test_mode && defined('WP_DEBUG') && WP_DEBUG) {
- $this->log('Skipping signature verification in debug mode');
- return true;
- }
+ // Even in debug mode, require headers to be present to prevent accidental acceptance of malformed requests
return false;
}🤖 Prompt for AI Agents
In @inc/gateways/class-paypal-webhook-handler.php around lines 172 - 176, The
current unconditional bypass in the signature-check block (when $this->test_mode
and WP_DEBUG) is unsafe; change it to only skip cryptographic verification while
still requiring the webhook headers to exist and be well-formed: keep the
$this->test_mode and WP_DEBUG branch but replace the immediate return true with
a header presence check for the PayPal webhook headers (e.g.,
'Paypal-Transmission-Id', 'Paypal-Transmission-Sig', 'Paypal-Transmission-Time',
'Paypal-Cert-Url') and return true only if those headers are present (otherwise
return false and log an error); update the log call ($this->log('Skipping
signature verification in debug mode')) to include which headers were missing so
the verification path in the class (the method using $this->test_mode and the
log call) still defends against spoofed requests in debug/test environments.
| if (empty($webhook_id)) { | ||
| $this->log('Webhook ID not configured, skipping verification', LogLevel::WARNING); | ||
| // Allow in test mode without webhook ID | ||
| return $this->test_mode; | ||
| } |
There was a problem hiding this comment.
Allowing webhooks without webhook ID in test mode is risky.
Returning true when webhook ID is not configured in test mode means any request to the webhook endpoint will be accepted. This could mask configuration issues and process illegitimate requests.
🔒 Suggested improvement
if (empty($webhook_id)) {
$this->log('Webhook ID not configured, skipping verification', LogLevel::WARNING);
- // Allow in test mode without webhook ID
- return $this->test_mode;
+ // Require webhook ID to be configured for security
+ return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (empty($webhook_id)) { | |
| $this->log('Webhook ID not configured, skipping verification', LogLevel::WARNING); | |
| // Allow in test mode without webhook ID | |
| return $this->test_mode; | |
| } | |
| if (empty($webhook_id)) { | |
| $this->log('Webhook ID not configured, skipping verification', LogLevel::WARNING); | |
| // Require webhook ID to be configured for security | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In @inc/gateways/class-paypal-webhook-handler.php around lines 183 - 187, The
current check in class PayPal_Webhook_Handler (when $webhook_id is empty)
returns $this->test_mode which accepts any webhook in test mode; change this to
never auto-accept missing webhook IDs. Replace the line "return
$this->test_mode" so the method logs a clear error and returns false (do not
accept the request), e.g., keep the warning log or escalate to error and then
return false; if you need an explicit opt-in for development, add a boolean
property like $allow_unverified_webhooks_in_test (default false) and only return
true when that property is explicitly true.
Summary
Fixes the issue where PayPal checkout would silently loop back to the
/registerpage instead of showing an error message when PayPal API calls fail (e.g., error 10002 "Security header is not valid" due to invalid credentials).Root Causes Fixed
get_checkout_details()not checking ACK=Failure - The function only checked HTTP status code (200 OK) but not the PayPal ACK field. When API credentials were invalid, PayPal would return HTTP 200 with ACK=Failure, which was being treated as valid data.confirmation_form()returned errors instead of displaying them - When an error occurred, the function returned an error string, but the calling code used output buffering and ignored return values, so errors were never shown to users.Missing validation for required data - No validation for
pending_payment,customer, ormembershipobjects before trying to render the confirmation form.Changes
get_checkout_details(): Added ACK=Failure check to detect PayPal API errors, returns WP_Error on failures instead of partial data, added comprehensive loggingconfirmation_form(): Changed return type from string to void, useswp_die()to display errors, added validation for required objectsprocess_confirmation(): Added proper WP_Error handling, improved error messages with better context, added logging for all error conditionscreate_recurring_profile()&complete_single_payment(): Added logging for API call lifecycle, added ACK=FailureWithWarning check, improved error messages to include PayPal error code and messageResult
Users now see clear error messages instead of being silently redirected back to the checkout page. All PayPal API errors are logged for debugging.
Test plan
/wp-admin/network/admin.php?page=wp-ultimo-logs&tab=paypal) for proper loggingFixes #193
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.