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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

duozersk created an issue. See original summary.

duozersk’s picture

Assigned: duozersk » Unassigned
Status: Active » Needs review

  • mglaman committed 5ebd762 on 8.x-2.x authored by duozersk
    Issue #2905147 by duozersk: Commerce Log: Call to a member function...

mglaman credited mglaman.

mglaman’s picture

Status: Needs review » Fixed

Great catch, thanks!

mglaman’s picture

Status: Fixed » Needs work

This causes an empty log message, due to no value for purchased_entity_label . We should pass the order item's label instead of NULL

mglaman’s picture

Here 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 fires CartEvents::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


  /**
   * {@inheritdoc}
   */
  public function createFromPurchasableEntity(PurchasableEntityInterface $entity, array $values = []) {
    $values += [
      'type' => $entity->getOrderItemTypeId(),
      'title' => $entity->getOrderItemTitle(),
      'purchased_entity' => $entity,
      'unit_price' => $entity->getPrice(),
    ];
    return self::create($values);
  }

PR: https://github.com/drupalcommerce/commerce/pull/787

mglaman’s picture

Status: Needs work » Needs review
duozersk’s picture

Status: Needs review » Needs work
+++ b/modules/log/tests/src/Kernel/CartIntegrationTest.php
@@ -137,6 +171,37 @@ class CartIntegrationTest extends CommerceKernelTestBase {
+ /**
+   * Tests that a log is not generated when a non-purchasable entity added.
+   *
+   * The cart manager does not fire the `CartEvents::CART_ENTITY_ADD` event
+   * unless there is a purchasable entity.
+   */
+  public function testAddedToCartNoPurchasableEntity() {

I think this comment is copy/paste and should be rewritten.

mglaman’s picture

I think this comment is copy/paste and should be rewritten.

Can 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.

+++ b/modules/log/tests/src/Kernel/CartIntegrationTest.php
@@ -119,6 +128,31 @@ class CartIntegrationTest extends CommerceKernelTestBase {
+   * Tests that a log is not generated when a non-purchasable entity added.
+   *
+   * The cart manager does not fire the `CartEvents::CART_ENTITY_ADD` event
+   * unless there is a purchasable entity.
+   */
+  public function testAddedToCartNoPurchasableEntity() {

@@ -137,6 +171,37 @@ class CartIntegrationTest extends CommerceKernelTestBase {
+   * Tests that a log is not generated when a non-purchasable entity removed.
+   *
+   * The cart manager does not fire the `CartEvents::CART_ENTITY_ADD` event
+   * unless there is a purchasable entity.
+   */
+  public function testRemovedFromCartNoPurchasableEntity() {

The latter comment is wrong.

duozersk’s picture

Can 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.

Indeed, 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 :)

mglaman’s picture

Status: Needs work » Needs review
+++ b/modules/log/tests/src/Kernel/CartIntegrationTest.php
@@ -137,6 +171,37 @@ class CartIntegrationTest extends CommerceKernelTestBase {
+    $logs = $this->logStorage->loadByEntity($cart);
+    $this->assertEquals(1, count($logs));
+    $log = end($logs);

Wait, 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.

duozersk’s picture

Oh, 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.

mglaman’s picture

Yeah, I'll remove it. We already explain it in the first method. Updated patch with simplified comment.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Looks great and covered with tests

  • mglaman committed 7507595 on 8.x-2.x
    Issue #2905147 by mglaman: Use order item label for purchased entity...
mglaman’s picture

Status: Reviewed & tested by the community » Fixed

Yay, thanks all! Committed.

Status: Fixed » Closed (fixed)

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