Skip to content

#40216-Replacing nested foreach with array union (+) to reduce time complexity #40217

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.

Sign up for GitHub

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

Open
wants to merge 14 commits into
base: 2.4-develop
Choose a base branch
from
Open

#40216-Replacing nested foreach with array union (+) to reduce time complexity #40217

Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2c3a2c9
#40216-Replacing nested foreach with array_replace to reduce time com…
Oct 4, 2025
f3f5bc9
#40216 - array union seems to be better optimised from memory perspec…
Oct 4, 2025
85c13c6
#40216 - Moving the setting to outside of if to fix data setting issue
Oct 4, 2025
f8d70b6
#40216 - Moving the attribute value to right side to keep the left ar…
Oct 5, 2025
1ce65b5
Merge branch '2.4-develop' into Fixing-BigOComplexity-in-Eav
senthilengg Oct 7, 2025
072db63
#40216 - Reproduced WebApiFunctional Test errors & fixed
Oct 9, 2025
ab42d68
#40216 - Removing a confusing comment
Oct 9, 2025
48b92ff
Merge branch '2.4-develop' into Fixing-BigOComplexity-in-Eav
engcom-Hotel Oct 13, 2025
189dd72
Merge branch '2.4-develop' into Fixing-BigOComplexity-in-Eav
senthilengg Oct 14, 2025
48c2f62
#40217 - Introduced _setItemAttributeValues function and deprecate _s…
Oct 16, 2025
5062fef
Merge branch '2.4-develop' into Fixing-BigOComplexity-in-Eav
senthilengg Oct 17, 2025
409afc2
Merge branch '2.4-develop' into Fixing-BigOComplexity-in-Eav
senthilengg Oct 21, 2025
5399f76
Remove blank line in AbstractCollection.php
senthilengg Oct 21, 2025
cd2251d
Merge branch '2.4-develop' into Fixing-BigOComplexity-in-Eav
engcom-Dash Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 33 additions & 22 deletions app/code/Magento/Eav/Model/Entity/Collection/AbstractCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
* Copyright 2025 Adobe
* All Rights Reserved.
*/

namespace Magento\Eav\Model\Entity\Collection;
Expand Down Expand Up @@ -1233,8 +1233,27 @@ public function _loadAttributes($printQuery = false, $logQuery = false)
throw $e;
}

$attributeCode = $data = [];
$entityIdField = $entity->getEntityIdField();

foreach ($values as $value) {
$this->_setItemAttributeValue($value);
$entityId = $value[$entityIdField];
$attributeId = $value['attribute_id'];
if (!isset($attributeCode[$attributeId])) {
$attributeCode[$attributeId] = array_search($attributeId, $this->_selectAttributes);
if (!$attributeCode[$attributeId]) {
$attribute = $this->_eavConfig->getAttribute(
$this->getEntity()->getType(),
$attributeId
);
$attributeCode[$attributeId] = $attribute->getAttributeCode();
}
}
$data[$entityId][$attributeCode[$attributeId]] = $value['value'];
}

if ($data) {
$this->_setItemAttributeValue($data);
}
}
}
Expand Down Expand Up @@ -1303,32 +1322,24 @@ protected function _addLoadAttributesSelectValues($select, $table, $type)
/**
* Initialize entity object property value
*
* Parameter $valueInfo is _getLoadAttributesSelect fetch result row
* Parameter $valueInfo is [product_id => [attribute_code => value]]
*
* @param array $valueInfo
* @return $this
* @throws LocalizedException
*/
protected function _setItemAttributeValue($valueInfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

The method signature behavior has changed. Any plugin/extension overriding _setItemAttributeValue() will break.

Recommendations:

  • Create a new method _setItemAttributeValues() (plural) for batch processing
  • Keep _setItemAttributeValue() (singular) unchanged for BC
  • Add an automated test for the new method.

Copy link
Author

Choose a reason for hiding this comment

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

@engcom-Hotel I think you didn't notice its a protected function in an abstract class. Let me know if you still think we need to introduce new function for other reasons. I couldn't think of a scenario someone wants to override the under the hood Eav Object processing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @senthilengg,

Thanks for your reply!

but I am still thinks that this is the BiC change. Please go through with the below link:

https://developer.adobe.com/commerce/contributor/guides/code-contributions/backward-compatibility-policy/#modifying-the-default-values-of-optional-arguments-in-public-and-protected-methods

and let me know If I missed anything here.

Copy link
Author

@senthilengg senthilengg Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@engcom-Hotel Thank you for sharing the link. Its done!

{
$entityIdField = $this->getEntity()->getEntityIdField();
$entityId = $valueInfo[$entityIdField];
if (!isset($this->_itemsById[$entityId])) {
throw new LocalizedException(
__('A header row is missing for an attribute. Verify the header row and try again.')
);
}
$attributeCode = array_search($valueInfo['attribute_id'], $this->_selectAttributes);
if (!$attributeCode) {
$attribute = $this->_eavConfig->getAttribute(
$this->getEntity()->getType(),
$valueInfo['attribute_id']
);
$attributeCode = $attribute->getAttributeCode();
}

foreach ($this->_itemsById[$entityId] as $object) {
$object->setData($attributeCode, $valueInfo['value']);
foreach ($valueInfo as $entityId => $value) {
if (!isset($this->_itemsById[$entityId])) {
throw new LocalizedException(
__('A header row is missing for an attribute. Verify the header row and try again.')
);
}
// Run only once
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: If it "runs only once" why is there a foreach loop? This suggests _itemsById[$entityId] contains multiple objects, contradicting the comment.

Copy link
Author

@senthilengg senthilengg Oct 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@engcom-Hotel Without this loop during my manual functional testing and unit testing is working well and passed, I have tested with configurable, bundle and simple products. However without the loop its failing WebAPI Graphql MTF test so there could be a scenario the test script except this to run in a loop but I couldn't reproduce myself. Tried with Xdebug profile as well during the manual test.

To give you another perspective I couldn't think of a scenario where this _itemsById contains more than one product object under the same Id.

_itemsById was set here

magento2/app/code/Magento/Eav/Model/Entity/Collection/AbstractCollection.php

Line 1168 in f6aa682

if (isset($this->_itemsById[$object->getId()])) {
and if you notice it was fetched from DB. $object->getId() here being the entity ID from the DB can't be a duplicated. So to me it looks like a legacy code sitting there and push the object to the _itemsById array if the same id exists. And this is the reason why MTF Web API test also expecting something, similar however in real world, I couldn't think of a scenario happens like this and thats the reason for my comment that it will run only once.

In any case if you feel its better to remove the please do so. Because if the object contains multiple it should have affected the stores much more since it would have run O( abc) c being the number of objects inside _itemsById

And Thank you so much for the review. Appreciate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your analysis @senthilengg. I agrees with your point. But I have suggestion below to change the comment as below:

// _itemsById[$entityId] is always an array (typically with one element)
// Foreach handles edge cases where multiple objects share the same entity ID

Copy link
Author

Choose a reason for hiding this comment

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

@engcom-Hotel Its done!

foreach ($this->_itemsById[$entityId] as $object) {
$object->setData($object->getData()+$value);
}
}

return $this;
Expand Down