Fixed Mage_Catalog_Model_Product_Status::addSaleableFilterToCollection() does nothing

[fballiano]

Try to create a new order from backend, select the customer, the store view and the try to "add product", the products shown on the grid that appears should be the saleable products, but they're not, they're both enabled and disabled products.

Why?
In app/code/core/Mage/Adminhtml/Block/Sales/Order/Create/Search/Grid.php there's a call to

Mage::getSingleton('catalog/product_status')->addSaleableFilterToCollection($collection);

which is deprecated and empty:

    /**
     * Add saleable filter to Product Collection
     *
     * @deprecated remove on new builds
     * @param Mage_Eav_Model_Entity_Collection_Abstract $collection
     * @return $this
     */
    public function addSaleableFilterToCollection(Mage_Eav_Model_Entity_Collection_Abstract $collection)
    {
        return $this;
    }

This PR fixes all the calls to addSaleableFilterToCollection methods.

Question, I would also remove the method but it will for sure break some installations, should we keep it as it is? althoguh the functionality is broken since years?

[fballiano]

I've added to this PR the fix for Mage_Catalog_Model_Product_Status::addVisibleFilterToCollection(), which historically was doing exactly the same thing as addSaleableFilterToCollection(), but was commented and deprecated years ago.

[sreichel]

Maybe there is/was a reason be able to sell diabled products?

which is deprecated and empty:

There was some commented code in addSaleableFilterToCollection(), that does exactly the same as your PR. Re-add it?

$collection->addAttributeToFilter('status', ['in' => $this->getSaleableStatusIds()]);

[fballiano]

Maybe there is/was a reason be able to sell diabled products?

you can't anyway, the order won't go through

There was some commented code in addSaleableFilterToCollection(), that does exactly the same as your PR. Re-add it?

I don't think that's a good idea to have a class called "something_product_status" modify the product_collection, which is probable why it was removed and deprecated 3000 years ago.

[sreichel]

Agree with this pr, but i also like idea of $this->getSaleableStatusIds() ... easy to rewrite if you want to show disabled/invisible produtcs too.

[kiatng]

I am reluctant on this PR because it might break some sites. I remember a used case where backend user needed to add disabled product to order, something like this:

Even though there is a red warning that the product is disabled, the order goes through:

[addison74]

Maybe there should be an option in the store configuration that allows to create orders containing disabled products as well.

By default it should be disabled because when the decision is made to disable a product it is done for several solid reasons:

• it is still part of the product offer but is not available for a longer period of time
• it is not part of the product offer (removed, updated, ...)

In the first case, I should be able to create an order as an administrator even if I deliver it in say 3 months. It is like a pre-order, an agreement between me and the customer.

[fballiano]

I think that if "they" wanted to have disabled products in the order they would have removed the call to Mage_Catalog_Model_Product_Status::addSaleableFilterToCollection() instead of commenting its content.

I've found myself many times trying to filter disabled products from a collection and getting frustrated because every piece of code you can find on the internet tells you to use Mage_Catalog_Model_Product_Status::addSaleableFilterToCollection() which doesn't work.

It is true, somebody could want disabled products in orders, which is wrong btw, otherwise disabling a product has no meaning anyway, if you just want to hide it from the frontend just use the visibility. I also think that a disabled product shouldn't be "reordered" (using the reorder funcionality in the frontend). At the same time we could have many more people having to check thousands of disabled products while creating an order and get frustrated that openmage keep showing those products (which can't even be filtered out).

[elidrissidev]

Looks good to me, but how about using Mage::getSingleton('catalog/product_status')->getSaleableStatusIds() as a best of both worlds solution? It also seems that method was meant to be an alternative to the deprecated one.

Same thing can be said for Mage::getSingleton('catalog/product_status')->addVisibleFilterToCollection() which can be addressed in a later PR.

[fballiano]

Looks good to me, but how about using Mage::getSingleton('catalog/product_status')->getSaleableStatusIds() as a best of both worlds solution? It also seems that method was meant to be an alternative to the deprecated one.

Same thing can be said for Mage::getSingleton('catalog/product_status')->addVisibleFilterToCollection() which can be addressed in a later PR.

Good idea @elidrissidev, I've converted the code!

[elidrissidev]

I am reluctant on this PR because it might break some sites. I remember a used case where backend user needed to add disabled product to order, something like this: Even though there is a red warning that the product is disabled, the order goes through:

You will now be able to override catalog/product_status to change this behavior to your needs.

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit eb27bfa. ± Comparison against base commit 1641eab.

[leissbua]

app/code/core/Mage/Catalog/Model/Resource/Product/Collection.php

is adding a status filter:

You again apply same kind of filter to:

app/code/core/Mage/CatalogSearch/Model/Layer.php

This is colliding with app/code/core/Mage/Catalog/Model/Resource/Layer/Filter/Price.php:

As it is trying (not very smart) to replace any e.status with 1=1, creating an invalid mysql query.

I really cant tell why this is not creating trouble for others, but code speaks for itself.

[elidrissidev]

@leissbua Do you face this issue in the latest release? The duplicate filter was already removed #2662. Additionally, #2660 was opened afterwards to address the naive condition replacement.

Edit: weirdly enough, the condition is still present in HEAD It was removed from app/code/core/Mage/Catalog/Model/Layer.php, but a similar one is still present in app/code/core/Mage/CatalogSearch/Model/Layer.php

[leissbua]

This is happening in the latest release and all previous releases i checked (19.4.21):

https://github.com/OpenMage/magento-lts/blob/v19.5.0-rc1/app/code/core/Mage/CatalogSearch/Model/Layer.php#L64

https://github.com/OpenMage/magento-lts/blob/v19.5.0-rc1/app/code/core/Mage/Catalog/Model/Resource/Layer/Filter/Price.php#L120

[elidrissidev]

Can you share reproduction steps? although I don't run the recent OM versions in production, I've used many times locally including the search, but never came across this issue.

[leissbua]

Enable flat catalog, do a search, get an invalid query. I think code speaks for itself, same reason you removed the additional status in-Filter in catalog.

[elidrissidev]

Already did those exact steps, and even made sure the code is going through the isEnabledFlat condition and through the extra condition to no avail.

The query I get seems valid:

SELECT `e`.`entity_id`, `e`.`type_id`, `e`.`attribute_set_id`, `e`.`name`, `e`.`short_description`, `e`.`price`, `e`.`special_price`, `e`.`special_from_date`, `e`.`special_to_date`, `e`.`small_image`, `e`.`thumbnail`, `e`.`news_from_date`, `e`.`news_to_date`, `e`.`status`, `e`.`url_key`, `e`.`required_options`, `e`.`image_label`, `e`.`small_image_label`, `e`.`thumbnail_label`, `e`.`msrp_enabled`, `e`.`msrp_display_actual_price_type`, `e`.`msrp`, `e`.`tax_class_id`, `e`.`price_type`, `e`.`weight_type`, `e`.`price_view`, `e`.`shipment_type`, `e`.`links_purchased_separately`, `e`.`links_exist`, `e`.`open_amount_min`, `e`.`open_amount_max`, `price_index`.`price`, `price_index`.`tax_class_id`, `price_index`.`final_price`, IF(price_index.tier_price IS NOT NULL, LEAST(price_index.min_price, price_index.tier_price), price_index.min_price) AS `minimal_price`, `price_index`.`min_price`, `price_index`.`max_price`, `price_index`.`tier_price`
FROM `catalog_product_flat_1` AS `e`
INNER JOIN `catalog_product_index_price` AS `price_index` ON price_index.entity_id = e.entity_id AND price_index.website_id = '1' AND price_index.customer_group_id = 0
WHERE (e.status = 1) AND (e.status IN(1));

Ignoring the fact there is an additional redundant condition, since I'm trying to identify how the issue happens first.

[leissbua]

It is valid, because there is no entity_id in condition:

It replaces and that gives:

WHERE 1=1 AND 1=1

If you have another condition like:

WHERE
	(e.status = 1)
	AND (e.entity_id IN(256413, 322947, 256409, 256410, 322945, 322946))
	AND (e.status IN(1))

it ends as

WHERE
	1=1
        AND (e.entity_id IN(256413, 322947, 256409, 256410, 322945, 322946))
	1=1

-> Invalid query

[fballiano]

I think I did something about this "1=1" thing, which I really don't like, but that wasn't approved, I can't find it at the moment.

[elidrissidev]

I think I did something about this "1=1" thing, which I really don't like, but that wasn't approved, I can't find it at the moment.

Here #2660

[leissbua]

Ok, how do we want to handle the situation, from my pov this needs to be fixed / removed:

https://github.com/OpenMage/magento-lts/blob/v19.5.0-rc1/app/code/core/Mage/CatalogSearch/Model/Layer.php#L64

magento-lts/app/code/core/Mage/CatalogSearch/Model/Layer.php

Lines 64 to 66 in 112f5a0

	->addAttributeToFilter('status', [
	'in' => Mage::getSingleton('catalog/product_status')->getVisibleStatusIds()
	]);

[fballiano]

@leissbua can you try if it still happens with #2660?

[leissbua]

I agree that the 1=1 is just stupid/dumb code and could be resolved. The case that e.status gets added twice is bad and additonally duplicates the entity_id in case as well. As the status Filter has been removed in catalog/model/layer.php states that it clearly should be removed in the search layer as well.

[fballiano]

I agree that the 1=1 is just stupid/dumb code and could be resolved.

if you could give a review to #2660 that would be great, I want to see that horrendous "1=1" thing go away