Product flat tables broken, ref #2603 #2658

sreichel · 2022-10-16T21:33:59Z

Preconditions (*)

19.4.x-latest
Sample Data installed
Flat product tables ON

Steps to reproduce (*)

ge to https://openmage.ddev.site/men/new-arrivals.htm

Actual result (*)

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '1=1 AND ( e.website_id = '1' ) AND ( e.customer_group_id = 0) AND (e.min_pric...' at line 2, query was: SELECT FLOOR((ROUND((e.min_price) * 1, 2)) / 100) + 1 AS `range`, COUNT(*) AS `count` FROM `catalog_product_index_price` AS `e`
 INNER JOIN `catalog_category_product_index` AS `cat_index` ON cat_index.product_id=e.entity_id AND cat_index.store_id=3 AND cat_index.visibility IN(2, 4) AND cat_index.category_id = '14' WHERE 1=1 1=1 AND ( e.website_id = '1' ) AND ( e.customer_group_id = 0) AND (e.min_price IS NOT NULL) GROUP BY FLOOR((ROUND((e.min_price) * 1, 2)) / 100) + 1 ORDER BY FLOOR((ROUND((e.min_price) * 1, 2)) / 100) + 1 ASC

#0 /var/www/html/lib/Varien/Db/Statement/Pdo/Mysql.php(104): Zend_Db_Statement_Pdo->_execute(Array)
#1 /var/www/html/app/code/core/Zend/Db/Statement.php(289): Varien_Db_Statement_Pdo_Mysql->_execute(Array)
#2 /var/www/html/lib/Zend/Db/Adapter/Abstract.php(480): Zend_Db_Statement->execute(Array)
#3 /var/www/html/lib/Zend/Db/Adapter/Pdo/Abstract.php(238): Zend_Db_Adapter_Abstract->query('SELECT FLOOR((R...', Array)
#4 /var/www/html/lib/Varien/Db/Adapter/Pdo/Mysql.php(502): Zend_Db_Adapter_Pdo_Abstract->query('SELECT FLOOR((R...', Array)
#5 /var/www/html/lib/Zend/Db/Adapter/Abstract.php(811): Varien_Db_Adapter_Pdo_Mysql->query('SELECT FLOOR((R...', Array)
#6 /var/www/html/app/code/core/Mage/Catalog/Model/Resource/Layer/Filter/Price.php(272): Zend_Db_Adapter_Abstract->fetchPairs(Object(Varien_Db_Select))
#7 /var/www/html/app/code/core/Mage/Catalog/Model/Layer/Filter/Price.php(152): Mage_Catalog_Model_Resource_Layer_Filter_Price->getCount(Object(Mage_Catalog_Model_Layer_Filter_Price), 100)

See WHERE 1=1 1=1 ... related code ... Mage_Catalog_Model_Resource_Layer_Filter_Price::_getSelect()

        // processing WHERE part
        $wherePart = $select->getPart(Zend_Db_Select::WHERE);
        $excludedWherePart = Mage_Catalog_Model_Resource_Product_Collection::MAIN_TABLE_ALIAS . '.status';
        foreach ($wherePart as $key => $wherePartItem) {
            if (strpos($wherePartItem, $excludedWherePart) !== false) {
                $wherePart[$key] = new Zend_Db_Expr('1=1');
                continue;
            }
            $wherePart[$key] = $this->_replaceTableAlias($wherePartItem);
        }

Before #2603

" FROM `catalog_product_index_price` AS `e`
 INNER JOIN `catalog_category_product_index` AS `cat_index` ON cat_index.product_id=e.entity_id AND cat_index.store_id=3 AND cat_index.visibility IN(2, 4) AND cat_index.category_id = '14' WHERE 1=1 AND ( e.website_id = '1' ) AND ( e.customer_group_id = 0) AND (e.min_price IS NOT NULL)"

$wherePart = {object[1]} 
 0 = {Zend_Db_Expr} "1=1"
$wherePartItem = "(e.status = 1)"

After:

" FROM `catalog_product_index_price` AS `e`
 INNER JOIN `catalog_category_product_index` AS `cat_index` ON cat_index.product_id=e.entity_id AND cat_index.store_id=3 AND cat_index.visibility IN(2, 4) AND cat_index.category_id = '14' WHERE 1=1 1=1 AND ( e.website_id = '1' ) AND ( e.customer_group_id = 0) AND (e.min_price IS NOT NULL)"

$wherePart = {object[2]} 
 0 = {Zend_Db_Expr} "1=1"
 1 = {Zend_Db_Expr} "1=1"
$wherePart = {string[2]} ["(e.status = 1)", "AND (e.status I..."]
 0 = "(e.status = 1)"
 1 = "AND (e.status IN(1))"

The text was updated successfully, but these errors were encountered:

elidrissidev · 2022-10-17T08:58:13Z

Good find! I swear Magento is gonna make me have trust issues :D

elidrissidev · 2022-10-17T09:18:27Z

The original status filter condition originates from:

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

Lines 441 to 447 in cc6317a

    
           protected function _initSelect() 
        
           { 
        
               if ($this->isEnabledFlat()) { 
        
                   $this->getSelect() 
        
                       ->from([self::MAIN_TABLE_ALIAS => $this->getEntity()->getFlatTableName()], null) 
        
                       ->where('e.status = ?', new Zend_Db_Expr(Mage_Catalog_Model_Product_Status::STATUS_ENABLED)); 
        
                   $this->addAttributeToSelect(['entity_id', 'type_id', 'attribute_set_id']);

fballiano · 2022-10-17T11:23:34Z

but why zend_db doesn't put an "AND" between the two "1=1", that's super weird

fballiano · 2022-10-17T11:26:29Z

and also why the "status" is being removed from the query? 😱

Winfle · 2022-10-17T11:51:27Z

Seems like this is very critical

elidrissidev · 2022-10-17T12:01:19Z

but why zend_db doesn't put an "AND" between the two "1=1", that's super weird

Because they are just replacing (e.status = 1) and AND (e.status IN(1)) by 1=1. Of course this is very naive because it assumes status condition is always at the beginning and therefore an AND is not needed.

fballiano · 2022-10-17T12:03:29Z

yes but anyway the "setPart()" gets an array of conditions and should put and "AND" between them

elidrissidev · 2022-10-17T13:39:34Z

Is anyone working on this or should I get my hand dirty?

fballiano · 2022-10-17T13:44:23Z

I was checkin before but had to stop for a couple of hours, i'll try again later

fballiano · 2022-10-17T16:16:41Z

PR sent :-)

sreichel · 2022-10-17T17:58:31Z

and also why the "status" is being removed from the query? 😱

Please correct me, but flat tables only includes enabled products.

Was it right to add status filter here?

fballiano · 2022-10-17T18:01:05Z

that's right, as far as I wrong, flat should only include enabled products

sreichel · 2022-10-17T18:24:16Z

Writing from smartphone and will test again later ...

Revert change to Catalog\Model\Product\Layer should fix it too. For flat tables status check isn't required, do we need it for eav tables?

fballiano · 2022-10-17T18:36:11Z

Writing from smartphone and will test again later ...

Revert change to Catalog\Model\Product\Layer should fix it too. For flat tables status check isn't required, do we need it for eav tables?

mmmm not exactly, we should check if flat is enabled in that point, so I guess it's more correct as it is now with the new PR I submitted. IMHO

sreichel · 2022-10-17T19:27:54Z

The original status filter condition originates from:

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

Lines 441 to 447 in cc6317a

protected function _initSelect()

{

if ($this->isEnabledFlat()) {

$this->getSelect()

->from([self::MAIN_TABLE_ALIAS => $this->getEntity()->getFlatTableName()], null)

->where('e.status = ?', new Zend_Db_Expr(Mage_Catalog_Model_Product_Status::STATUS_ENABLED));

$this->addAttributeToSelect(['entity_id', 'type_id', 'attribute_set_id']);

Flat table check already exists.

sreichel · 2022-10-17T19:29:49Z

but why zend_db doesn't put an "AND" between the two "1=1", that's super weird

Because they are just replacing (e.status = 1) and AND (e.status IN(1)) by 1=1. Of course this is very naive because it assumes status condition is always at the beginning and therefore an AND is not needed.

It is the first condition from initSelect(), not?

fballiano · 2022-10-17T19:35:06Z

ah ok, in that file yes, I was checkin app/code/core/Mage/Catalog/Model/Layer.php which you commented about before.

i'm updating the other PR

fballiano · 2022-10-17T19:37:01Z

@sreichel updated, but I'd still keep the changes to the "where" management which is wrong nevertheless

sreichel · 2022-10-17T20:22:48Z

Can we just revert changes that are not related to backend grids product visibility/status?

Everything else should address to a new issue/pr.

fballiano · 2022-10-17T20:26:56Z

But that was my first solution… and it worked…

fballiano · 2022-10-17T20:27:11Z

Can’t do it anyway now

sreichel · 2022-10-17T21:43:54Z

For flat tables OFF the query changed from ...

" FROM `catalog_product_index_price` AS `e`
 INNER JOIN `catalog_category_product_index` AS `cat_index` ON cat_index.product_id=e.entity_id AND cat_index.store_id=3 AND cat_index.visibility IN(2, 4) AND cat_index.category_id = '14' WHERE ( e.website_id = '1' ) AND ( e.customer_group_id = 0) AND (e.min_price IS NOT NULL)"

To ....

" FROM `catalog_product_index_price` AS `e`
 INNER JOIN `catalog_category_product_index` AS `cat_index` ON cat_index.product_id=e.entity_id AND cat_index.store_id=3 AND cat_index.visibility IN(2, 4) AND cat_index.category_id = '14'
 INNER JOIN `catalog_product_entity_int` AS `at_status_default` ON (`at_status_default`.`entity_id` = `e`.`entity_id`) AND (`at_status_default`.`attribute_id` = '96') AND `at_status_default`.`store_id` = 0
 LEFT JOIN `catalog_product_entity_int` AS `at_status` ON (`at_status`.`entity_id` = `e`.`entity_id`) AND (`at_status`.`attribute_id` = '96') AND (`at_status`.`store_id` = 3) WHERE (IF(at_status.value_id > 0, at_status.value, at_status_default.value) IN(1)) AND ( e.website_id = '1' ) AND ( e.customer_group_id = 0) AND (e.min_price IS NOT NULL)"

If confirmed please partially revert :(

fballiano · 2022-10-17T21:48:24Z

I don't know what partially revert means, linked PR already reverts that.

sreichel · 2022-10-17T21:50:54Z

I mean close #2659 and revert changes in Catalog\Model\Layer.php

fballiano · 2022-10-17T21:51:50Z

why revert the whole thing when we can just fix that one line?

fballiano · 2022-10-17T21:53:42Z

I still think #2659 is correct, I don't know how to partially revert a merged PR anyway.

sreichel · 2022-10-17T22:16:14Z

why revert the whole thing when we can just fix that one line?

Just revert changes to Catalog\Model\Layer.php ...

This introduced a bug that was not in before. Instead of making more changes with unpredictable impact (changed query for eav tables) i'd prefer to revert this part.

fballiano · 2022-10-17T22:20:26Z

done, but that's not the problem, the problem is that the setPart('where') is wrong.

sreichel · 2022-10-17T22:27:43Z

done, but that's not the problem, the problem is that the setPart('where') is wrong.

Not sure about "wrong" ... 1=1 in first place (as replacement for status) was not wrong, but it seems incomplete ...

If we can fix this issue with reverting some lines we should do it and take a look at this method in a seperate task.

fballiano · 2022-10-17T22:31:56Z

1=1 is not the point, setPart(where) is used in a wrong way, used this way it loses the "AND/OR" where type, that's why it has to be fixed.

and that's why 3 lines below the 1=1 there's the correct usage for the "$excludeJoinPart".

also, 1=1 looks like an uwful (but also pretty useless if written in a better way) to manage that, which is anyway done 3 lines below (probably it was written by multiple people in different times).

fballiano · 2022-10-17T22:45:09Z

The complete explanation: #2660

sreichel · 2022-10-18T00:17:53Z

IMHO ... the only problem is the change to Mage_Catalog_Model_Layer::prepareProductCollection()

This lines have been added ...

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

W/o nothing else has to be changed.

fballiano · 2022-10-18T00:23:27Z

You said to modify the other one

fballiano · 2022-10-18T00:24:00Z

And the setpart wrong implementation still stands

sreichel · 2022-10-18T00:24:32Z

You said to modify the other one

Then you got me wrong. Sorry.

fballiano · 2022-10-18T00:25:18Z

why revert the whole thing when we can just fix that one line?

Just revert changes to Catalog\Model\Product\Layer.php ...

This introduced a bug that was not in before. Instead of making more changes with unpredictable impact (changed query for eav tables) i'd prefer to revert this part.

…

sreichel · 2022-10-18T00:27:43Z

Catalog\Model*Product*\Layer.php was missleading? :(

fballiano · 2022-10-18T00:27:52Z

The original status filter condition originates from:

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

Lines 441 to 447 in cc6317a

protected function _initSelect()

{

if ($this->isEnabledFlat()) {

$this->getSelect()

->from([self::MAIN_TABLE_ALIAS => $this->getEntity()->getFlatTableName()], null)

->where('e.status = ?', new Zend_Db_Expr(Mage_Catalog_Model_Product_Status::STATUS_ENABLED));

$this->addAttributeToSelect(['entity_id', 'type_id', 'attribute_set_id']);

Flat table check already exists.

i don’t understand anymore

fballiano · 2022-10-18T00:28:30Z

Anyway it’s 1.30AM so I’ll pass

fballiano · 2022-10-18T10:22:32Z

prepareProductCollection

but that code is not checking if flat is enabled or not so it wouldn't work

sreichel added the bug label Oct 16, 2022

elidrissidev added bug confirmed and removed bug labels Oct 17, 2022

fballiano mentioned this issue Oct 17, 2022

Avoid filtering on product status if flat catalog is enabled #2659

Closed

fballiano mentioned this issue Oct 17, 2022

Rewrote the $select->setPart() usage because it looses the "where type" #2660

Closed

sreichel mentioned this issue Oct 19, 2022

Fixes #2658 #2662

Merged

4 tasks

sreichel closed this as completed in #2662 Oct 20, 2022

Product flat tables broken, ref #2603 #2658

Product flat tables broken, ref #2603 #2658

Comments

sreichel commented Oct 16, 2022 • edited Loading

Preconditions (*)

Steps to reproduce (*)

Actual result (*)

elidrissidev commented Oct 17, 2022

elidrissidev commented Oct 17, 2022

fballiano commented Oct 17, 2022

fballiano commented Oct 17, 2022

Winfle commented Oct 17, 2022

elidrissidev commented Oct 17, 2022

fballiano commented Oct 17, 2022

elidrissidev commented Oct 17, 2022

fballiano commented Oct 17, 2022

fballiano commented Oct 17, 2022

sreichel commented Oct 17, 2022 • edited Loading

fballiano commented Oct 17, 2022

sreichel commented Oct 17, 2022

fballiano commented Oct 17, 2022

sreichel commented Oct 17, 2022

sreichel commented Oct 17, 2022 • edited Loading

fballiano commented Oct 17, 2022

fballiano commented Oct 17, 2022

sreichel commented Oct 17, 2022

fballiano commented Oct 17, 2022

fballiano commented Oct 17, 2022

sreichel commented Oct 17, 2022

fballiano commented Oct 17, 2022

sreichel commented Oct 17, 2022 • edited Loading

fballiano commented Oct 17, 2022

fballiano commented Oct 17, 2022

sreichel commented Oct 17, 2022 • edited Loading

fballiano commented Oct 17, 2022

sreichel commented Oct 17, 2022

fballiano commented Oct 17, 2022

fballiano commented Oct 17, 2022

sreichel commented Oct 18, 2022

fballiano commented Oct 18, 2022

fballiano commented Oct 18, 2022

sreichel commented Oct 18, 2022

fballiano commented Oct 18, 2022

sreichel commented Oct 18, 2022

fballiano commented Oct 18, 2022

fballiano commented Oct 18, 2022

fballiano commented Oct 18, 2022

sreichel commented Oct 16, 2022 •

edited

Loading

sreichel commented Oct 17, 2022 •

edited

Loading

sreichel commented Oct 17, 2022 •

edited

Loading

sreichel commented Oct 17, 2022 •

edited

Loading

sreichel commented Oct 17, 2022 •

edited

Loading