Steps to reproduce:

  1. Have at least 2 payment gateways and an order.
  2. Go to /admin/commerce/orders/{commerce_order}/payments/add.
  3. Focus the Amount field and change the default value.
  4. Without unfocusing the Amount field, click directly on a different Payment Option than the default one.
  5. Do this multiple times and observe an AJAX race between the Amount field and the Payment Option field; if the Amount field finishes its call first, then the new value will be kept; otherwise, the new value will be lost.

The same happens with the Transaction Type field. On slow enough connections you can trigger a three-way race.

Proposed solution:

The AJAX behaviours for the Amount and the Transaction Type fields were introduced in #3538209: Provide a new "Add payment" form. I don't see any reason for them since no other field on the form depends on them, so I propose removing them. If they need to stay (maybe accomodating some kind of iframe gateways?), then split away Amount & Received & Transaction Type from the commerce_payment_details fieldset. Actually, if you take into account that sites might have custom payment fields on the form, then splitting these up might be a better choice to accomodate custom weights and custom grouping.

Issue fork commerce-3613653

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ts.ag created an issue. See original summary.

velmir_taky made their first commit to this issue’s fork.

velmir_taky opened merge request !727

velmir_taky’s picture

velmir_taky commented
Status: Active » Needs review

Confirmed the race and went with removing the AJAX (option 1).

Nothing in the rebuild actually depends on amount or transaction_type: they're only read in submitForm(), the order summary is rendered from the order entity (not from the entered amount), and no template/preprocess touches them. Only payment_option changes the form (gateway => transaction type/received visibility + the inline method form), so it keeps its AJAX. With a single field triggering the refresh, the concurrent requests that dropped the typed amount are gone.

Left the fields inside the fieldset for now — splitting Amount/Received/Transaction type out (option 2) is a fair follow-up if we want custom weights/grouping, but it's a larger change and not needed to close the race.

DefaultPaymentAdminTest still covers the submit path. Added a small FunctionalJavascript test that locks the behaviour the race broke: enter a custom amount, switch the payment option, amount is still there after the rebuild. It's a regression guard for the value persistence, not a timing reproducer.

velmir_taky’s picture

velmir_taky commented
Related issues: +#3614106: ProductLayoutBuilderIntegrationTest::testProductWithoutVariationsDoesNotCrash fails on Drupal 11.4 (Views UI wizard AJAX)
jsacksick’s picture

jsacksick commented

There was a reason for these fields to be ajaxified, pretty sure of that? Perhaps for the gateway plugins to react / get the updated transaction type and amount?

velmir_taky’s picture

velmir_taky commented

Yeah, I went digging for that specifically before pulling the #ajax. Couldn't find anything relying on it.

On the new form the gateway bit is the add-payment-method inline form (PaymentMethodAddForm) - it just builds the payment method plugin form, never touches amount or transaction_type. Grepped the whole payment module: the only readers of either are the two $form_state->getValue() calls in submitForm(). Nothing in a #process/build path looks at them. The summary is rendered off the order entity, and transaction_type's #access is keyed on the gateway plugin (SupportsAuthorizationsInterface) - which already flips via the payment_option rebuild.

All three elements were pointing at the same generic ajaxRefreshForm, so the rebuild wasn't handing the gateway plugin anything it doesn't already get - the values are stashed in $form_state on every rebuild regardless.

If there's a gateway that genuinely needs to re-render on amount change (some hosted-field/iframe thing pre-authing on build), that's the case for option 2 - pull Amount/Received/Transaction type out of the commerce_payment_details fieldset and give them their own targeted #ajax wrapper. Didn't hit anything in core that needs it, but happy to go that route if you'd rather keep the reactivity.

jsacksick’s picture

jsacksick commented

@velmir_taky: If there is anything depending on that, it'd be on contrib. We're primarily testing/using the new form with Authorize.net and Cybersource.