Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In catalog/product model the clearInstance function doesn't clear typeInstanceSingleton #3392

Closed
ioweb-gr opened this issue Jul 19, 2023 · 5 comments

Comments

@ioweb-gr
Copy link
Contributor

Preconditions (*)

  1. OpenMage 20.0.20
  2. One Simple Product
  3. One configurable Product
  4. A script that works in a loop that processes products by initializing the catalog/product model once and then using clearInstance() / load($id) functions to save resources from always initializing the model.

Steps to reproduce (*)

  1. Load the catalog/product model first and store in variable
  2. Load a simple product by using the productModel->load function
  3. Use function isSaleable to trigger populating the typeInstanceSingleton
  4. Clear the productModel using produtModel->clearInstance()
  5. Load a configurable product by using the productModel->load function with the configurable product's id.
  6. Try to get the usedProducts function of the configurable product type instance inside the product

Sample script

<?php
/*
 * Copyright (c) 2023. IOWEB TECHNOLOGIES
 */


// Include Magento's bootstrap file
require_once 'app/Mage.php';
Mage::app();

// Get the first available simple product
$simpleProductId = Mage::getModel('catalog/product')->getCollection()
    ->addAttributeToFilter('type_id', 'simple')
    ->addAttributeToFilter('status', Mage_Catalog_Model_Product_Status::STATUS_ENABLED)
    ->getFirstItem()->getId();

$productModel = Mage::getModel('catalog/product');
$configurableModel = Mage::getModel('catalog/product_type_configurable');

$simpleProduct = $productModel->load($simpleProductId);
if ($simpleProduct) {
    // Check if the simple product is saleable
    if ($simpleProduct->isAvailable()) {
        echo "First available simple product is saleable." . PHP_EOL;
    } else {
        echo "First available simple product is not saleable." . PHP_EOL;
    }
} else {
    echo "No simple products found." . PHP_EOL;
}


// Get the first available configurable product
$configurableProductId = Mage::getModel('catalog/product')->getCollection()
    ->addAttributeToFilter('type_id', 'configurable')
    ->addAttributeToFilter('status', Mage_Catalog_Model_Product_Status::STATUS_ENABLED)
    ->getFirstItem()->getId();

if ($configurableProductId) {
    $configurableProduct = $productModel->clearInstance()->load($configurableProductId);
    // Fetch the configurable children
    $configurableChildren = Mage::getModel('catalog/product_type_configurable')
        ->getUsedProducts(null, $configurableProduct);

    if (count($configurableChildren) > 0) {
        echo "Configurable product children: " . PHP_EOL;
        foreach ($configurableChildren as $child) {
            echo "Child SKU: " . $child->getSku() . PHP_EOL;
        }
    } else {
        echo "No configurable children found for the first available configurable product." . PHP_EOL;
    }
} else {
    echo "No configurable products found." . PHP_EOL;
}

echo "Done." . PHP_EOL;

Where the output is

/httpdocs$ php test-typeinstance.php
First available simple product is saleable.
PHP Fatal error:  Uncaught Error: Call to undefined method Mage_Catalog_Model_Product_Type_Simple::getUsedProducts() in /httpdocs/app/code/core/Mage/Catalog/Model/Resource/Product/Type/Configurable/Attribute/Collection.php:152
Stack trace:
#0 /httpdocs/app/code/core/Mage/Catalog/Model/Resource/Product/Type/Configurable/Attribute/Collection.php(117): Mage_Catalog_Model_Resource_Product_Type_Configurable_Attribute_Collection->_addAssociatedProductFilters()
#1 /httpdocs/lib/Varien/Data/Collection/Db.php(595): Mage_Catalog_Model_Resource_Product_Type_Configurable_Attribute_Collection->_afterLoad()
#2 /httpdocs/app/code/core/Mage/Catalog/Model/Product/Type/Configurable.php(253): Varien_Data_Collection_Db->load()
#3 /httpdocs/app/code/core/Mage/Catalog/Model/Product/Type/Configurable.php(329): Mage_Catalog_Model_Product_Type_Configurable->getConfigurableAttributes()
#4 /httpdocs/test-typeinstance.php(43): Mage_Catalog_Model_Product_Type_Configurable->getUsedProducts()
#5 {main}
  thrown in /httpdocs/app/code/core/Mage/Catalog/Model/Resource/Product/Type/Configurable/Attribute/Collection.php on line 152

Expected result (*)

The correct TypeInstance is loaded for Configurable Product

Actual result (*)

  1. The simple product TypeInstance is loaded because it's already set in the productModel
@kiatng
Copy link
Contributor

kiatng commented Jul 20, 2023

The method clearInstance() is defined in the class Mage_Core_Model_Abstract:

image

Now, let's look at the code:

/**
* Clearing object for correct deleting by garbage collector
*
* @return $this
*/
final public function clearInstance()
{
$this->_clearReferences();
Mage::dispatchEvent($this->_eventPrefix . '_clear', $this->_getEventData());
$this->_clearData();
return $this;
}
/**
* Clearing cyclic references
*
* @return $this
*/
protected function _clearReferences()
{
return $this;
}
/**
* Clearing object's data
*
* @return $this
*/
protected function _clearData()
{
return $this;
}

It has only 4 statements. The first and third statements you can scroll down the above code snippet and see that it does nothing. Whereas the event catalog_product_clear is observed in

<catalog_product_clear>
<observers>
<inventory>
<class>cataloginventory/observer</class>
<method>removeInventoryData</method>
</inventory>
</observers>
</catalog_product_clear>

which is:

/**
* Remove stock information from static variable
*
* @param Varien_Event_Observer $observer
* @return Mage_CatalogInventory_Model_Observer
*/
public function removeInventoryData($observer)
{
$product = $observer->getEvent()->getProduct();
if (($product instanceof Mage_Catalog_Model_Product)
&& $product->getId()
&& isset($this->_stockItemsArray[$product->getId()])
) {
unset($this->_stockItemsArray[$product->getId()]);
}
return $this;
}

I would think that this issue is invalid.

For your use case:

// Get the first available configurable product
$configurableProductId = Mage::getModel('catalog/product')->getCollection()
    ->addAttributeToFilter('type_id', 'configurable')
    ->addAttributeToFilter('status', Mage_Catalog_Model_Product_Status::STATUS_ENABLED)
    ->getFirstItem()->getId();

The getId() part is unnecessary, can be simplified to just

// Get the first available configurable product
$configurableProduct = Mage::getModel('catalog/product')->getCollection()
    ->addAttributeToFilter('type_id', 'configurable')
    ->addAttributeToFilter('status', Mage_Catalog_Model_Product_Status::STATUS_ENABLED)
    ->getFirstItem();

And if you need the id, you can get it like so: $configurableProductId = $configurableProduct->getId();.

If you agree, I am closing this.

@kiatng kiatng closed this as completed Jul 20, 2023
@kiatng kiatng reopened this Jul 20, 2023
@kiatng
Copy link
Contributor

kiatng commented Jul 20, 2023

My bad. The methods _clearReference() and _clearData() exist in Mage_Catalog_Model_Product.

@ioweb-gr
Copy link
Contributor Author

@kiatng Hi, don't forget this is a sample code only just to replicate the issue. The real code is fetching all ids based on many rules and to speed things up the collection is limited to only some fields.

Later on for the filtered product list the product needs to be loaded fully.

So I know I can just get the first item without loading later but it wouldn't showcase the issue then :)

Even if clear instance didn't exist in the product model, as long as it causes a bug and not clearing fully the model as expected, it would still need to be overriden in the product model to perform the task correctly in my opinion

@ioweb-gr
Copy link
Contributor Author

Basically no matter the circumstances or how a product is loaded when invoking the type instance for a configurable product, if it returns the simple product type instance instead of the configurable one then it's a bug and not the expected behavior don't you agree?

kiatng added a commit to kiatng/magento-lts that referenced this issue Jul 20, 2023
fballiano added a commit that referenced this issue Jul 21, 2023
…t. (#3395)

* Fixed issue #3392 clearInstance() method in Mage_Catalog_Model_Product.

* PHPStan

* whitespace

* PHPStan

---------

Co-authored-by: Fabrizio Balliano <[email protected]>
@kiatng
Copy link
Contributor

kiatng commented Jul 21, 2023

Fixed in PR #3395

@kiatng kiatng closed this as completed Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants