PRE-3585: Set alias with Unified hosted field - #315
Merged
Conversation
hdelaforce-payplug
force-pushed
the
feature/PRE-3585_create_uhf_aliasing
branch
3 times, most recently
from
August 28, 2026 14:20
4209f19 to
7d8a439
Compare
hdelaforce-payplug
force-pushed
the
feature/PRE-3585_create_uhf_aliasing
branch
3 times, most recently
from
August 28, 2026 14:51
01eadbe to
2748168
Compare
adumont-payplug
left a comment
Collaborator
There was a problem hiding this comment.
Code Review — PRE-3585 (UHF Aliasing)
Reviewed the full diff against develop, cross-checked the new payplug/unified-plugin-core DTO signatures against the actual 1.1.0 release source, and traced the webhook routing end-to-end. Inline comments below call out specific lines; summary here.
Dimension ratings
- Security: Good — no injection/XSS/auth-bypass found; one data-integrity concern noted inline (client-trusted card metadata fallback).
- Performance: Good — the new HTTP client timeout is a solid addition; no N+1s or unbounded loops introduced.
- Correctness: Needs work — one confirmed uncaught-exception bug (see inline), several untested branches.
- Maintainability: Needs work — a trait-as-mixin pattern repeated across 4 new files (see inline comments).
Important — would fix before or shortly after merge
CaptureAliasPaymentRequestHandlerlets aLogicExceptionescape uncaught instead of failing the payment request gracefully, unlike its sibling handler in this same PR — see inline comment.- Four new traits (
AppliesPaymentCaptureOutcomeTrait,BuildsCommonPaymentContextTrait,PersistsPayplugCardTrait,ResolvesSelectedCardTrait) require their host classes to "just happen to" declare specific properties rather than being explicit, independently-testable injected collaborators — see inline comments on each. composer.json'spayplug/unified-plugin-coreconstraint was loosened (^1.0.1→1.*) instead of raised to the^1.1.0this PR's code actually requires (PaymentDto,PaymentOutput,AddressDto, etc. don't exist before 1.1.0) — see inline comment.
Worth a follow-up ticket
- Client-submitted card metadata (
last4/expiration/country/brand) is used as a fallback source of truth for persistedCardrecords, and the JS field names extracting it are explicitly flagged in-code as unverified against the real SDK response — see inline comments onhosted-fields_controller.jsandPersistsPayplugCardTrait.php. - Non-atomic duplicate-alias guard in
PersistsPayplugCardTrait(check-then-act, now reachable from two independent code paths for the same alias). - Test coverage gaps: the brand-precedence branch in
CardDataFromPaymentMethodExtractoris never distinctly exercised (every test uses an identical value on both sides of the fallback),PersistsPayplugCardTrait's null-customer guard is untested, and there's no Behat/end-to-end coverage at all for paying with a saved alias. - Naming inconsistency (pre-existing, not introduced here):
OrderAddressDtoFactoryandPayPlugPaymentDataCreatordo the same conceptual job — building API request data from a Sylius order — under different suffixes.
Minor
validityDateregex doesn't range-check month 01-12.- Casting an absent exp-month/year form field to
(int) '' → 0makes "missing" indistinguishable from a legitimately-fetched0downstream. - The new HTTP client timeout's comment undersells its scope — it now applies to every call through the client, not just the one best-effort enrichment call it cites.
notificationUrl's route changed from a fixed URL to a per-payment-request one inBuildsCommonPaymentContextTrait. Confirmed with the team this field has no effect on PayPlug's side regardless of value, so this isn't a functional regression — just now-silently-dead code that used to carry an explanatory comment. Worth a one-line note (or removing the assignment) so a future reader doesn't have to rediscover that.
Positive observations
CaptureHostedPaymentRequestCommandProvider::isAlreadyInFlight()correctly handles a customer switching to a different saved card after an earlier alias attempt, rather than replaying a stale one — and it's tested.IntegrationDescriptionProviderand the HTTP client timeout both cite concrete production incidents (a real 400 response; an unbounded best-effort call) rather than speculative hardening.- Happy-path and edge-case test coverage (malformed API responses, ownership mismatches, missing customer email) is thorough overall.
hdelaforce-payplug
force-pushed
the
feature/PRE-3585_create_uhf_aliasing
branch
3 times, most recently
from
September 1, 2026 16:38
603bed7 to
830d84e
Compare
hdelaforce-payplug
force-pushed
the
feature/PRE-3585_create_uhf_aliasing
branch
from
September 1, 2026 17:05
830d84e to
57cbfac
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
hdelaforce-payplug
force-pushed
the
feature/PRE-3585_create_uhf_aliasing
branch
from
September 2, 2026 09:59
57cbfac to
22b8be0
Compare
adumont-payplug
approved these changes
Sep 2, 2026
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
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.
Description
Motivation:
Related issue(s): Closes #
Type of Change
Checklist
Code Quality
Testing
Security & Ops