-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
[Forwardport] Fix false cache_lifetime usage in xml layouts #17350
[Forwardport] Fix false cache_lifetime usage in xml layouts #17350
Conversation
Hi @jignesh-baldha. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
Hi @orlangur, thank you for the review. |
Hi @jignesh-baldha , please, fix unit test according to changes. |
@sidolov Fixed unit test according to changes. |
@jignesh-baldha , I think that removing dataset for test it's not a good idea. |
@sidolov Please check now, I've changes dataset. |
'expectsCacheSave' => $this->never(), | ||
'expectedResult' => 'dataFromCache', | ||
'expectedResult' => '', |
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.
Generally looks good. It would better use null for expected result because we started return null in lib/internal/Magento/Framework/View/Element/AbstractBlock.php
line: 1077
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.
@sidolov I tried with null for the expected result in the first two datasets but it does not work when running the unit test.
Hi @jignesh-baldha. Thank you for your contribution. |
Original Pull Request
#16086
Fix cache_lifetime usage with false value
Description
Fix
<argument name="cache_lifetime" xsi:type="boolean">false</argument>
Usage eg in
Magento/Swatches/view/frontend/layout/checkout_cart_configure_type_configurable.xml
Manual testing scenarios
Reproduced in 2.2 and 2.1 version
Expected result
Configurable product options should be preselected
Actual result:
Configurable product options are not preselected
It happens because there is a check in
View/Element/AbstractBlock.php
protected function _loadCache() { if ($this->getCacheLifetime() === null || !$this->_cacheState->isEnabled(self::CACHE_GROUP)) { return false; }
Contribution checklist