If a cart has multiple expired promotions, they should be removed automatically. However, the following code block causes issues with this:

From commerce/modules/promotion/src/PromotionOrderProcessor.php:

    $coupons_field_list = $order->get('coupons');
    $constraints = $coupons_field_list->validate();
    /** @var \Symfony\Component\Validator\ConstraintViolationInterface $constraint */
    foreach ($constraints as $constraint) {
      list($delta, $property_name) = explode('.', $constraint->getPropertyPath());
      $coupons_field_list->removeItem($delta);
    }

If the promotions with delta 1 and 2 are expired, delta 1 gets removed first. Then, the deltas reset, so there is no longer a delta 2, and when the function attempts to remove the promotion at delta 2, this error gets thrown:

InvalidArgumentException: Unable to remove item at non-existing index. in Drupal\Core\TypedData\Plugin\DataType\ItemList->removeItem() (line 138 of /var/app/current/web/paper/core/lib/Drupal/Core/TypedData/Plugin/DataType/ItemList.php).

CommentFileSizeAuthor
#5 3257480-5.patch1.31 KBjsacksick

Issue fork commerce-3257480

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eheller created an issue. See original summary.

eheller’s picture

Status: Active » Needs review
eheller’s picture

Issue summary: View changes
jsacksick’s picture

The attached patch is making use of the filter() callback instead. The one thing that I dislike about the changes from the merge request is that we perform the same validation over and over for each of the coupons (granted you'd probably never have 10 coupons applied, but still makes sense to me to do the validation once as reperforming checks that were already performed sounds like a bad idea performance wise...

  • jsacksick committed 1c625d9 on 8.x-2.x
    Issue #3257480 by eheller, jsacksick: Properly support removing multiple...

  • jsacksick committed a7d7b6a on 3.0.x
    Issue #3257480 by eheller, jsacksick: Properly support removing multiple...
jsacksick’s picture

Status: Needs review » Fixed

Committed the patch from #5.

Status: Fixed » Closed (fixed)

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