Bugfix: addIdFilter does not work on category collection when flat catalog is enabled

[markvds]

See it in action on the RSS page when Top Level Category RSS Feed is enabled and Flat Catalog Category is enabled.

Mage_Rss_Block_List::CategoriesRssFeed() calls addIdFilter($nodeIds). Mage_Catalog_Model_Resource_Category_Flat_Collection::addIdFilter() in turn calls $this->addFieldToFilter('entity_id', $condition);, but entity_id has become ambigious because of the join you add on the collection in the observer.

The only I could think of to fix this, was to extend Mage_Catalog_Model_Resource_Category_Flat_Collection and change the column reference to main_table.entity_id.

Actually, this is a bugfix for the core class which should have used main_table.entity_id and not for your extension :)

[Vinai]

Thank you for the contribution, much appreciated! I just want to take a little time to write a testcase for the error, and fully understand the patch before I merge. Cheers!

[markvds]

Maybe it's even easier if I give you an example:

The query that would have been executed without the patch:

SELECT `main_table`.`entity_id`, `main_table`.`level`, `main_table`.`path`, `main_table`.`position`, `main_table`.`is_active`, `main_table`.`is_anchor`, `main_table`.`url_key`, `main_table`.`name`, `main_table`.`is_anchor` 
FROM `catalog_category_flat_store_9` AS `main_table` 
    INNER JOIN `groupscatalog_category_idx` ON main_table.entity_id=groupscatalog_category_idx.entity_id AND groupscatalog_category_idx.group_id=0 AND groupscatalog_category_idx.store_id=9 
WHERE (is_active = '1') AND (entity_id IN('613')) 
ORDER BY name ASC

As you can see, there's a WHERE clause on entity_id. But that field exists in both catalog_category_flat_store_9 and groupscatalog_category_idx. So the only thing I do in the patch is adding main_table to the WHERE clause.

The query that gets executed with the patch applied:

SELECT `main_table`.`entity_id`, `main_table`.`level`, `main_table`.`path`, `main_table`.`position`, `main_table`.`is_active`, `main_table`.`is_anchor`, `main_table`.`url_key`, `main_table`.`name`, `main_table`.`is_anchor` 
FROM `catalog_category_flat_store_9` AS `main_table` 
    INNER JOIN `groupscatalog_category_idx` ON main_table.entity_id=groupscatalog_category_idx.entity_id AND groupscatalog_category_idx.group_id=0 AND groupscatalog_category_idx.store_id=9 
WHERE (is_active = '1') AND (main_table.entity_id IN('613')) 
ORDER BY name ASC

And I'll have a close look once you have the test ready. I have little experience with them :(

BTW I see that you have your own coding standard and I used Magento's

[Vinai]

Thanks. Regarding the coding standard, I'm moving over to Magento's, just took me 5 years to get used to it :)

[Vinai]

I also understand the issue, I just haven't looked at the core code that is responsible, again.

How about simply renaming the groupscatalog_*_idx.entity_is field to catalog_entity_id? I think that might be easier and might help avoid similar conflicts in future. It would also avoid adding another class rewrite.

[markvds]

You are completely right! Renaming to column will also solve the problem. It's just that I don't see that as 'simply' :)

Still I think it's a bug in the core, because 'this is just the way we do things in Magento': giving equal fields the same name if they represent the same data. And all other methods on the collection actually do add main_table.

Forget my patch and rename the column. And good luck with (finally!) getting used to the coding standard :)

[Vinai]

Fixed in latest commit https://github.com/Vinai/groupscatalog2/commit/49a834174981a91713dcd919ec19f0f237a7d14f by changing the column name from entity_id to catalog_entity_id

[markvds]

Great, thanks!