Skip to content
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

Eav config cache followup for multi scope load #3044

Merged
merged 10 commits into from
Mar 9, 2023
Merged

Eav config cache followup for multi scope load #3044

merged 10 commits into from
Mar 9, 2023

Conversation

davidhiendl
Copy link
Contributor

@davidhiendl davidhiendl commented Feb 24, 2023

Due to the way Mage_Eav_Model_Entity_Type::getAttributeCollection reuses the collection and the fact that Attribute_Collection::addStoreLabel does not support adding multiple stores at once it's impossible to load multiple stores attributes during the same context (page request, cli call, cron job, ...).

This is a followup that adds a non-breaking bypass to this behaviour, allowing retrieval of a non-cache collection.

It also stops the API from sometimes loading more data then actually requested, due to the collection being modified (but this did not appear to break anything).

Related Pull Requests

#2993

Fixed Issues (if relevant)

  1. Fixes EAV Config Cache #2993 (comment)
  2. Fixes EAV Config Cache #2993 (comment)

Manual testing scenarios (*)

  1. Disable cache
  2. Create multiple store views (2+)
  3. Create order in demo shop
  4. Visit order page in admin

@github-actions github-actions bot added the Component: Eav Relates to Mage_Eav label Feb 24, 2023
@davidhiendl
Copy link
Contributor Author

@luigifab @darinda Thanks for reporting. Here is the PR if you need this right away.

luigifab
luigifab previously approved these changes Mar 3, 2023
Copy link
Contributor

@luigifab luigifab left a comment

Choose a reason for hiding this comment

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

Tested with PHP 8.0 and 8.2.

@davidhiendl
Copy link
Contributor Author

Is it possible to get this merged, would make it easier to get the changes for #3057 committed and into a PR?

@fballiano
Copy link
Contributor

@luigifab could you check it again?

@fballiano
Copy link
Contributor

Is it possible to get this merged, would make it easier to get the changes for #3057 committed and into a PR?

when we get another green light I'll merge it right away. I keep commenting stuff cause I hate when PRs go sleepy.

@fballiano fballiano merged commit 6020634 into OpenMage:20.0 Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Eav Relates to Mage_Eav
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants