Fixing behavior of Mage_Catalog_Model_Resource_Product_Action::updateAttributes

[midlan]

Hello everyone,

look at this code:

Mage::getResourceSingleton('catalog/product_action')->updateAttributes(123, array('non_existing_attribute' => 'value'), 0);

If you execute it, you will end up with this error:

Error: Call to a member function getAttributeId() on bool

Such error is hard to debug in your running magento, because magento won't tell you which attribute is missing and caused it.
So I went to the method and I wanted to fix the behavior to raise exception that shows also the attribute name.

BUT there is already some detection for missing attributes:

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

Line 72 in 8c25b15

if (!$attribute->getAttributeId()) {

The problem is that most of the time, when attribute is not found, the $attribute = $this->getAttribute($attrCode); would be false which is not checked anywhere in the Mage_Catalog_Model_Resource_Product_Action::updateAttributes method and it raises the error I described above.

I looked into the code of getAttribute method and it seems to me that there shouldn't be situation that attribute object without id is returned instead of false (but I am not telling it could not happen). So my proposed solution is change the check like that:

$attribute = $this->getAttribute($attrCode);
if (!$attribute || !$attribute->getAttributeId()) {
    Mage::throwException('Product attribute "' . $attrCode . '" not found.');
}

Which is little BC break. Instead of non-catchable fatal error, it would still be error but catchable with meaningful message.

Other solution might be to change the condition like that

$attribute = $this->getAttribute($attrCode);
if (!$attribute || !$attribute->getAttributeId()) {
    continue;
}

Which is how it was originally probably meant. But the BC break is even worse, instead of non-catchable fatal error, it would be complete oposite - silently ignored.

[Sekiphp]

If you just fix the contiditon, the iteration of loop will be skipped and method will ends correctly, but now is provided fatal error.

So in my opinion, is more compatible (with current code) to throw Exception.