Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
When using order items without any purchasable entities assigned to them - the "onCartOrderItemRemove" event handler leads to the following error message:
Error: Call to a member function label() on null in Drupal\commerce_log\EventSubscriber\CartEventSubscriber->onCartOrderItemRemove() (line 65 of /var/www/html/web/modules/contrib/commerce/modules/log/src/EventSubscriber/CartEventSubscriber.php)
Will create PR soon.
Comment | File | Size | Author |
---|---|---|---|
#14 | commerce_log_call_to_a-2905147-14.patch | 4.33 KB | mglaman |
Comments
Comment #2
duozerskhttps://github.com/drupalcommerce/commerce/pull/780
Comment #5
mglamanGreat catch, thanks!
Comment #6
mglamanThis causes an empty log message, due to no value for
purchased_entity_label
. We should pass the order item's label instead ofNULL
Comment #7
mglamanHere is a revised fix which uses the order item's label. Also, the
CartEvents::CART_ENTITY_ADD
does not fire unless there is a purchasable entity. However, it still firesCartEvents::CART_ORDER_ITEM_REMOVE
regardless if one is present. Using the order item label should solve all problems.The order item label is the purchasable entity's, based on
PR: https://github.com/drupalcommerce/commerce/pull/787
Comment #8
mglamanComment #9
duozerskI think this comment is copy/paste and should be rewritten.
Comment #10
mglamanCan you elaborate? I wrote that comment specifically for that method, to test that no log is generated. I did copy it for the removeItem test, which should have proper text.
The latter comment is wrong.
Comment #11
duozerskIndeed, the second comment is the one that is wrong... I just did the same error, copy/pasted from the wrong place when trying to provide more code context :)
Comment #12
mglamanWait, I forgot. It is valid. We do two operations here but only assert one log. That's why I copied it here.
There won't be a log for adding. But there will be for removal.
This helps document our functionality.
Comment #13
duozerskOh, right you are. Indeed, there are 2 operations there and only one of them is resulting in log entry... Still, it feels like the 2nd comment should have mention of that (that removal is generating a log entry) - currently it is really confusing to have the same comment for 2 different tests.
Comment #14
mglamanYeah, I'll remove it. We already explain it in the first method. Updated patch with simplified comment.
Comment #15
andypostLooks great and covered with tests
Comment #17
mglamanYay, thanks all! Committed.