0

I have this code in a custom observer to check current products in quote for stockstatusses etc. But this code adds like 4 to 6 seconds to loading the checkout. I guess the way the data is retrieved is far from ideal, but I'm not sure how to fix it. Can anybody give advice to make this more lean?

    $quote = Mage::getModel('checkout/session')->getQuote();
    $cartItems = $quote->getAllItems();

    foreach ($cartItems as $item) {
        $product = Mage::getModel('catalog/product')->load($item->getProductId());
        $stock = Mage::getModel('cataloginventory/stock_item')->loadByProduct($product)->getQty();
        [...]
    }

Update: Thanks to your input the current code is getting along, but I have one snag I can't seem to figure out. The goal of this observer is to check whatever is in the cart, and check a few things, to alter the checkout process accordingly. The things it checks:

  • Quantity of stock
  • Weight
  • A custom boolean for alternative stock

The code is now:

$cartItems = Mage::getSingleton('checkout/cart')->getQuote()->getAllVisibleItems();

foreach ($cartItems as $item) {
    $stock = Mage::getModel('cataloginventory/stock_item')->loadByProduct($item->getProductId)->getQty();
    if ($stock < 1) { ... }
    if ($item->getCustomStock()) { ... }
    if ($item->getWeight() >= 30) { ... }
}

I still couldn't (figure out) to write this as a collection as in the suggested documentation, because I couldn't get the current cart contents to behave as a collection. Even so, the loading done in the loop now only gets the stock info of the selected product, so it is already way more efficient. The thing that I'm stuck at is the getCustomStock bit; it's a custom attribute boolean named custom_stock, but I guess this attribute isn't collected here. How would I add it / any other advice on this approach?

2 Answers 2

1

This is a very common performance issue that you are experiencing. I'll start with pointing you to the top 5 performance-hindering code issues identified by Magento's ECG a while back: http://info2.magento.com/rs/magentosoftware/images/Conquer_the_5_Most_Common_Magento_Coding_Issues_to_Optimize_Your_Site_for_Performance.pdf

Be sure to read the section labeled: SQL Queries Inside a Loop, which basically explains how model loading/saving/deleting within a loop is exponentially slower than proper usage of collections.


In this particular case, I suggest that the direction of resolution should be focused on the purpose and proper usage of an observer. The observed event should have been passed some data objects during the dispatch. These data objects should be used appropriately, and perhaps redundant loading may be eliminated from this case.

Furthermore, I don't think I understand why you are checking a quote for product stock status, as this should be something that Magento already handles. Consider checking the stock settings within the system configuration, as well as the product settings.

Additional insight would allow for more accurate answers and possibly identification of the real underlying issue.


Update

While I feel as though there should be some different method for handling the stock-checks (for example, "custom stock" perhaps be worked into a solution using some other stock settings -- such as allowing back-orders -- on an "applicable product"-basis), I'll still go ahead with providing some assistance with your current approach since I think it would be helpful in other areas.

Note: You should still review the docs: http://docs.magento.com/m1/ce/user_guide/catalog/inventory.html

First off, which event are you observing? The reason that I ask this is to know what data object we'll have available. If you are not able to use the $observer->getEvent()->..., and instead must resort to the session for the data, then you be able to improve efficiency with observing a more appropriate event.

I'm still trying to wrap my head around the underlying purpose of all this, and what sort of actions are taken for the triggering logic within the observer.

The good stuff

Rather than getting the collection of items in the cart and performing those checks within a loop, you should find better performance with collection filtering, but I'm going to take some different approaches for now.

Note that the getAllVisibleItems() is defined as:

    /**
     * Get array of all items what can be display directly
     *
     * @return array
     */
    public function getAllVisibleItems()
    {
        $items = array();
        foreach ($this->getItemsCollection() as $item) {
            if (!$item->isDeleted() && !$item->getParentItemId()) {
                $items[] =  $item;
            }
        }
        return $items;
    }

You have written within the: foreach ($cartItems as $item) {:

 $stock = Mage::getModel('cataloginventory/stock_item')->loadByProduct($item->getProductId)->getQty();

But we don't need that since we can: $item->getProduct() and then further: $item->getProduct()->getStockItem();

So you may replace:

 $stock = $item->getProduct()->getStockItem()->getQty();
 // $stock = Mage::getModel('cataloginventory/stock_item')->loadByProduct($item->getProductId)->getQty();

At this point, it seems you want to be able to get the custom_stock product attribute, which you mentioned is a boolean. There is a way to efficiently get a single attribute value from a product without screwing performance by loading an entire model, but I'd have to look that up. However, I still think there is a more efficient method to all of this.

Here is where I would find collection filtering to be most useful and performance-boosting.

Assuming you can obtain the IDs of the products, (typically a scenario for the member function of a collection: getAllIds(), but not sure in this case), you'll be able to do this:

$collection = Mage::getModel('catalog/product')->getCollection()
    ->addAttributeToSelect('custom_stock')
    ->addAttributeToSelect('weight')
    ->addAttributeToFilter('entity_id',array('in' => $productIds))
    ->addAttributeToFilter('custom_stock',true)
    ->addAttributeToFilter('weight', array('gte' => 30));

if ($collection->getSize()) { // now you can check stock < 1, if needed.  }
1
  • Thanks for the PDF, very useful and educational! I've updated my question with the goal and status after reading the PDF, could you maybe point me in the right direction as how to get that custom attribute as well?
    – Asitis
    Apr 16, 2018 at 15:50
0

It is slow to load models in a chain when you have only to read informations. Models are better when used to manage objects and properties. In this case I think you're not going to edit the product nor the stock item. If so, you could use a direct query.

$connection = Mage::getSingleton('core/resource')->getConnection('core_read');
foreach ($cartItems as $item){
    $query = "SELECT * FROM `cataloginventory_stock_item` WHERE `product_id` = :product_id ;"
    $bind = array('product_id'=>$item->getProductId());
    $results = $connection->fetchAll($query,$bind);
    // .... go on
}

You can improve speed further by avoiding the foreach at all (you can improve the style of this query example as you wish):

$resource = Mage::getSingleton('core/resource');
$connection = $resource->getConnection('core_read');
$quote_id = Mage::getModel('checkout/session')->getQuoteId();
$query = "SELECT * FROM `cataloginventory_stock_item` WHERE `product_id` IN (SELECT product_id FROM sales_flat_quote_item where quote_id = :quote_id )";
$bind = array('quote_id' => $quote_id);
$results = $connection->fetchAll($query, $bind);

But in this case you will skip control of products deleted after the quote was last updated.

7
  • 1
    This is absolutely incorrect and completely by-passes the purpose of the Magento Framework and it's functionalities. In addition to this not being acceptable development within the Magento Framework, the fetchAll() call is also listed as one of the top 5 performance-hindering issues -- see page 15 of info2.magento.com/rs/magentosoftware/images/…
    – df2k2
    Apr 16, 2018 at 14:00
  • @df2k2 What do you mean for incorrect? How can you say so while you do not have informations about the purpouse of the request, ie what it is needed to do after with the result and from what the observer is triggered? From page 15: for large result sets. 1 result (for standard stock) is not a large set.
    – Fra H
    Apr 16, 2018 at 14:02
  • Sorry if I have upset or offended you. It is not at all a personal issue with you, but I feel the need to highlight the importance of not using direct SQL calls.
    – df2k2
    Apr 16, 2018 at 14:08
  • If it was one result, why would you still use fetchAll? Isn't there a fetchOne?
    – df2k2
    Apr 16, 2018 at 14:09
  • @df2k2 I'm ok with that while they won't trigger different observers and so. But I still do not see it as incorrect as it perfectly fits this particular request. About fecthAll vs fetchOne: there are commercial modules for multistock, so there could be one result for each physical warehouse.
    – Fra H
    Apr 16, 2018 at 14:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.