Guard against missing status in CompletePaidPaymentsCommand#523
Open
Renrhaf wants to merge 1 commit into
Open
Conversation
When PayPalClient::request() cannot reach the PayPal API (for example
when an uncaptured order has expired after ~3h and GET /v2/checkout/
orders/{id} returns 404 RESOURCE_NOT_FOUND), it logs the error and
silently returns the error payload instead of throwing. That payload
has no "status" key, so the existing `$details['status'] === 'COMPLETED'`
check in CompletePaidPaymentsCommand emits a PHP warning
("Undefined array key \"status\"") — promoted to an ErrorException by
Symfony's error handler — on every hourly cron tick, forever, for each
abandoned PayPal checkout.
Skip the payment when the response has no status key; the underlying
error has already been logged inside PayPalClient. The happy path is
unchanged.
Adds a dedicated unit test for CompletePaidPaymentsCommand covering
the COMPLETED path, the new no-status branch (with an assertion that
no PHP warning is raised), a non-terminal status (APPROVED), and the
non-PayPal gateway skip.
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.
The bug
CompletePaidPaymentsCommand::execute()runssylius-paypal:complete-payments(typically from cron) and, for each PayPalPaymentstuck inprocessing, callsOrderDetailsApi::get()and checks$details['status'] === 'COMPLETED'.PayPalClient::request()never throws on non-2xx responses — when an API call fails (for example when an uncaptured PayPal order has expired after ~3h andGET /v2/checkout/orders/{id}returns404 RESOURCE_NOT_FOUND) it logs the error but silently returns the JSON error body as an array. That body looks like{ "name": "RESOURCE_NOT_FOUND", "debug_id": "…", "message": "…" }— nostatuskey.Consequence: every hourly cron tick emits
Warning: Undefined array key "status"(promoted toErrorExceptionby Symfony's error handler), thePaymentkeeps sitting inprocessing, and the same cycle repeats forever for each abandoned PayPal checkout. One real-world production instance collected ~9 duplicate Sentry events over 9 hours for a single abandoned order before this was caught.Reproduction: create a PayPal order through the shop, close the PayPal approval window without clicking Pay, then wait > 3h (or otherwise expire the order on PayPal's side) and trigger the cron.
The fix
Skip the payment when the response has no
statuskey — the underlying error has already been logged insidePayPalClient::request(). The happy path (status === 'COMPLETED'→ transition) is unchanged.Only one check is added; the diff is a handful of lines in the command plus a dedicated unit test covering:
APPROVED) is left aloneOut of scope
This PR is intentionally minimal. It stops the warning and the hourly log noise, but a
Paymentwhose PayPal order has permanently disappeared will still stayprocessingand be re-polled until it is cancelled by hand. Deciding when to automatically transition such payments tofailed(retry-limit? age-based? explicit 404 detection?) is a bigger design call better left to a follow-up.