* place an order, in my case with payment method commerce_cheque (but any should give this error)
* mark the cheque as paid
expected
* hook_commerce_payment_order_paid_in_full() should fire and (in my case) change order status
experienced
* hook does not fire
(wow, this was a nasty bug to track down...)
* payment changes $transactio->status from pending to success
* DrupalCommerceEntityController::save fires hook_commerce_payment_transaction_update() before it clears entity cache.
* So commerce_payment_order_balance($order) loads the old $transaction which is still pending
* order balance is calculated wrong
* and hook not fired
see attached patch:
hook invocation should come after cache update.
i think it should also come after releaseLock() - but a DrupalCommerceEntityController guru might consider this.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | entity-controller-save-1427780-11.patch | 4.54 KB | helior |
| #9 | entity-controller-save-test-1427780-9.patch | 3.33 KB | helior |
| #1 | 0001-fixed-1427780-DrupalCommerceEntityController-save-sh.patch | 1.21 KB | geek-merlin |
Comments
Comment #1
geek-merlinhere's the patch.
Comment #2
rszrama commentedJust to clarify here: the bug is with updating pending transactions to successful. Inserting a successful payment transaction directly will trigger this rule properly. That said, I'm interested in other solutions that don't involve changing the nature of the base controller unless absolutely necessary. For example, perhaps we just need to ensure that we've cleared the cache of payment transactions before we perform our save. : ?
Comment #3
geek-merlinryan,
think i get your point about being conservative about the controller.
i thought about that for a while and i'm sure the controller need be fixed:
look at the current controller::save
it does
* write
* invoke aftersave hook
* clear cache
* release lock
but
* write
* clear cache
* release lock
should be an atomic operation
so
* invoke aftersave hook
has to come after that.
so the controller is buggy and has to be fixed.
which my patch does, and fixes this issue and surely others to come.
to the point: we don's wanna invoke a hook while our cache is inconsistent.
Comment #4
rszrama commentedOk, I'll give it a close look ASAP and ost likely roll this into a quick 1.3 (i.e. it's important enough not to postpone for weeks). I talked it over with Damien at dinner, and I think he would agree with you.
Comment #5
damien tournoud commentedI'm fine with the change in the controller (
EntityAPIControllerclears the cache before invoking the hooks too); but we need a test for the initial bug report.Comment #6
mossy2100I hope this will be fixed soon! Thanks for working on it :)
Comment #7
rszrama commentedTagging this one as a must-fix for 1.3.
Comment #8
helior commentedBoom.
Comment #9
helior commentedHere's a patch for the just the test requested. (Does not contain the fix from #1)
Although this issue is revolved around the commerce_payment_transaction entity, it affects all commerce entities. I used the Commerce Product entity as my testing grounds as it is the simplest setup.
Comment #11
helior commentedAlrighties.. So now that we see that #9 fails without the patch, let's apply the patch from #1.
Comment #12
helior commentedCan you review this when you get a chance? I'm actually not sure if this is the "right" way to test this sort of thing, but hey, it's there :)
Comment #13
rszrama commentedFinally walked through this patch and the tests tonight with helior. Kudos to Axel for finding and fixing such a tricky issue, and thanks to Helior for figuring out a simple way to test for the behavior. I actually hadn't seen the technique of testing the test in a patch on its own before testing it alongside the fix, but it makes perfect sense now that I've seen it demonstrated. : )
Comment #15
gladiatorhl2 commentedI suppose that this issue is again the same one and I am not sure that it is related to commerce 1.9 or to commerce cheque. When the payment method commerce_cheque is used, hook_commerce_payment_order_paid_in_full() is not triggered.
Comment #16
gladiatorhl2 commentedI have found a problem. Sorry for the false alarm, it was not clear at all. I've spent a lot of time to fix this issue and test all the modules.