PRE-3553 refactor: Set UHF as standard payment option - #310

Review

Overview

This PR folds Unified Hosted Fields (UHF) into the standard payplug gateway as a display-mode option, rather than its own gateway factory (payplug_uhf/UhfGatewayFactory). It:

• Removes UhfGatewayFactory, UhfGatewayConfigurationType(Extension), and their dedicated services/twig hooks/translations.
• Adds a hostedFieldsMode radio (integrated_payment / hosted_fields / redirected-by-default) to PayPlugGatewayConfigurationTypeExtension, backed by two new persisted flags (INTEGRATED_PAYMENT, HOSTED_FIELDS) plus HF_IDENTIFIER/HF_SUB_MERCHANT_ID fields.
• Adds a shouldValidateBaseCurrency()/baseCurrencyViolationMessage() extension hook on AbstractGatewayConfigurationType so only integrated_payment mode enforces EUR-only channels.
• Updates PostPaymentSelectEventSubscriber, PaymentMethodValidator, IsCanSavePaymentMethodValidator, and shop templates to gate on the payplug factory + HOSTED_FIELDS config flag instead of a separate factory name.

The removal of the old gateway is thorough and clean — I checked the full branch tree (not just the diff) and found no stray references to UhfGatewayFactory / payplug_uhf left anywhere.

🔴 Critical: referenced templates don't exist

config/twig_hooks/admin.yaml adds three new hook entries:

hosted_fields_mode:
    template: '@PayPlugSyliusPayPlugPlugin/admin/payment_method/form/hosted_fields_mode.html.twig'
hf_identifier:
    template: '@PayPlugSyliusPayPlugPlugin/admin/payment_method/form/hf_identifier.html.twig'
hf_sub_merchant_id:
    template: '@PayPlugSyliusPayPlugPlugin/admin/payment_method/form/hf_sub_merchant_id.html.twig'

None of these three .twig files exist anywhere in the PR (checked the full branch tree — only deferred_capture.html.twig, fees_for.html.twig, live_checkbox.html.twig, one_click.html.twig, renew_oauth.html.twig exist under templates/admin/payment_method/form/). As it stands, opening the admin "create/edit PayPlug payment method" page will throw a Twig LoaderError (template not found) — this breaks the admin form entirely, not just the new fields.

🔴 Critical: referenced Stimulus controller doesn't exist

assets/package.json / assets/controllers.json register:

"hosted-fields-mode": {
  "main": "admin/controllers/hosted-fields-mode_controller.js",
  ...
}

There is no assets/admin/ directory at all in this repo — every existing controller lives under assets/shop/controllers/. This file was never added. PayPlugExtension::channelBaseCurrencies()'s docblock even says it "feeds the admin hosted-fields-mode Stimulus controller's client-side EUR-only gating," confirming the controller was planned but never committed. This will fail the asset build (Symfony UX Stimulus/Encore statically resolves the lazy-controller map) or, at best, 404 at runtime.

Together, these two gaps suggest the PR is missing its final commit(s) — the JS/Twig half of the "hosted fields mode" admin UI never made it in.

🟡 Test coverage gaps

• No test file exists (before or after this PR) for PayPlugGatewayConfigurationTypeExtension, and this PR adds substantial new logic there (POST_SET_DATA/POST_SUBMIT/SUBMIT listeners, translator injection, missingHostedFieldsRequirements error-adding). Worth at least one test verifying the pre-selection, the merge-back of resolveDisplayModeFlags into submitted data, and the added FormErrors.
• PayPlugGatewayFactory::resolveDisplayMode(), resolveDisplayModeFlags(), and missingHostedFieldsRequirements() are new pure static helpers with real edge cases (both flags true, blank vs. whitespace-only vs. non-scalar values) but have no direct unit tests — only indirectly exercised via PaymentMethodValidatorTest/PostPaymentSelectEventSubscriberTest.
• PayPlugExtension's three new Twig functions (displayMode, hostedFieldsCompanyId, channelBaseCurrencies) also have no test coverage (there's no PayPlugExtensionTest at all).

The PostPaymentSelectEventSubscriberTest updates themselves are solid — the new "different factory, coincidentally truthy config key" and "config key absent" tests are good, deliberate edge-case coverage.

🟢 Minor: PasswordType + always_empty: false exposes the value in page source anyway

->add(PayPlugGatewayFactory::HF_SUB_MERCHANT_ID, PasswordType::class, [
    ...
    'always_empty' => false, // avoids clobbering on blank resubmit
])

The comment explains why always_empty is disabled, but the net effect is that on the edit form, the stored SubMerchant ID is rendered into the value="" attribute of an <input type="password"> — visually masked but fully visible via view-source/devtools. If this field is meant to be treated as sensitive (hence PasswordType), this defeats that; if it's just an identifier (not a secret), TextType would be more honest about what protection is actually provided. Not blocking, just worth a conscious call either way.

🟢 Things done well

• IsCanSavePaymentMethodValidator's GATEWAYS_SKIP check is still correctly placed inside the try block after createForPaymentMethod() — this preserves a previously-hard-won fix (credential validation still runs for skip-listed factories) and wasn't regressed by removing the UHF entry.
• resolveDisplayModeFlags() always returning both keys explicitly (rather than only the "true" one) is a nice touch — it correctly clears a stale flag when an admin switches away from a previously-selected mode.
• shouldValidateBaseCurrency()/baseCurrencyViolationMessage() as protected hooks on the abstract type is a clean way to scope the EUR-only constraint to integrated_payment only, without duplicating the currency-check loop in PayPlugGatewayConfigurationType.
• Defensive ?? false additions in PaymentMethodValidator::processPayplug() guard against legacy configs missing the newer keys.

Summary

The refactor/removal of the standalone UHF gateway is clean and consistent, but the PR looks incomplete: the admin-side templates and Stimulus controller it wires up in config never got committed, which will break the admin payment method form page and/or the asset build. I'd block merge until those files are added (or the config/package.json entries are removed if they're meant for a follow-up PR), and would ask for at least basic unit coverage on the new form-extension listeners and PayPlugGatewayFactory helpers.

Review Update — PR #310 (new commits pushed)

New commits substantially expand the PR (+1223/−319 across 38 files now, was +489/−325 across 34). This addresses both critical blockers from the previous round and adds thorough test coverage.

✅ Previously-flagged blockers: resolved

• Missing admin templates — hosted_fields_mode.html.twig, hf_identifier.html.twig, hf_sub_merchant_id.html.twig are now present (two are renames of the old integrated_payment.html.twig/hf_identifier_default.html.twig, correctly updated to read config.hostedFieldsMode / config.hfIdentifier; the third is new). config/twig_hooks/admin.yaml no longer references anything that doesn't exist.
• Missing Stimulus controller — rather than adding the missing hosted-fields-mode_controller.js, the client-side EUR-gating feature was dropped entirely: assets/controllers.json/package.json no longer reference it, and PayPlugExtension::channelBaseCurrencies() (which fed it) was removed along with the ChannelRepositoryInterface dependency. Confirmed via the full branch tree that no dangling reference remains anywhere. This is a reasonable simplification — currency validation is still fully enforced server-side (shouldValidateBaseCurrency()); the only loss is a live client-side hint before submit, which isn't required for correctness.

🟢 Test coverage: now strong

New/updated test files: PayPlugGatewayFactoryTest, PayPlugGatewayConfigurationTypeExtensionTest, PayPlugGatewayConfigurationTypeExtensionFormSubmissionTest, PayPlugGatewayConfigurationTypeTest, PayPlugExtensionTest, plus updates to AbstractGatewayConfigurationTypeTest, PostPaymentSelectEventSubscriberTest, PaymentMethodValidatorTest, IsCanSavePaymentMethodValidatorTest. This closes every gap flagged last round:

• resolveDisplayMode() / resolveDisplayModeFlags() / missingHostedFieldsRequirements() each get direct unit coverage, including edge cases (both flags true, blank vs. whitespace-only, unknown display-mode string).
• PayPlugGatewayConfigurationTypeExtensionFormSubmissionTest is a genuinely valuable addition — it exercises the real Symfony form lifecycle via TypeTestCase (not mocks) and documents a real bug the author caught while writing it: a FormError added to a child during the parent's PRE_SUBMIT gets silently wiped because Form::submit() resets that child's own $errors right after, before POST_SUBMIT fires. Moving the conditional-requirement check to POST_SUBMIT fixes it, and the test pins the fix with both a "both fields blank → invalid" and "both filled → valid" case, plus separate coverage for the currency check (integrated_payment rejects non-EUR, hosted_fields doesn't) and radio pre-selection on edit. This is the kind of test that would have caught a real regression.
• PayPlugExtensionTest covers hostedFieldsCompanyId() including the missing-key fallback.

🟢 Small correctness polish

AbstractGatewayConfigurationType's currency-check listener now dedupes identical flash messages ($flashedMessages guard) before adding them to the flash bag — relevant because PayPlugGatewayConfigurationType::baseCurrencyViolationMessage() returns the same static string for every channel, so without the guard, N non-EUR channels would previously have queued N identical flash banners. Doesn't affect the generic per-gateway message (which still interpolates #channel_code# and stays distinct per channel), so no regression there.

🟡 Still worth a look (unchanged from last review)

PayPlugGatewayFactory::HF_SUB_MERCHANT_ID is still rendered via PasswordType with 'always_empty' => false. On the edit form this puts the stored value into the value="" attribute of a type="password" input — masked visually, but fully readable via view-source. If the field is meant to be treated as sensitive, this doesn't actually protect it; if it's just an identifier, TextType would be more honest. Not blocking, just a call worth making consciously.

Summary

Both critical issues from the prior review are fixed, and the added test suite is unusually rigorous — including a real Symfony-forms lifecycle bug caught and regression-tested along the way. This looks mergeable modulo the PasswordType/always_empty question above, which is a minor UX/security-clarity nit rather than a defect.