Add payment domain models and currency utilities#27
Add payment domain models and currency utilities#27lohanidamodar wants to merge 10 commits intomainfrom
Conversation
This commit introduces several improvements to make the library more robust and usable following patterns from other utopia-php libraries: New Model Classes: - Customer: Type-safe customer data with address support - PaymentMethod: Comprehensive payment method handling with card expiration checks and display formatting - Payment: Full payment intent/transaction model with status helpers - Refund: Complete refund model with status and reason tracking - Currency: Helper class with validation, conversion between units, formatting, and support for zero/three-decimal currencies Improvements to Existing Classes: - Address: Added toArray(), fromArray(), isComplete(), isEmpty() methods for consistency with other models - Exception: Expanded error types covering card errors, authentication, customer/payment method issues, and refund errors. Added helper methods: isCardError(), isAuthenticationError(), isRetryable(), requiresUserAction(), getUserMessage(), and toArray() - Stripe Adapter: Fixed error handling when response is not an array All new classes include: - Full PHP 8.0+ type hints - Fluent interface (setters return $this) - toArray() and fromArray() for serialization - Comprehensive PHPDoc documentation - Backward compatible with existing API Tests: - Added comprehensive test suites for all new model classes - Added tests for Address class improvements - 107 new tests with 567 assertions, all passing https://claude.ai/code/session_01A28bsuCNWYbM1gS8oJLBRr
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds a large set of domain value objects and utilities (Currency, Customer, Payment, PaymentMethod, Refund, Dispute, SetupIntent, WebhookEvent, IdempotencyKey, Cursor, PaginatedResult, StripeWebhookEvents) and extends Address with toArray/fromArray/isComplete/isEmpty. Exception was expanded with detailed error constants and helpers. Adapter, Pay and Stripe implementations/signatures were tightened to return these domain objects (and accept Address/IdempotencyKey where appropriate); handleError now declares Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 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)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Pay/Payment/Payment.php`:
- Around line 571-580: The getAmountDecimal method incorrectly assumes 2-decimal
currencies by always dividing $this->amount by 100; update it to convert using
the Currency utility instead (e.g., call Currency::fromSmallestUnit or the
provided Currency::fromSmallestUnit($amount, $currency) helper) so it uses the
currency's actual minor unit scale; replace the hardcoded division in
getAmountDecimal(int $decimals = 2): float to use the Currency conversion for
$this->amount and $this->currency, then round the resulting decimal to $decimals
before returning.
In `@src/Pay/Refund/Refund.php`:
- Around line 376-385: getAmountDecimal currently always divides $this->amount
by 100 which breaks zero- and three-decimal currencies; update
Refund::getAmountDecimal to determine the currency's decimal places via the
Currency utility (e.g., Currency::decimalPlaces or similar API) using
$this->currency, compute the divider as pow(10, $currencyDecimalPlaces) and
divide $this->amount by that value, then round to the requested $decimals for
display; reference the method name getAmountDecimal and the Currency class when
making this change.
🧹 Nitpick comments (4)
src/Pay/Currency.php (2)
236-246: Unused$localeparameter should be removed or documented as reserved.The
$localeparameter is declared but never used. Either remove it or add a note that locale-based formatting is planned for a future implementation.♻️ Suggested fix
- public static function format(int $amount, string $currency, ?string $locale = null): string + public static function format(int $amount, string $currency): string { $decimalAmount = self::fromSmallestUnit($amount, $currency); $decimals = self::getDecimalPlaces($currency); $currency = strtoupper($currency); // Simple formatting without locale support for portability $formatted = number_format($decimalAmount, $decimals, '.', ','); return $currency.' '.$formatted; }
298-307:meetsMinimummay produce unexpected results for three-decimal currencies.For three-decimal currencies (BHD, JOD, KWD, OMR, TND), the
$minimumCentsparameter represents cents (1/100), but these currencies use fils/millimes (1/1000). A$minimumCents = 50would mean 0.50 in standard currencies but would still be compared against the smallest unit (fils) without adjustment.Consider documenting this behavior or adding scaling logic for three-decimal currencies.
src/Pay/Customer/Customer.php (1)
35-38: Return type documentation inconsistency.The constructor always sets
createdAttotime()when null is passed (Line 37), sogetCreatedAt()will never actually return null after construction. However, the PHPDoc on Line 204 states@return int|null. Consider updating the return type to justintfor accuracy, or documenting that null is only possible if explicitly set viasetCreatedAt(null).src/Pay/PaymentMethod/PaymentMethod.php (1)
438-455: Verify 2-digit year handling inisExpired().The PHPDoc specifies
expYearshould be 4 digits, but some payment processors may return 2-digit years (e.g.,25instead of2025). The current implementation withDateTime::createFromFormat('Y-n', ...)expects a 4-digit year and would produce incorrect results for 2-digit years.Consider adding validation or normalization in
fromArray()or documenting this requirement explicitly.🛡️ Optional: Normalize 2-digit years in fromArray
- expYear: isset($cardData['exp_year']) ? (int) $cardData['exp_year'] : ($data['expYear'] ?? null), + expYear: isset($cardData['exp_year']) + ? ((int) $cardData['exp_year'] < 100 ? 2000 + (int) $cardData['exp_year'] : (int) $cardData['exp_year']) + : ($data['expYear'] ?? null),
- Adapter abstract class now returns model types (Customer, Payment, PaymentMethod, Refund) instead of generic arrays - Stripe adapter updated to convert API responses to model instances - Pay facade updated with proper return types - createCustomer now accepts Address|null instead of array for address This is a breaking change that improves type safety and IDE support. Users can still access data as arrays using toArray() on any model. Methods updated: - purchase() -> returns Payment - retryPurchase() -> returns Payment - getPayment() -> returns Payment - updatePayment() -> returns Payment - refund() -> returns Refund - createCustomer() -> returns Customer - getCustomer() -> returns Customer - updateCustomer() -> returns Customer - listCustomers() -> returns array<Customer> - createPaymentMethod() -> returns PaymentMethod - getPaymentMethod() -> returns PaymentMethod - updatePaymentMethod() -> returns PaymentMethod - updatePaymentMethodBillingDetails() -> returns PaymentMethod - listPaymentMethods() -> returns array<PaymentMethod> https://claude.ai/code/session_01A28bsuCNWYbM1gS8oJLBRr
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 (1)
src/Pay/Adapter/Stripe.php (1)
259-275:⚠️ Potential issue | 🟠 MajorUpdate tests to pass Address objects instead of arrays.
The tests at
tests/Pay/Adapter/StripeTest.php:33,479, and529are passing arrays directly tocreateCustomer(), but the method signature now requires anAddressobject ornull. These calls need to be updated to instantiateAddressobjects with the array data or passnullif no address is needed.This is a breaking change and causes type errors in strict PHP environments.
🧹 Nitpick comments (6)
src/Pay/Adapter/Stripe.php (4)
81-96: Consider using strict null comparison.Lines 85 and 89 use loose comparison (
!= null). While functionally equivalent for null checks, strict comparison (!== null) is more explicit and avoids potential issues with falsy values.Suggested improvement
- if ($amount != null) { + if ($amount !== null) { $requestBody['amount'] = $amount; } - if ($reason != null) { + if ($reason !== null) { $requestBody['reason'] = $reason; }
219-227: Replace deprecatedasArray()withtoArray().According to the PR summary,
asArray()is deprecated in favor oftoArray(). This should be updated for consistency.Suggested fix
if (! is_null($address)) { - $requestBody['billing_details']['address'] = $address->asArray(); + $requestBody['billing_details']['address'] = $address->toArray(); }
269-271: Replace deprecatedasArray()withtoArray().Same as the billing details case - use the non-deprecated method.
Suggested fix
if (! is_null($address)) { - $requestBody['address'] = $address->asArray(); + $requestBody['address'] = $address->toArray(); }
308-325: Replace deprecatedasArray()withtoArray()in updateCustomer.For consistency with the deprecation, update this usage as well.
Suggested fix
if (! is_null($address)) { - $requestBody['address'] = $address->asArray(); + $requestBody['address'] = $address->toArray(); }src/Pay/Adapter.php (2)
110-118: Consider using explicit nullable type hints for consistency.The parameters use implicit nullable syntax (
int $amount = null) rather than explicit nullable (?int $amount = null). While both work, mixing styles reduces consistency. The explicit?syntax is preferred in modern PHP.Suggested improvement
- abstract public function refund(string $paymentId, int $amount = null, string $reason = null): Refund; + abstract public function refund(string $paymentId, ?int $amount = null, ?string $reason = null): Refund;
202-212: Inconsistent nullable type hint for$addressparameter.Line 185 uses
?Address $address = nullbut line 212 usesAddress $address = null. For consistency, use the explicit nullable syntax throughout.Suggested fix
- abstract public function updateCustomer(string $customerId, string $name, string $email, Address $address = null, string $paymentMethod = null): Customer; + abstract public function updateCustomer(string $customerId, string $name, string $email, ?Address $address = null, ?string $paymentMethod = null): Customer;
New features: - Dispute model for chargeback handling with status/reason constants - SetupIntent model for future payment flows - WebhookEvent class (provider-agnostic) with event categories - StripeWebhookEvents class with Stripe-specific event constants - IdempotencyKey helper for preventing duplicate charges - Pagination support with Cursor and PaginatedResult classes Improvements: - Add PARAM_IDEMPOTENCY_KEY constant to base Adapter - Add idempotency key support to purchase() and refund() methods - Add additionalParams to refund() for consistency with purchase() - Add isDeleted() method to Customer model - Update tests to use model-based API https://claude.ai/code/session_01A28bsuCNWYbM1gS8oJLBRr
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/Pay/Dispute/Dispute.php`:
- Around line 541-545: getAmountDecimal currently always divides by 100 which is
wrong for currencies with 0 or 3 decimal places; update getAmountDecimal to
consult the new Currency utilities (e.g., Currency class) to determine the
correct number of decimal places for $this->currency when $decimals is not
explicitly provided, compute the divisor as pow(10, $currencyDecimals), divide
$this->amount by that divisor, then round to the resolved number of decimals;
refer to the getAmountDecimal method and the Currency class (use its method that
returns decimal places or subunit divisor) to implement this logic.
In `@src/Pay/Pagination/PaginatedResult.php`:
- Around line 89-104: getPreviousCursor() currently falls back to returning
$this->endingBefore when it can't extract an ID from the first item, which
mirrors the same misleading behavior noted for getNextCursor(); update
getPreviousCursor() (and references to $this->data, reset($this->data),
$firstItem, method getId) to return null instead of $this->endingBefore when no
ID can be determined (i.e., after the object/array checks fail), so the method
only returns a valid cursor or null to accurately represent pagination state.
- Around line 64-79: The getNextCursor() method currently returns
$this->startingAfter as a fallback when the last item has no extractable ID;
change this to return null instead because startingAfter represents the current
request cursor, not the next page. Update the logic in getNextCursor()
(referencing the method name) so after checking is_object/$lastItem->getId and
is_array/$lastItem['id'] fail, the method returns null rather than
$this->startingAfter; ensure the hasMore/empty($this->data) early-exit remains
unchanged.
In `@src/Pay/Webhook/WebhookEvent.php`:
- Around line 414-427: In WebhookEvent::fromArray ensure requestId is always a
string or null by replacing the current fallback chain with logic that first
checks if $data['request'] is an array and contains a string 'id', then uses
that; else if $data['request'] is a string use it; otherwise use null — i.e.
extract requestId from $data['request']['id'] only when it exists and is a
string, or from $data['request'] only when it is a string, preventing an array
from being assigned to requestId.
In `@tests/Pay/Adapter/StripeTest.php`:
- Around line 112-114: Update the assertions in the StripeTest.php test to call
the actual PaymentMethod accessor names: replace calls to getExpiryYear() and
getExpiryMonth() on $pm with getExpYear() and getExpMonth() respectively so the
test uses the PaymentMethod methods getExpYear() and getExpMonth(); keep the
expected values (2030 and 8) and the last4() assertion unchanged.
🧹 Nitpick comments (6)
src/Pay/Customer/Customer.php (1)
36-39: Return type inconsistency:getCreatedAt()can never return null.Since the constructor always sets
$this->createdAt = $createdAt ?? time()(Line 39), the value is never null after construction. However,getCreatedAt()returns?int. Consider either:
- Changing the return type to
intforgetCreatedAt()- Or keeping
?intfor semantic consistency with other domain modelsThis is a minor inconsistency that doesn't affect functionality.
src/Pay/Webhook/WebhookEvent.php (1)
354-385: Category detection order may cause unexpected results.The
getCategory()method checks predicates in order, but some event types match multiple categories. For example:
customer.subscription.createdcontains both "customer" and "subscription"isSubscriptionEvent()is checked afterisPaymentEvent(), so subscription events are correctly categorized- However,
isCustomerEvent()is checked last, which meanscustomer.subscription.*events will be categorized asCATEGORY_SUBSCRIPTION, notCATEGORY_CUSTOMERThis is likely intentional (more specific categorization wins), but consider documenting this priority behavior.
src/Pay/SetupIntent/SetupIntent.php (1)
487-498: Consider whetherisComplete()should includeSTATUS_CANCELED.The
isComplete()method returnstruefor bothSUCCEEDEDandCANCELEDstatuses. While technically both are terminal states, semantically "complete" might imply success. Consider renaming toisTerminal()or documenting that "complete" means "no further action possible."src/Pay/Idempotency/IdempotencyKey.php (2)
151-176: Consider documenting hour-boundary behavior for deterministic keys.The hour-level timestamp granularity (
date('Y-m-d-H')) means a retry crossing an hour boundary will generate a different idempotency key. This is a reasonable trade-off for automatic key generation, but callers relying on deterministic keys for retries should be aware of this limitation.Consider adding a note in the PHPDoc explaining this behavior, or offering an overload that accepts a custom timestamp for retry scenarios.
195-199: Minor doc inconsistency: comment mentions underscores but regex also allows hyphens.The comment says "alphanumeric with optional underscores" but the regex
/^[a-zA-Z0-9_-]{8,64}$/also allows hyphens. Consider updating the comment to accurately reflect the allowed characters.📝 Proposed fix
- // Key should be alphanumeric with optional underscores, 8-64 characters + // Key should be alphanumeric with optional underscores and hyphens, 8-64 characters return (bool) preg_match('/^[a-zA-Z0-9_-]{8,64}$/', $key);src/Pay/Adapter.php (1)
219-219: Minor inconsistency: nullable type hint style.Line 192 (
createCustomer) uses?Address $address = nullbut line 219 (updateCustomer) usesAddress $address = null. While both work, the explicit?Addressis preferred for clarity and consistency.📝 Proposed fix
- abstract public function updateCustomer(string $customerId, string $name, string $email, Address $address = null, string $paymentMethod = null): Customer; + abstract public function updateCustomer(string $customerId, string $name, string $email, ?Address $address = null, ?string $paymentMethod = null): Customer;
Resolve conflicts between model-based return types and new authorize/capture/cancel methods from main. All new methods return typed Payment model objects instead of raw arrays. https://claude.ai/code/session_01A28bsuCNWYbM1gS8oJLBRr
Tests for SetupIntent, WebhookEvent, Dispute, IdempotencyKey, Cursor, and PaginatedResult models. Covers constructors, getters/setters, status checks, helper methods, fromArray with both camelCase and snake_case (Stripe) formats, toArray, fluent interfaces, and constants. 111 new tests with 484 assertions. https://claude.ai/code/session_01A28bsuCNWYbM1gS8oJLBRr
- Fix getAmountDecimal() in Payment, Refund, and Dispute to use
Currency utility instead of hardcoding division by 100, properly
handling zero-decimal (JPY) and three-decimal (BHD) currencies
- Fix test method names: getExpiryYear/Month -> getExpYear/Month
to match actual PaymentMethod accessor names
- Fix PaginatedResult cursor fallback: return null instead of stale
request cursors when item IDs cannot be extracted
- Fix WebhookEvent::fromArray() requestId type safety: validate
that request field is a string before assigning, handle Stripe's
object format {id, idempotency_key} correctly
https://claude.ai/code/session_01A28bsuCNWYbM1gS8oJLBRr
- Remove unused $locale parameter from Currency::format() - Add three-decimal currency handling to Currency::meetsMinimum() - Fix Customer::getCreatedAt() return type to int (always set in constructor) - Add 2-digit expiration year normalization in PaymentMethod::fromArray() - Replace 12 loose null comparisons with strict !== null in Stripe adapter - Replace 3 deprecated asArray() calls with toArray() in Stripe adapter - Add tests for three-decimal meetsMinimum and 2-digit year normalization https://claude.ai/code/session_01A28bsuCNWYbM1gS8oJLBRr
…ompatibility The toArray() method used camelCase 'postalCode' key but Stripe API expects snake_case 'postal_code', causing customer creation to fail. https://claude.ai/code/session_01A28bsuCNWYbM1gS8oJLBRr
Bug fixes: - Fix typo: $pyamentMethodId → $paymentMethodId in Stripe adapter - Fix deletePaymentMethod() to validate response instead of always returning true - Fix off_session/confirm sent as string 'true' instead of boolean - Fix flatten() key collisions using array_replace instead of + - Fix Address getters removing unnecessary null coalescing on non-nullable props - Fix Address::getCity() return type from ?string to string Type safety improvements: - Return SetupIntent objects from future payment methods instead of raw arrays - Improve base Adapter::handleError() to use typed Exception with proper error codes (401→auth, 429→rate_limit, 5xx→api_error) instead of generic \Exception - Fix singular/plural mismatch: listFuturePayment → listFuturePayments in Pay facade New features: - Add pagination support (limit, startingAfter) to listCustomers and listPaymentMethods - Add validatePayment() in Adapter base class for currency/amount validation - Add getDispute() and submitDisputeEvidence() to Adapter, Stripe, and Pay facade https://claude.ai/code/session_01A28bsuCNWYbM1gS8oJLBRr
Stripe's form-encoded API expects string 'true' not PHP boolean true, because http_build_query converts boolean true to '1' which Stripe rejects as 'Invalid boolean: 1'. https://claude.ai/code/session_01A28bsuCNWYbM1gS8oJLBRr
Summary
This PR introduces comprehensive domain models and utilities for the payment system, including new classes for managing customers, payments, and currency handling, along with improvements to error handling and the Address class.
Key Changes
New Classes
Currency: Helper class for currency validation and conversionCustomer: Domain model for payment customerstoArray()andfromArray()Payment: Domain model for payment intents/transactionsisSucceeded(),isRefunded(), etc.)Enhanced Classes
Address:toArray()method (preferred over deprecatedasArray())fromArray()static factory method with support for both camelCase and snake_case keysisComplete()andisEmpty()helper methodsException:isCardError(),isAuthenticationError(),isRetryable(),requiresUserAction()getUserMessage()for user-friendly error messagestoArray()for serializationStripeadapter:Exception::GENERAL_UNKNOWNconstant instead of passing raw responseImplementation Details
https://claude.ai/code/session_01A28bsuCNWYbM1gS8oJLBRr
Summary by CodeRabbit
New Features
Bug Fixes
Tests