-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
Reduce getId calls #3007
Reduce getId calls #3007
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see question in code
@@ -273,8 +273,8 @@ protected function _removeErrorsFromQuoteAndItem($item, $code) | |||
$canRemoveErrorFromQuote = true; | |||
|
|||
/** @var Mage_Sales_Model_Quote_Item $quoteItem */ | |||
foreach ($quoteItems as $quoteItem) { | |||
if ($quoteItem->getItemId() == $item->getItemId()) { | |||
foreach ($quoteItems as $quoteItemId => $quoteItem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is the ID coming from in the Collection? As far as I see its build in most of the cases without the itemId as key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would an alternate be to actually code getItemId() instead of relying on the __call() method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that majority of collection have the "id" as key?
From lib/Varien/Data/Collection.php, there are some references for id used as key. But perhaps I'm wrong.
From the method load
of a collection, there is something about the id:
magento-lts/lib/Varien/Data/Collection/Db.php
Lines 583 to 592 in fc98af9
if (is_array($data)) { | |
foreach ($data as $row) { | |
$item = $this->getNewEmptyItem(); | |
if ($this->getIdFieldName()) { | |
$item->setIdFieldName($this->getIdFieldName()); | |
} | |
$item->addData($row); | |
$item->setDataChanges(false); | |
$this->addItem($item); | |
} |
magento-lts/lib/Varien/Data/Collection.php
Lines 359 to 365 in fc98af9
public function addItem(Varien_Object $item) | |
{ | |
$itemId = $this->_getItemId($item); | |
if (!is_null($itemId)) { | |
if (isset($this->_items[$itemId])) { | |
throw new Exception('Item (' . get_class($item) . ') with the same id "' . $item->getId() . '" already exist'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The item id is indeed the key for all collections.
merged and cherrypicked to v20 |
Description
This reduce number of getItemId() calls (found with Blackfire).
Same as #2957 #2677 #2699 #1571.
before (with a big cart with 51 downloadable, simple and bundle products):
after:
To check the PR, I added
if ($quoteItemId != $quoteItem->getItemId()) exit('boom');
.Yes this will not change many things, but this + many others...
Contribution checklist