-
Notifications
You must be signed in to change notification settings - Fork 9.4k
magento/magento2#39948: Magento_Persistent performance overhead on non-cart, non-checkout pages #39967
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
base: 2.4-develop
Are you sure you want to change the base?
magento/magento2#39948: Magento_Persistent performance overhead on non-cart, non-checkout pages #39967
Conversation
…on-checkout pages - new QuoteResourceWrapper class that uses direct SQL queries instead of loading the entire quote object, which should improve performance
Hi @KrasnoshchokBohdan. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
@magento run all tests |
…on-checkout pages - static tests fix
…on-checkout pages - unit tests
@magento run all tests |
…on-checkout pages - static tests
@magento run Static Tests |
@magento run Database Compare, Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests, Magento Health Index, Sample Data Tests B2B, Sample Data Tests CE, Sample Data Tests EE, Semantic Version Checker, Unit Tests, WebAPI Tests |
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.
Pull Request Overview
This PR introduces a new QuoteResourceWrapper
to optimize persistent quote checks via direct SQL queries and updates the CheckExpirePersistentQuoteObserver
(and its tests) to use it instead of loading full quote models.
- Add
QuoteResourceWrapper
withisActive
andisPersistent
methods - Refactor
CheckExpirePersistentQuoteObserver
to use the new wrapper and remove model loading - Update unit tests to cover both the observer and the wrapper
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
app/code/Magento/Persistent/Model/QuoteResourceWrapper.php | New resource wrapper for efficient SQL-based quote queries |
app/code/Magento/Persistent/Observer/CheckExpirePersistentQuoteObserver.php | Refactor observer to use QuoteResourceWrapper |
app/code/Magento/Persistent/Test/Unit/Model/QuoteResourceWrapperTest.php | Unit tests for QuoteResourceWrapper |
app/code/Magento/Persistent/Test/Unit/Observer/CheckExpirePersistentQuoteObserverTest.php | Updated observer tests to mock the new wrapper |
Comments suppressed due to low confidence (5)
app/code/Magento/Persistent/Observer/CheckExpirePersistentQuoteObserver.php:77
- [nitpick] Matching the checkout page by the raw string
'checkout'
may unintentionally match other URLs. Consider using a more precise route or configuration-based constant for clarity and robustness.
private $checkoutPagePath = 'checkout';
app/code/Magento/Persistent/Observer/CheckExpirePersistentQuoteObserver.php:25
- [nitpick] Property naming is inconsistent: some fields use underscore prefixes (e.g.,
$_customerSession
) while others (e.g.,$request
) use camelCase without prefixes. Standardize on a single naming convention to improve readability.
protected $_customerSession;
app/code/Magento/Persistent/Observer/CheckExpirePersistentQuoteObserver.php:3
- The copyright year
2015
appears outdated compared to other files (which use2025
). Please update it for consistency.
* Copyright 2015 Adobe
app/code/Magento/Persistent/Test/Unit/Model/QuoteResourceWrapperTest.php:204
- [nitpick] In
testIsPersistentTypeCasting
, consider adding awith('quote')
expectation ongetTableName
to ensure the correct table is used in the query.
$this->resourceConnectionMock->expects($this->once())->method('getTableName')->willReturn($tableName);
app/code/Magento/Persistent/Test/Unit/Observer/CheckExpirePersistentQuoteObserverTest.php:123
- [nitpick] The test uses an integer
10
for the quote ID. Ensure consistent use of integer IDs across all tests to avoid potential type mismatches.
$quoteId = 10;
$this->quoteResourceWrapper = $quoteResourceWrapper ?: ObjectManager::getInstance() | ||
->get(QuoteResourceWrapper::class); |
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.
Direct use of ObjectManager
is discouraged. Consider injecting QuoteResourceWrapper
via constructor dependency injection (with a default null and fallback) to improve testability and adhere to best practices.
$this->quoteResourceWrapper = $quoteResourceWrapper ?: ObjectManager::getInstance() | |
->get(QuoteResourceWrapper::class); | |
$this->quoteResourceWrapper = $quoteResourceWrapper; |
Copilot uses AI. Check for mistakes.
Description (*)
I decided to test the solution proposed by the author. It turned out that the changes reduced the execution time of CheckExpirePersistentQuoteObserver from ~52ms to less than 1ms.


Before
After
So I make this pull request to initiate further discussion.
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)