Skip to content

Suggestion: Move caching of source models into AbstractSource #20068

Open
@schmengler

Description

@schmengler

Summary (*)

The EAV source model base class \Magento\Eav\Model\Entity\Attribute\Source\AbstractSource contains a protected attribute $_options that is meant to cache the options. Unfortunately this is not enforced, instead each implementation of getAllOptions() is responsible to use that cache properly.

EAV source models often load possible values for attributes from external resources. Every access to those values, e.g. with $product->getAttributeText() calls getAllOptions() on the source model. So it makes sense that the result of this method should only be calculated once. As far as I see, all core source models already do this, using $_options, but it is easy to miss in custom implementations, which can have significant performance impact.

Examples (*)

A typical "correct" implementation of getAllOptions():

    public function getAllOptions()
    {
        if (empty($this->_options)) {
            $materials = $this->materialRepository->getList($this->searchCriteriaBuilder->create())->getItems();
            foreach ($materials as $material) {
                $this->_options[] = ['value' => $material->getId(), 'label' => $material->getTitle()];
            }
        }
        return $this->_options;
    }

A typical problematic implementation:

    public function getAllOptions()
    {
        $options = [];
        $materials = $this->materialRepository->getList($this->searchCriteriaBuilder->create())->getItems();
        foreach ($materials as $material) {
            $options[] = ['value' => $material->getId(), 'label' => $material->getTitle()];
        }
        return $options;
    }

Proposed solution

Move caching as default behavior into AbstractSource:

    public function getAllOptions()
    {
        if (empty($this->_options)) {
            $this->_options = $this->loadOptions();
        }
        return $this->_options;
    }
    abstract protected function loadOptions() : array;

Implementations then override loadOptions() instead of getAllOptions(). The core source models should lead by example.

BC considerations

To maintain backwards compatibility, loadOptions() can be optional instead:

    protected function loadOptions() : array
    {
        return [];
    }

This way existing implementations, that override getAllOptions() still work.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Component: EavIssue: ConfirmedGate 3 Passed. Manual verification of the issue completed. Issue is confirmedPriority: P3May be fixed according to the position in the backlog.Progress: ready for devReproduced on 2.2.xThe issue has been reproduced on latest 2.2 releaseReproduced on 2.3.xThe issue has been reproduced on latest 2.3 releaseSeverity: S3Affects non-critical data or functionality and does not force users to employ a workaround.help wanted

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions