Problem/Motivation

`PaymentGatewayForm::dispatchFailedPaymentEvent()` assumes the current route always contains the `commerce_order` parameter. This isn’t always true, given that the form is inline, causing failed payment events to miss the order context and not to be dispatched.

Affected code:
- `modules/contrib/commerce/modules/payment/src/Plugin/Commerce/InlineForm/PaymentGatewayForm.php` reads `$this->routeMatch->getParameter('commerce_order')` as the sole source.

Steps to reproduce

1. Invoke the inline form inside a flow without `commerce_order` in the route.
2. Trigger a gateway failure.
3. Observe missing order context in the dispatched `FailedPaymentEvent`.

Proposed resolution

- Resolve the order via robust fallbacks:
1. Check `FormStateInterface::getBuildInfo()['callback_object']` and call `getOrder()` if available.

Issue fork commerce-3566256

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:

Comments

ahmad khader created an issue. See original summary.

ahmad khader’s picture

jsacksick’s picture

Status: Needs review » Needs work

I don't think the change is acceptable as is...Checking if a method_exists over an interface isn't really optimal... Isn't there a getEntity() method in your form? Isn't your form implementing the EntityFormInterface?

ahmad khader’s picture

Hi jsacksick,
Given that it's an inline form, I feel like we cannot assume that `getEntity()` will always return an order. The `getEntity()` method in my case returns `orderItem()`. One potential solution is to create an interface to verify if the callback_object form is an instance of this interface, and it's safe to use getOrder(), BTW Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\MultistepDefault (what fresh install use) already uses the getOrder function Alternatively, we could pass the order as a configuration parameter and access it using `$configuration->['order']`.

ahmad khader’s picture

StatusFileSize
new281.33 KB

This image shows a fresh install of the Kickstart, where the plugin form doesn't have getEntity() and doesn't implement the EntityFormInterface.
Fresh install kickstart

We could introduce a new interface, something like 'SupportsOrderEntity' with getOrder(), so that any form using an inline form payment gateway could implement it.

jsacksick’s picture

Ok so maybe what we can do is the following instead?

if ($form_state->getFormObject() instanceof CheckoutFlowInterface)?

The CheckoutFlowInterface has a getOrder() method.

ahmad khader’s picture

Hmmm, what if we removed them from CheckoutFlowInterface and let it extend the new interface?

  /**
   * Sets the current order.
   *
   * @param \Drupal\commerce_order\Entity\OrderInterface $order
   *   The order.
   *
   * @return $this
   */
  public function setOrder(OrderInterface $order): static;

  /**
   * Gets the current order.
   *
   * @return \Drupal\commerce_order\Entity\OrderInterface
   *   The current order.
   */
  public function getOrder();

Because not every form will have getPreviousStepId and getNextStepId (be checkoutflow form).
and we say if ($form_state->getFormObject() instanceof SupportsOrderEntity)?

ahmad khader’s picture

Status: Needs work » Needs review
StatusFileSize
new5.63 KB

I updated the logic. Let me know if that works for you.

jsacksick’s picture

Version: 3.2.0 » 3.x-dev
Status: Needs review » Needs work

hm... In theory yes, but feels weird for this new interface to have a setter...
Maybe we should go with your first commit... I feel more comfortable shipping with this workaround for now rather than trying to figure out where this interface should live, figuring the naming, the setter etc...

So let's just stick to the first version.

ahmad khader’s picture

Agreed!

ahmad khader’s picture

Status: Needs work » Needs review
jsacksick’s picture

Status: Needs review » Needs work

Tests are failing, several calls to dispatchFailedPaymentEvent() weren't updated. So this needs additional work.

ahmad khader’s picture

Status: Needs work » Needs review

jsacksick’s picture

Status: Needs review » Fixed

Merged, thank you!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.