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

Reduce needless saves by avoiding setting _hasDataChanges flag #2066

Merged

Conversation

colinmollenhour
Copy link
Member

In some cases a setter is called just to store a reference to an object and not actually update the model data but it causes the model to be considered dirty thereby causing needles model saves. This tackles specific instances of this occurring but there is also a more holistic approach proposed in #1306.

This implements alternative changes proposed in #1306 (comment) and also an alternative solution to #1307

@woutersamaey
Copy link
Contributor

Insta-like!

fballiano
fballiano previously approved these changes May 4, 2022
kiatng
kiatng previously approved these changes May 5, 2022
@fballiano
Copy link
Contributor

since this is on v20 branch we should modify the README to add this PR to the "changes between v19 and v20" ;-)

@colinmollenhour
Copy link
Member Author

I added a note to README so it dismissed approvals from @fballiano and @kiatng

@fballiano fballiano merged commit c450d45 into OpenMage:20.0 Jun 9, 2022
*/
public function hasStockItem()
{
return !!$this->_stockItem;
Copy link
Contributor

Choose a reason for hiding this comment

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

why there is a double negation?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a cast to boolean as far as I've read

Copy link
Collaborator

Choose a reason for hiding this comment

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

I never used it but it is the equivalent (bool) $variables. Maybe the code with this variant !! should be modified to be more explicit for most and not to be able to search the Internet for the "double not" operator in PHP. A longer discussion exists here https://stackoverflow.com/questions/2127260/double-not-operator-in-php. Some people consider using it "fancy".

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, this is a habit from writing C.. In PHP probably better to just use (bool) but they are functionally equivalent.

@github-actions
Copy link

github-actions bot commented Jun 9, 2022

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  7 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit c450d45. ± Comparison against base commit f93c9e0.

@luigifab
Copy link
Collaborator

luigifab commented Jun 9, 2022

By adding:

public function setStockItem(Mage_CatalogInventory_Model_Stock_Item $stockItem)

Sorry, but there is:

/* back compatible stock item */
foreach ($productCollection as $product) {
$object = new Varien_Object(array('is_in_stock' => $product->getData('is_salable')));
$product->setStockItem($object);
}

And this produce:

Mage_Catalog_Model_Product::setStockItem(): Argument #1 ($stockItem) must be of type
Mage_CatalogInventory_Model_Stock_Item, Varien_Object given,
called in app/code/core/Mage/CatalogInventory/Model/Stock/Status.php on line 497

@fballiano
Copy link
Contributor

I'll keep a tab with this PR open so that we don't forget about this problem...

colinmollenhour added a commit to colinmollenhour/magento-lts that referenced this pull request Jun 9, 2022
…hod and fix incorrect class instantiated in addStockStatusToProducts. Refs OpenMage#2066
@colinmollenhour
Copy link
Member Author

@luigifab I just pushed a fix: #2208

@colinmollenhour colinmollenhour deleted the reduce-saves-hasdatachanges branch June 9, 2022 20:41
fballiano pushed a commit that referenced this pull request Jun 9, 2022
…hod and fix incorrect class instantiated in addStockStatusToProducts. Refs #2066 (#2208)
@luigifab
Copy link
Collaborator

luigifab commented Sep 5, 2022

Another side effect, with Amasty_Promo_Model_Sales_Quote_Item, there is:
$stockItem = $this->_data['product']->_data['stock_item']->_data;

That produce: Undefined array key "stock_item"
Solution is to call directly getStockItem().

@ADDISON74
Copy link
Collaborator

If we have a problem with this PR then it must be reported in Issues.

@luigifab
Copy link
Collaborator

luigifab commented Sep 5, 2022

No problem, but if another people encounter the same problem, I hope his preferred search engine will lead him here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Catalog Relates to Mage_Catalog Component: Customer Relates to Mage_Customer Component: Sales Relates to Mage_Sales Component: Wishlist Relates to Mage_Wishlist documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants