feat: [E-Commerce] Logic: Payment gateway integration (Stripe/PayPal mock)#230
feat: [E-Commerce] Logic: Payment gateway integration (Stripe/PayPal mock)#230
Conversation
…mock) - PaymentService abstract base with Injectable trait - GatewayInterface for consistent adapter API - MockPaymentGateway — simulates success/failure for testing - StripePaymentGateway — PaymentIntent, refund, webhook handling - PayPalPaymentGateway — PayPal REST SDK integration - payments.yml config with default gateway selection Relates to #71
There was a problem hiding this comment.
Pull request overview
Introduces a payment-gateway abstraction layer intended for the e-commerce checkout flow (Issue #79 / Epic #71), adding Stripe, PayPal, and mock gateway implementations plus initial SilverStripe configuration.
Changes:
- Added
GatewayInterfaceplus gateway implementations for Stripe and PayPal. - Added an abstract
PaymentServiceand aMockPaymentGatewayimplementation. - Added
app/_config/payments.ymlfor gateway selection/configuration.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/Payments/GatewayInterface.php | Defines the common adapter API for payment gateways. |
| app/src/Payments/PaymentService.php | Introduces an abstract base class with shared validation/ID helpers. |
| app/src/Payments/MockPaymentGateway.php | Provides a mock gateway implementation for test/dev flows. |
| app/src/Payments/Gateway/StripePaymentGateway.php | Adds Stripe adapter (payment intent/refund/status/webhook stubs). |
| app/src/Payments/PayPalPaymentGateway.php | Adds PayPal adapter (payment creation/refund/status/webhook stub). |
| app/_config/payments.yml | Attempts to wire the default gateway via SilverStripe config/DI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $intent = PaymentIntent::create($params); | ||
|
|
||
| $customerEmail = $order['CustomerEmail'] ?? null; | ||
| if ($customerEmail) { | ||
| $customers = Customer::search(['email' => $customerEmail, 'limit' => 1]); | ||
| if ($customers->count() === 0) { | ||
| Customer::create(['email' => $customerEmail, 'description' => $order['FirstName'] . ' ' . $order['LastName']]); | ||
| } |
There was a problem hiding this comment.
PaymentIntent and Customer are referenced but not imported or fully-qualified, so this class will fatal at runtime. Add the appropriate use Stripe\PaymentIntent; / use Stripe\Customer; imports (or prefix with \Stripe\...) to ensure the SDK classes resolve correctly.
| public function processPayment(array $order, array $payment = []): array | ||
| { | ||
| try { | ||
| $amount = (float) ($order['TotalAmount'] ?? 0); |
There was a problem hiding this comment.
processPayment() reads the amount from $order['TotalAmount'], but the interface/docs and the other gateways use Amount. As-is, callers passing Amount will end up charging 0 and get an error. Standardise on a single key (e.g. Amount) across all gateways and update this implementation accordingly.
| $amount = (float) ($order['TotalAmount'] ?? 0); | |
| // Prefer standard 'Amount' key; fall back to legacy 'TotalAmount' if needed | |
| $amount = (float) ($order['Amount'] ?? $order['TotalAmount'] ?? 0); |
| } catch (Exception $e) { | ||
| return [ | ||
| 'Success' => false, | ||
| 'Error' => $e->getMessage(), | ||
| 'PaymentIntentID' => $intent->id ?? '', | ||
| 'ClientSecret' => $intent->client_secret, | ||
| ]; |
There was a problem hiding this comment.
In the catch block, ClientSecret is set to $intent->client_secret without a null/defined guard. If the exception occurs before $intent is assigned, this will trigger an undefined variable / property access error and mask the original exception. Use a safe fallback (e.g. $intent->client_secret ?? '') and ensure $intent is initialised.
| $redirectUrls = new \PayPal\Api\RedirectUrls(); | ||
| $redirectUrls->setReturnUrl($paymentDetails['return_url'] ?? Director::absoluteBaseURL() . 'checkout/paypal/return/') | ||
| ->setCancelUrl($paymentDetails['cancel_url'] ?? Director::absoluteBaseURL() . 'checkout/paypal/cancel/'); | ||
|
|
||
| $payment = new \PayPal\Api\Payment(); | ||
| $payment->setIntent('sale') | ||
| ->setPayer($payer) | ||
| ->setRedirectUrls($redirectUrls) | ||
| ->setTransactions([ | ||
| new \PayPal\Api\Transaction([ | ||
| 'amount' => $amountObj, | ||
| 'description' => $orderData['Description'] ?? sprintf('Order #%s', $orderData['OrderID']), | ||
| ]) | ||
| ]); | ||
|
|
||
| $payment->create($this->getApiContext()); | ||
|
|
There was a problem hiding this comment.
Director::absoluteBaseURL() is used but SilverStripe\Control\Director is not imported/qualified, and $this->getApiContext() is called but no getApiContext() method exists in this class. Either add the missing import and implement getApiContext() (including auth with client ID/secret) or remove the calls; otherwise payment creation/refunds will fatally error.
| $secret = Environment::getEnv('PAYPAL_SECRET') ?? ''; | ||
| if (empty($clientId)) { | ||
| throw new \Exception('PayPal client ID not configured. Set PAYPAL_CLIENT_ID environment variable.'); | ||
| } |
There was a problem hiding this comment.
The constructor reads PAYPAL_SECRET into $secret but never validates or uses it. If real SDK mode is intended, missing/empty secrets will only fail later (or never be applied). Validate that both PAYPAL_CLIENT_ID and PAYPAL_SECRET are set, and wire them into the API context initialisation.
| } | |
| } | |
| if (empty($secret)) { | |
| throw new \Exception('PayPal secret not configured. Set PAYPAL_SECRET environment variable.'); | |
| } |
| /** | ||
| * Handle webhook (PayPal notifications) | ||
| */ | ||
| public function handleWebhook(array $payload): array | ||
| { | ||
| $eventType = $payload['event_type'] ?? 'unknown'; | ||
|
|
||
| return [ | ||
| 'Success' => true, | ||
| 'EventType' => $eventType, | ||
| 'Message' => 'Webhook received', | ||
| ]; | ||
| } |
There was a problem hiding this comment.
The docblock says this gateway handles “webhook verification”, but handleWebhook() currently trusts the payload and always returns success. Either implement signature/verification (as required by PayPal) or adjust the documentation/return values so the method doesn’t imply verification it doesn’t perform.
| abstract public function processPayment(array $orderData, array $paymentDetails): array; | ||
|
|
||
| /** | ||
| * Refund a previous payment | ||
| * | ||
| * @param string $transactionID Transaction to refund | ||
| * @param float $amount Amount to refund (0 for full) | ||
| * @return array Result with Status, RefundID, Message | ||
| */ | ||
| abstract public function refundPayment(string $transactionID, float $amount = 0): array; | ||
|
|
||
| /** | ||
| * Get payment status | ||
| * | ||
| * @param string $transactionID Transaction to check | ||
| * @return array Result with Status, Message | ||
| */ | ||
| abstract public function getPaymentStatus(string $transactionID): array; | ||
|
|
||
| /** | ||
| * Handle incoming webhook | ||
| * | ||
| * @param array $payload Webhook payload | ||
| * @return array Result with Status, Message | ||
| */ | ||
| abstract public function handleWebhook(array $payload): array; | ||
|
|
There was a problem hiding this comment.
PaymentService defines refundPayment() but GatewayInterface defines processRefund(). This mismatch makes it impossible to treat all gateways uniformly, and currently Stripe/PayPal implement the interface while Mock extends PaymentService. Align the method names/signatures (e.g. have PaymentService implement GatewayInterface and use processRefund() everywhere, or rename the interface method) so swapping adapters is actually possible.
| class MockPaymentGateway extends PaymentService | ||
| { | ||
| /** @var bool Simulate success or failure */ |
There was a problem hiding this comment.
MockPaymentGateway extends PaymentService and therefore exposes refundPayment(), but it does not implement GatewayInterface (which requires processRefund()). If the intention is “swappable via config without changing calling code”, make Mock implement the same interface as Stripe/PayPal (either by implementing GatewayInterface directly or by fixing PaymentService/method names as noted).
| App\Payments\PaymentService: | ||
| dependencies: | ||
| gateway: %$App\Payments\MockPaymentGateway | ||
|
|
||
| # Stripe Configuration | ||
| StripePaymentGateway: |
There was a problem hiding this comment.
This SilverStripe config isn’t declared under SilverStripe\\Core\\Injector\\Injector:, so it won’t configure DI as intended. Also, mapping dependencies for App\\Payments\\PaymentService is problematic because the class is abstract and it doesn’t define a gateway property/setter. Consider configuring Injector to bind App\\Payments\\GatewayInterface (or a concrete payment service) to the desired gateway implementation, and use fully-qualified class names for gateway-specific config (e.g. App\\Payments\\Gateway\\StripePaymentGateway).
| App\Payments\PaymentService: | |
| dependencies: | |
| gateway: %$App\Payments\MockPaymentGateway | |
| # Stripe Configuration | |
| StripePaymentGateway: | |
| SilverStripe\Core\Injector\Injector: | |
| App\Payments\GatewayInterface: | |
| class: App\Payments\MockPaymentGateway | |
| # Stripe Configuration | |
| App\Payments\Gateway\StripePaymentGateway: |
| /** | ||
| * Process a mock payment | ||
| */ | ||
| public function processPayment(array $orderData, array $paymentDetails): array | ||
| { | ||
| $this->validateOrderData($orderData); | ||
| $this->validateAmount($orderData['Amount']); | ||
|
|
||
| $transactionId = $this->generateTransactionID('MOCK'); | ||
| $status = self::$simulateSuccess ? 'Success' : 'Failure'; | ||
|
|
||
| self::$transactions[$transactionId] = [ | ||
| 'OrderID' => $orderData['OrderID'], | ||
| 'Amount' => $orderData['Amount'], | ||
| 'Currency' => $orderData['Currency'] ?? 'USD', | ||
| 'Status' => $status, | ||
| 'CreatedAt' => date('Y-m-d H:i:s') | ||
| ]; | ||
|
|
||
| return [ | ||
| 'Status' => $status, | ||
| 'TransactionID' => $transactionId, | ||
| 'Message' => $status === 'Success' | ||
| ? 'Mock payment successful' | ||
| : self::$errorMessage | ||
| ]; | ||
| } |
There was a problem hiding this comment.
No PHPUnit coverage is added for the new payment abstraction/gateways. Given existing unit tests in app/src/Test, add tests for MockPaymentGateway (success/failure paths, refund, status lookup) and any shared contract assumptions so the adapter API stays stable.
Fixes #79
Payment gateway abstraction with Stripe, PayPal, and mock adapters.
Architecture
PaymentService (abstract base)
GatewayInterface
MockPaymentGateway
StripePaymentGateway
PayPalPaymentGateway
Relates to #71