Setting 'show_out_of_stock' to 'No' has no effect

[koenner01]

Setting the value of 'Display Out of Stock Products' in the backend to 'No' does not have the effect desired. If you set this to 'No' you would expect that only products that are in stock are shown.

Preconditions

1. PHP7.0
2. Checked in both MG2.1.3 CE and MG2.1.4 CE

Steps to reproduce

1. Create a configurable product and set itself and all associated simples to out of stock
2. Set the value for 'Display Out of Stock Products' to 'No' in the store configuration
3. Go to a product list containing your configurable product

Expected result

1. The configurable product should not be shown

Actual result

1. The configurable product is shown

We traced this issue back to Magento\CatalogInventory\Helper\Stock.
In the function addIsInStockFilterToCollection a flag check is done on 'require_stock_items'. Because this flag is never set or used in Magento (wtf!), it causes the second parameter in addStockDataToCollection to always be false. This has the undesired effect that out of stock products are shown...

public function addIsInStockFilterToCollection($collection)
{
    $stockFlag = 'has_stock_status_filter';
    if (!$collection->hasFlag($stockFlag)) {
        $isShowOutOfStock = $this->scopeConfig->getValue(
            \Magento\CatalogInventory\Model\Configuration::XML_PATH_SHOW_OUT_OF_STOCK,
            \Magento\Store\Model\ScopeInterface::SCOPE_STORE
        );
        $resource = $this->getStockStatusResource();
        $resource->addStockDataToCollection(
            $collection,
            !$isShowOutOfStock && $collection->getFlag('require_stock_items')
        );
        $collection->setFlag($stockFlag, true);
    }
}

It is our opinion that the '&& $collection->getFlag('require_stock_items')' should be removed as it is never set or referenced in the entire Magento project. Removing this part also fixes the fact the out-of-stock products are shown

[orlangur]

git blame shows it was introduced in c966dc1 and this flag probably exists in another edition only (thus should be treated as always true here).

[Corefix]

I can verify this is still a problem, it should properly just change && to ||.

I created a PR that seems to fix this for me: #8736

[koenner01]

I don't think the value should be changed to an 'or' because the flag that's being referenced does not exist in the CE edition. Like @orlangur said, it would be better to replace the flag value with true or just remove it all together ?

The fix we are currently using which works, is:

public function addIsInStockFilterToCollection($collection)
{
    $stockFlag = 'has_stock_status_filter';
    if (!$collection->hasFlag($stockFlag)) {
        $isShowOutOfStock = $this->scopeConfig->getValue(
            \Magento\CatalogInventory\Model\Configuration::XML_PATH_SHOW_OUT_OF_STOCK,
            \Magento\Store\Model\ScopeInterface::SCOPE_STORE
        );
        $resource = $this->getStockStatusResource();
        $resource->addStockDataToCollection(
            $collection,
            !$isShowOutOfStock
        );
        $collection->setFlag($stockFlag, true);
    }
}

[Corefix]

@koenner01 No but I think the flag is valuable in some use cases, where you would want to limit what your collection contains to products in store, and this is then an easy way.

[koenner01]

But is it not weird and confusing to leave a flag in the code that isn't referenced anywhere in the entire CE ?

[Corefix]

No I don't think so. Their are lots of things in CE not used anywhere the easiest thing I could think of would be lots of the events fired in Magento (I know this isn't an event).

Also if EE or ECE base code starts being different from the CE project it will be super confusing for extension providers / SI's.

[koenner01]

Doesn't it make more sense then that we should set the flag value to true if its value is non existent ?
That way the flag is still being referenced (and open for use in other classes) but still doesn't block the stockFilter function.

[orlangur]

@koenner01, dunno what's the reasoning behind PR acceptance (just a quick-fix as a temporary solution?).

I believe c966dc1 is simply wrong from modularity perspective and should be done as plugin/interceptor or any other way of extending CE behavior NOT residing in CE repository but in another one where all operations with this flag occur.

[eInyzant]

I think @koenner01 is true : If the flag is NULL then it means that it has not been set before. Then I think the best way is to add a default value to true.

[aholovan]

Hello,
@Corefix, @koenner01 have You guys tested the fix on 2.1.6, 2.1.7? With this fix I've got 500 error on Cart when some simple product of configurable is in Quote and on that moment is out of stock.

PHP Fatal error: Uncaught Error: Call to a member function getPrice() on null in ../vendor/magento/module-configurable-product/Model/Product/Type/Configurable/Price.php:43\nStack trace:\n#0 ../vendor/magento/module-catalog/Model/Product.php(554)

[hostep]

@aholovan: #5519 (comment)