Each entity type has a custom storage class with custom load methods, but they've grown apart over time. Let's make sure they're named consistently. The first step was #2906026: Rename PaymentStorage::loadMultipleForOrder to loadMultipleByOrder.

Incomplete list:
PromotionStorage: loadAvailable() -> this method accepts an $order_type and $store. The order type is loaded for no reason, we only need its ID. Pass it the full order instead, which leaves room for expansion in the future (using the order's placed date, for example).
LogStorage::loadByEntity -> needs to be loadMultipleByEntity().
CouponStorage::loadByCode -> should be loadEnabledByCode. We can also add a separate pure loadByCode().

Debatable: PaymentGatewayStorage::loadMultipleForOrder($order) sounds better as loadApplicable($order) but I'm worried that loadForUser($account) then makes less sense.

CommentFileSizeAuthor
#5 interdiff-3-5.txt4.2 KBczigor
#5 commerce-2906218-storage_load_method_cleanup-5.patch28.95 KBczigor
#3 interdiff-2-3.txt614 bytesczigor
#3 commerce-2906218-storage_load_method_cleanup-3.patch31.21 KBczigor
#2 commerce-2906218-storage_load_method_cleanup-2.patch31.29 KBczigor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz created an issue. See original summary.

czigor’s picture

czigor CreditAttribution: czigor at Centarro commented
Assigned: bojanz » czigor
Status: Active » Needs review
FileSize
commerce-2906218-storage_load_method_cleanup-2.patch31.29 KB

Let's see what the testbot thinks about this one.

czigor’s picture

czigor CreditAttribution: czigor at Centarro commented
FileSize
commerce-2906218-storage_load_method_cleanup-3.patch31.21 KB
interdiff-2-3.txt614 bytes

Fixing docblock.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Needs work 
diff --git a/modules/promotion/src/PromotionStorageInterface.php b/modules/promotion/src/PromotionStorageInterface.php
index 5690751..e503419 100644
--- a/modules/promotion/src/PromotionStorageInterface.php
+++ b/modules/promotion/src/PromotionStorageInterface.php
@@ -2,9 +2,8 @@
 
 namespace Drupal\commerce_promotion;
 
-use Drupal\commerce_order\Entity\OrderTypeInterface;
-use Drupal\commerce_store\Entity\StoreInterface;
 use Drupal\Core\Entity\ContentEntityStorageInterface;
+use Drupal\commerce_order\Entity\OrderInterface;
 
 /**
  * Defines the interface for promotion storage.
@@ -22,6 +21,6 @@ interface PromotionStorageInterface extends ContentEntityStorageInterface {
    * @return \Drupal\commerce_promotion\Entity\PromotionInterface[]
    *   The available promotions.
    */
-  public function loadAvailable(OrderTypeInterface $order_type, StoreInterface $store);
+  public function loadAvailable(OrderInterface $order);
 
 }

at a glance, it looks like you didn't update the @params in the docblock?

 -      $variation = $variation_storage->loadFromContext($entity);
+      $variation = $variation_storage->loadByContext($entity);

We should not be making this change. Context is not a field on the variation, so the By rule doesn't make sense

czigor’s picture

czigor CreditAttribution: czigor at Centarro commented
Status: Needs work » Needs review
FileSize
commerce-2906218-storage_load_method_cleanup-5.patch28.95 KB
interdiff-3-5.txt4.2 KB

Addressed the issues above.

  • bojanz committed f009693 on 8.x-2.x authored by czigor 
    Issue #2906218 by czigor, bojanz: Clean up storage load methods
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Fixed

Thanks!

Status: Fixed » Closed (fixed)

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