search

Vinai / nicer-image-names

Magento extension to build catalog image file names from product attributes so they have neat descriptive names.
77 stars 31 forks source link

usesSource can be unsafe criteria for deciding to use getAttributeText #32

Open mpchadwick opened 8 years ago

mpchadwick commented 8 years ago

I spent a bunch of time debugging why this extension wasn't working for me (none of the attributes were actually pulling values).

It turns out that usesSource() will incorrectly return true on an attribute if getSource() was previously called on that attribute.

getSource() sets source model to default source model...

https://github.com/OpenMage/magento-mirror/blob/magento-1.9/app/code/core/Mage/Eav/Model/Entity/Attribute/Abstract.php#L381

_getDefaultSourceModel() returns eav/entity_attribute_source_table

https://github.com/OpenMage/magento-mirror/blob/d409dff20e992e97546568974399c456958299f9/app/code/core/Mage/Catalog/Model/Resource/Eav/Attribute.php#L336-L339

usesSource thinks that there's a source model...

https://github.com/OpenMage/magento-mirror/blob/d409dff20e992e97546568974399c456958299f9/app/code/core/Mage/Eav/Model/Entity/Attribute/Abstract.php#L397

This is probably a bug in Magento, but unfortunately it's quite common for 3rd party modules to incorrectly call getSource on attributes that don't have source models.

I first ran into an issue here, which was calling getSource() on everything it needed to send to GA (sku, name).

Next there was an SEO module that was calling getSource() on the name attribute. So it was being done by 2 different modules in the same project!

Not sure just yet what the best solution is, but might be nice to add some defense to detect for this situation. I'm willing to submit a PR.

philwinkle commented 8 years ago

Dang NomadMage # 2 speaker issuing a PR to NomadMage # 1 speaker. Doesn't get better than this.