Skip to content

Conversation

empiricompany
Copy link
Contributor

@empiricompany empiricompany commented Apr 9, 2025

Description

This PR fixes an issue where website-specific attribute values (stored in tables like customer_eav_attribute_website) are not loaded correctly for EAV attributes. The problem affects attributes like taxvat that should respect website-specific configurations.

Problem

The issue occurs in the EAV attribute loading process. When loading entity attributes in Mage_Eav_Model_Config::_loadEntityAttributes(), the code calls:

$entityAttributes = $entityType->newAttributeCollection()
    ->addStoreLabel($storeId)
    ->getData();

The newAttributeCollection() method in Mage_Eav_Model_Entity_Type calls _getAttributeCollection(), which creates a collection using:

$collection = Mage::getModel('eav/entity_attribute')->getCollection();

This always returns an instance of Mage_Eav_Model_Resource_Entity_Attribute_Collection, which does not have the setWebsite() method needed to load website-specific attribute values.

For entity types like 'customer' that have website-specific attributes, this means the website-specific values are never loaded, even though they exist in the database.

Proposed Solution

The solution has two parts:

1. Fix the attribute collection creation

Modify the _getAttributeCollection() method in Mage_Eav_Model_Entity_Type to use the entity's configured attribute collection class instead of always using the base collection:

protected function _getAttributeCollection()
{
    $collectionClass = $this->getEntityAttributeCollection();
    $collection = Mage::getResourceModel($collectionClass);
    
    $objectsModel = $this->getAttributeModel();
    if ($objectsModel) {
        $collection->setModel($objectsModel);
    }

    return $collection;
}

This leverages the existing getEntityAttributeCollection() method, which returns the collection class specified in the entity type's XML configuration. By default, it returns 'eav/entity_attribute_collection', but modules can override this for specific entity types.

2. Fix the attribute loading process with cache

Modify the _loadEntityAttributes() method in Mage_Eav_Model_Config to check for website-specific collections and set the website ID:

protected function _loadEntityAttributes($entityType, $storeId)
{
    // preload attributes in array form to avoid instantiating
    // models for every attribute even if it is never accessed
    $collection = $entityType->newAttributeCollection()
        ->addStoreLabel($storeId);

    // if collection supports per-website values, set website id
    if ($collection instanceof Mage_Eav_Model_Resource_Attribute_Collection) {
        $websiteId = Mage::app()->getStore($storeId)->getWebsiteId();
        $collection->setWebsite($websiteId);
    }

    $entityAttributes = $collection->getData();
    
    // ... rest of the method ...
}

Using instanceof instead of method_exists() is more reliable and efficient because:

  1. Type safety: It ensures we're only calling setWebsite() on collections that are designed to handle website-specific values
  2. Performance: Type checking is faster than method existence checking
  3. Clarity: It explicitly shows that only collections extending Mage_Eav_Model_Resource_Attribute_Collection support website-specific values
  4. Consistency: It follows object-oriented principles by relying on the type hierarchy rather than duck typing

Currently, only two collections in the core code implement the abstract class with setWebsite():

  • Mage_Customer_Model_Resource_Attribute_Collection
  • Mage_Customer_Model_Entity_Attribute_Collection (which extends the first one)

This is consistent with the database schema, as only the Customer module has a customer_eav_attribute_website table to store website-specific attribute values. Other entity types like Catalog do not have equivalent tables and their collections do not extend the abstract class with setWebsite().

For example, the Customer module can specify:

<entity_attribute_collection>customer/resource_attribute_collection</entity_attribute_collection>

With these changes, entity types that have website-specific attributes can provide their own attribute collection class that implements the setWebsite() method, and the attribute loading process will correctly use this method when caching attributes.

Diagram

flowchart TD
    A[Mage_Eav_Model_Config::_loadEntityAttributes] --> B[entityType->newAttributeCollection]
    B --> C[Mage_Eav_Model_Entity_Type::_getAttributeCollection]
    
    subgraph "Before Fix"
        C -->|Always creates| D["Mage.getModel(eav/entity_attribute).getCollection"]
        D -->|Returns| E[Mage_Eav_Model_Resource_Entity_Attribute_Collection]
        E -->|No setWebsite method| F[Website-specific values not loaded]
    end
    
    subgraph "After Fix"
        C2[Mage_Eav_Model_Entity_Type::_getAttributeCollection] -->|Uses configuration| G["this.getEntityAttributeCollection"]
        G -->|Returns collection class| H["Mage.getResourceModel(collectionClass)"]
        H -->|For customer entity| I[Mage_Customer_Model_Resource_Attribute_Collection]
        I -->|Has setWebsite method| J[Website-specific values loaded correctly]
    end
Loading

Benefits

  1. Fixes website-specific attribute values: Attributes like taxvat will now correctly respect website-specific configurations.
  2. Generic solution: Works for all entity types, not just customer.
  3. Configurable: Each module can specify its own attribute collection class.
  4. Respects Magento architecture: Uses the existing XML configuration system.
  5. Minimal change: Only modifies two methods in two classes.
  6. Type-safe implementation: Uses proper OOP principles with instanceof checks.
  7. Cache-aware: Ensures website-specific values are respected even when attributes are cached.

Testing

  1. Create a multi-website setup
  2. Configure different settings for the taxvat attribute on different websites
  3. Verify that the attribute respects the website-specific configuration
  4. Test with cache enabled to ensure website-specific values are still respected

Note: This solution requires thorough testing across different environments and configurations as it affects a core part of the EAV system. Comprehensive regression testing is strongly recommended to ensure no unintended side effects occur in other parts of the system that rely on attribute collections.

Backward Compatibility

This change is backward compatible as it:

  1. Does not change any public APIs
  2. Uses existing configuration mechanisms
  3. Falls back to the default behavior if no custom collection class is specified
  4. Only applies website-specific behavior to collections that explicitly support it

Related Issues

This fixes the following issues:

  • Issue #4732: Website-specific attribute values not loading correctly

@github-actions github-actions bot added the Component: Eav Relates to Mage_Eav label Apr 9, 2025
@empiricompany empiricompany requested a review from kiatng April 9, 2025 10:27
@empiricompany empiricompany self-assigned this Apr 9, 2025
@empiricompany

This comment was marked as outdated.

@empiricompany empiricompany marked this pull request as draft April 9, 2025 10:30
@empiricompany

This comment was marked as outdated.

@empiricompany empiricompany marked this pull request as ready for review April 9, 2025 10:59
Copy link

Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested.

@kiatng kiatng merged commit 9ff5c56 into OpenMage:main Apr 18, 2025
19 checks passed
fballiano added a commit to MahoCommerce/maho that referenced this pull request Apr 18, 2025
@sreichel sreichel added the bug label May 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Component: Eav Relates to Mage_Eav
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants