Skip to content

Conversation

@zolat
Copy link

@zolat zolat commented Feb 21, 2018

As noted in issue #13595 the Magento\Theme\Block\Html\Footer block is not saved to cache. This issue has been looked at as part of #MLAU18

Description

In investigating the issue it was found that the footer block does not have a cache lifetime set and it also has code in its constructor to set the cache lifetime to false. This pull request removes the forcing of cache lifetime to false and sets the default value to 3600 as done so in the \Magento\Theme\Block\Html\Topmenu block.

Fixed Issues (if relevant)

  1. loadCache for Block Magento\Theme\Block\Html\Footer dont work #13595

Manual testing scenarios

  1. Disable full page caching so that block level caching may be observed
  2. View a page that contains the footer block. E.g. homepage
  3. Debug \Magento\Framework\View\Element\AbstractBlock::_saveCache on the first time a given footer block is loaded, and see that is is successfully saved to cache.
  4. Debug \Magento\Framework\View\Element\AbstractBlock::_loadCache on a following request and confirm that the observed footer block is loaded from cache.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Feb 21, 2018

CLA assistant check
All committers have signed the CLA.

*
* @return int
*/
protected function getCacheLifetime()
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be public?

Copy link
Author

Choose a reason for hiding this comment

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

hey @ihor-sviziev it is protected in \Magento\Framework\View\Element\AbstractBlock::getCacheLifetime

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok. So I approved your changes

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
MAGETWO-88149 has been created to process this Pull Request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants