New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Suggestion: Move caching of source models into AbstractSource #20068
Comments
Hi @schmengler. Thank you for your report.
Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:
where @schmengler do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?
|
Hi @engcom-backlog-nazar. Thank you for working on this issue.
|
@engcom-backlog-nazar Thank you for verifying the issue. Based on the provided information internal tickets |
Hi @davidverholen. Thank you for working on this issue.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions! |
Well, the core team decided against it (see #20087 (review)). It is still relevant because carelessly written extensions easily bring down performance in product exports. It would be easy and helpful to prevent those developer errors. |
What if someone uses the options list programatically and doesn't want to cache it? I mean, I'm writing a custom import script. And while looping products in the CSV, if some attribute value is missing, I create it. Problem is that if I try to get the "fresh" list of options of that attribute to get the id of the new created value.. well, I can't, cause I get the old cached list. I might end up doing it with SQL queries instead.. Here's the code that I'm using:
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions! |
You already have this problem with all existing implementations that follow best practice and do cache the options. If you explicitly need it uncached, you can always create a new instance of the source model to have an empty |
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 ofgetAllOptions()
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()
callsgetAllOptions()
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()
:A typical problematic implementation:
Proposed solution
Move caching as default behavior into
AbstractSource
:Implementations then override
loadOptions()
instead ofgetAllOptions()
. The core source models should lead by example.BC considerations
To maintain backwards compatibility,
loadOptions()
can be optional instead:This way existing implementations, that override
getAllOptions()
still work.The text was updated successfully, but these errors were encountered: