Repository navigation
Conversation
jhoaraupp
left a comment
jhoaraupp
left a comment
There was a problem hiding this comment.
Review summary: approve once the CHANGELOG is fixed. Nothing here can lose or duplicate money.
Checks run locally (PR head on a scratch worktree, UPC 1.2.2 loaded ahead of a local 1.2.1 vendor):
- PHPUnit: full suite OK, 1071 tests.
OrderAddressDtoCreatorTestandPaymentCaptureContextBuilderTest: 60 tests OK. - The same
tests/PHPUnit/Upcrun against UPC 1.2.1 gives 28 errors and 9 failures, so the^1.2.2bump is required. - ECS: clean on both changed files.
- PHPStan: not conclusive locally (static reflection still reads the 1.2.1 vendor); to confirm on CI.
- Called
splitPhonedirectly on edge cases:0612345678on MQ gives+33612345678, a BL0690…mobile is kept in strict mode thanks to the GP retry,+33…on DE is kept in non-strict mode and dropped in strict mode, and0639…on YT gives+262…. All as expected. - Checked:
PaymentCaptureContextBuilderis only used by the Hosted Fields new-card and saved-card handlers, so the scope described is right and Oney is not affected.
PR title and description (no file to anchor this on):
- The title is cut off: "…to match the address countr…".
- The description has leftover pasted text: "A gen 1 new message (ctrl+End) ↓ territory (
0696…on MQ →+596…)". - Typo carried over from the template: "ffeature".
Not for this PR, but worth knowing: a national-format number is still read in the address country. For example 0612345678 on a DE address becomes +49612345678, a valid German landline. This was already the case before the PR.
Worth keeping: the same region is used for toE164 and isMobile, an unusable number gives [null, null] and never '', the named argument reads clearly at the call site, and the tests cover all 11 territories in both modes.
| The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | ||
| and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | ||
|
|
||
| ## [2.4.0] - Unreleased |
There was a problem hiding this comment.
[Medium] Add these entries under the existing ## [2.0.0] - Unreleased section instead of a new 2.4.0 one.
On develop the convention is to add to the 2.0.0 - Unreleased section (#333, #335 and #337 all did). This creates a second "Unreleased" heading with a new version number.
The ### Fixed entry also describes a bug in Hosted Fields, which merchants have never had: it is still listed under 2.0.0 / Added. Merchant release notes built from this would announce a fix for something that was never released.
Suggested fix: drop this section, add the UPC requirement under a ### Changed heading in 2.0.0, and either drop the Fixed line or fold it into the existing Hosted Fields entry:
-## [2.4.0] - Unreleased
-
-### Changed
-- Requires `payplug/unified-plugin-core` 1.2.2 or later
-
-### Fixed
-- Hosted Fields payments, ...
-
## [2.0.0] - Unreleased
+
+### Changed
+- Requires `payplug/unified-plugin-core` 1.2.2 or later| * @param bool $requireRegionMatch true: the phone number must belong to one of the address's | ||
| * phoneRegions(); false: any valid number is sent, formatted in its own country | ||
| */ | ||
| public function createBilling(OrderInterface $order, bool $requireRegionMatch = true): ?BillingDto |
There was a problem hiding this comment.
[Low] The default of requireRegionMatch is the opposite of what the only caller uses.
The only caller in prod (PaymentCaptureContextBuilder) passes false. A future caller that forgets the argument gets strict mode, which drops the phone without any error: exactly the bug this PR fixes.
Either option is fine:
- Keep
truefor backward compatibility, and add a line to the docblock saying that the Hosted Fields capture path usesfalse. - Or, if Oney stays out of this path (pending the ticket author), drop the parameter and always read in non-strict mode. That also removes about half of the new test cases.
Same for createShipping() at line 61.
There was a problem hiding this comment.
Done, défaut conservé à true (compatibilité), docblock précisant que le chemin Hosted Fields passe false. Pas de suppression du paramètre : le ticket PRE-3761 demande Oney en strict, ce que le paramètre permet.
| $common->shipping = $this->orderAddressDtoCreator->createShipping($order); | ||
| // Any valid phone number is sent, even from another country than the address. The | ||
| // creator's strict mode (requireRegionMatch: true) is there for a method that needs it. | ||
| $common->billing = $this->orderAddressDtoCreator->createBilling($order, requireRegionMatch: false); |
There was a problem hiding this comment.
[Info] The ticket's acceptance criterion "Oney: a foreign number is rejected" is still not covered.
The description already says so. I confirmed that this builder is only used by CaptureHostedPaymentRequestHandler and CaptureAliasPaymentRequestHandler, so Oney never goes through it. Let's leave the ticket open until the ticket author has decided on the Oney path (and so on whether the strict mode is worth keeping, see the comment on OrderAddressDtoCreator).
There was a problem hiding this comment.
Vu ce matin au daily avec le PM, le critère reste ouvert. Oney n'est pas strict aujourd'hui et on garde cet état
fc22bb2 to
64afcc3
Compare
Require UPC ^1.2.2 and send a valid phone number even when it belongs to another country than the billing or shipping address. A metropolitan French number entered on an overseas address is sent as French, for Hosted Fields and for the other payment methods.
64afcc3 to
59fd320
Compare
Description
Phone numbers are no longer dropped from the billing and shipping details sent to PayPlug in two cases:
+33…on a DE address) used to be rejected, becauseOrderAddressDtoCreatorcalled UPC'sPhoneHelperin strict mode.07 81 23 45 67on MQ) used to be read as a local number, judged invalid and sent asnull.Motivation: since UPC 1.2.2 (PRE-3751, payplug/unified-plugin-core#42),
PhoneHelper::toE164()andisMobile()accept an optionalbool $requireRegionMatch = true.OrderAddressDtoCreator::splitPhone()called them with two arguments, so it stayed strict: a number such as+33612345678on a DE address became[null, null]and the payment was created with no phone. This is the same regression PrestaShop hit in5.1.0 (PRE-3704, fixed by PRE-3752 / payplug-prestashop#141). Fixing it here before the next release from
develop.What changes
composer.json/composer.lock:payplug/unified-plugin-core^1.2.1→^1.2.2. The lock diff is the UPC entry and thecontent-hashonly (giggsey/libphonenumber-for-phpstays at 8.13.45).OrderAddressDtoCreator::createBilling()/createShipping(): new optional last parameterbool $requireRegionMatch = true. Constructor and DI are unchanged, and the default keeps the previous strictbehaviour for any other caller.
PaymentCaptureContextBuilder: passesrequireRegionMatch: falsefor billing and shipping.New
Upc\PhoneRegionResolver: the single source of the overseas rule, shared by both paths. For GP, MQ, RE, GF, YT, PM, BL, MF, NC, PF and WF it returns the address region first, then the regions sharing itsnumbering (BL ↔ MF ↔ GP), then
FR; any other country returns only itself. Same rule asgetPhoneNumberRegions()in payplug-prestashop#141. A genuine local number keeps its territory (0696…on MQ →+596…).OrderAddressDtoCreatorretries through that rule in strict and non-strict mode, with the same winning region fortoE164andisMobile. An unusable number gives no phone, never''.Legacy
PayPlugPaymentDataCreator::formatNumber()(libphonenumber) applies the same retry. Signature and return shape are unchanged, and foreign numbers are still accepted as before.CHANGELOG.md: "Requires UPC 1.2.2 or later" under### Changed, and one entry under### Fixed, both in the 2.0.0 sectionWhich code path serves which payment method
OrderAddressDtoCreator(new path): thepayplugmethod with Hosted Fields enabled, new card (CaptureHostedPaymentRequestHandler) and saved card (CaptureAliasPaymentRequestHandler).PayPlugPaymentDataCreator::formatNumber()(legacy path): Oney, Bancontact, Wero, Scalapay, Apple Pay, American Express, andpayplugwithout Hosted Fields.Foreign numbers were already accepted on the legacy path (verified by running
formatNumber()), so only Hosted Fields had that bug. The overseas case affected both paths.Scope, please read
OrderAddressDtoCreatoron Sylius, so that criterion is not covered by this PR.OneyInvalidDataRetrieverandOneyCompleteInfoDTO): they require a valid mobile number but do not check the region, so+33612345678on a DE address passes.A
07 81 23 45 67on MQ is refused by them.formatNumber()is shared, so Oney's payload goes through the new retry. In the checkout-form flow it changes nothing for Oney, because the validators refuse such a number before the payload is built (thevalidators were run; the checkout guard in
PaymentTypeExtensionwas read, not run). A payment that reachescreate()without that form was not traced.PhoneHelper(false), is pending the ticket author. Strict mode exists and is tested through theparameter, ready for that.
Type of Change
BC: none. PHP floor: unchanged (
^8.2). Behaviour: more valid phone numbers are now sent instead of being dropped.Checklist
Code Quality
Testing
sonarcloudCI job (to confirm on theCI)
Security & Ops
Testing notes
Automated (run in Docker, PHP 8.2)
OrderAddressDtoCreatorTest: 49 new cases. They cover the ticket's examples, non-strict, strict through the parameter, strict by default, the11 territories in both modes, a null or lowercase country, and shipping.
PaymentCaptureContextBuilderTest: checks that a foreign number is sent for billing and shipping.FRretry,FRfirst, stricttoE164orisMobile, default flipped tofalse, nostrtoupper) each make at least one test fail.