The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
While fixing the free product test @mglaman found that it could be shortened by using the wrapper returned from commerce_cart_order_refresh() but then we realized that wasn't returning good results. Tried tracking down why the EMW cache was stale and it turns out to be commerce_discount_commerce_cart_order_refresh() was taking a wrapper then passing in the value to rules_invoke_event('commerce_discount_order', $wrapper->value()); Which then on every rule re-wraps it back again. The controller cache deals with itself but somehow the wrapper doesn't deal with it's cache well with entity references.
Proposed resolution
It may be better to fix entity api by helping clear it's cache better or removing it's cache all together.
But simplest solution is to pass the wrapper in to the order.
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | order_wrapper_updating-2540234-16-pass.patch | 16.57 KB | joelpittet |
| #16 | order_wrapper_updating-2540234-16-tests-only.patch | 16.07 KB | joelpittet |
| #15 | order_wrapper_updating-2540234-15.patch | 2.65 KB | joelpittet |
| #13 | order_wrapper_updating-2540234-7.patch | 2.14 KB | joelpittet |
| #1 | wrapper_updating-2540234-1.patch | 518 bytes | joelpittet |
Comments
Comment #1
joelpittetComment #2
mglamanI think it's proper to pass the entire wrapper, since this is part of the hook_commerce_cart_order_refresh() implementation. After Commerce Discount works its magic other modules will be tossed through the following:
If we're bypassing the wrapper in the module, then we could be disrupting expectations in other modules.
Comment #3
joelpittet@mglaman thanks and FYI, #1343196: $wrapper->reset() should unset $wrapper->cache this entity api issue is somewhat related if we want/need to clear the cache of an EMW.
Comment #4
joelpittetBetter title
Comment #6
mglamanSince test bot doesn't like us, here is a build to test patch https://travis-ci.org/mglaman/commerce_discount-travis-/builds/72900108.
Which, after doing this, we should make a test for this!
Comment #7
mglamanHere is a patch that compliments #1 and provides wrapper test coverage for order refreshes.
EDIT: Travis build with both patches: https://travis-ci.org/mglaman/commerce_discount-travis-/builds/72933245
Comment #9
joelpittettestbot is being nice, retest
Comment #11
joelpittet#7 pickup!
Comment #12
joelpittetComment #13
joelpittetComment #15
joelpittetThat was tests only and expected to fail. Here is both the tests and the fix from #1
Comment #16
joelpittetThe tests only were using assertEqual but only passing in one argument. I refactored them and then did all the other ones to clean things up. Hope this works!
Comment #18
joelpittetOk this is committed and pushed to -dev. Thanks for the test @mglaman!