It looks like there was a recent change to ShippingMethod.php in commit ff6fe492 found here which changed the Condition evaluation behavior and led to this ticket. This change can cause conditions on shipping methods to evaluate differently than before which can be quite serious
https://git.drupalcode.org/project/commerce_shipping/-/commit/ff6fe492f8...
ConditionGroup::evaluate() method returns TRUE if no conditions are passed. If you then use "Only one condition must pass" as the Condition operator, an empty Shipment condition group will always return TRUE causing all evaluations of that shipping method's conditions to always return TRUE.
In our case, we offer free shipping on orders over $100. This is set as an Order condition. We also have a certain product type that we always offer free shipping on using a custom Order condition that we wrote in-house--so 2 order conditions either of which we intend to grant free shipping to the customer. When ConditionGroup::evaluate() runs against Order conditions, it returns a normal evaluation. But since we selected "Only one condition must pass", the empty Shipment condition group always returns TRUE thus causing all orders to receive free shipping since our shipping method does not have a condition under it (the expression would be true || false, which evaluates to true). Logically, you would think the lack of a condition means you just want that condition to be ignored, not assumed to be true.
Do away with the concept of 'order conditions' vs 'shipment conditions'. Logically, I think of the "Condition operator" field on the plugin config form to apply to all conditions regardless of whether it applies to the order or shipment. Making this distinction does not seem to really do anything except increase complexity and also inadvertently cause bugs like this. In ShippingMethod::applies(), all conditions are initially pulled from the plugin configuration. Then array_filter() gets ran twice to split this group up into shipment conditions and order conditions. Simply do away with the array_filter()s and evaluate all of them together.
After closer inspection, this would not work properly
Figure out the proper way forward
n/a
n/a
n/a
I'm on a bit of a time crunch so I cannot supply a proper patch at the moment which tests can run against but I hope I described the issue well enough. I can try and submit a patch soon when I have a bit more time available
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 3282369-8.patch | 1.24 KB | jsacksick |
Comments
Comment #2
cchoe1 commentedOkay I'm in a bit of a rush so I didn't fully comprehend what was going on with the condition evaluations. So I missed 1 crucial detail where the shipment conditions require a shipment entity to be passed while the order conditions require an order entity to be passed. This makes it a bit more of a tricky problem and this makes sense because a condition based on the shipment would require that specific shipment to be passed over for evaluation (since an order can have multiple shipments in some cases).
It seems like the quickest way to solve this would simply be to create a 2nd Free Shipping method and split the order conditions up and just use "All conditions must pass" for both shipping methods. This works but it does mess up an existing setup and I would say anyone who uses the "Only one condition must pass" should probably evaluate their shipping methods in case the logic has changed. I would probably go as far as to say that the "Only one condition must pass" should probably not even be used because unless you have a valid Shipment condition, it's probably not going to work properly.
Comment #3
cchoe1 commentedComment #4
cchoe1 commentedComment #5
cchoe1 commentedComment #6
unrealauk commentedHi everyone. I have faced this too. I can't understand why we need to divide all conditions into 2 groups.
It will be nice if someone can give more details about this decision.
I have checked this issue deeper. We can't use this code for the OR operator.
In case when $shipment_conditions is empty it always returns TRUE. This behavior is broken OR operator at all.
I have written the patch using only one condition group.
Comment #7
init90The issue leads to potential money loss, so I think we can treat it as critical.
The patch posted by @unrealauk fixed the problem for us.
Comment #8
jsacksick commentedWhat about the attached patch? If the conditions are empty it'll now return FALSE. The patch from #6 reverts the change from #3269908: Apply conditions operator between order and shipping conditions too, so I don't think that is the right approach.
Comment #10
jsacksick commentedWent ahead and committed the fix from #8 since the patch from #6 reverts #3269908: Apply conditions operator between order and shipping conditions too which is not desired.
With the patch from #8, if there's no shipments conditions, the shipments condition group will return FALSE. Same with the order conditions. Which means one condition group at least must have conditions and they need to evaluate to TRUE. There's code before that that checks if there are no conditions at all and in this case the method will always apply.