Magento2: Top menu cache issue - 2.3.1

Created on 18 Apr 2019 · 49Comments · Source: magento/magento2

Preconditions (*)

1.Magento 2.3.1 with all cache types enabled

Steps to reproduce (*)

Create 2-3 categories, set include in menu = Yes
On frontend click on any category page in top menu
Flush magento cache
Open the same category page
Open any other category page

Expected result (*)

Just opened category is marked as active in top menu

Actual result (*)

Both categories in top menu are marked as active
test

Cache Fixed in 2.3.x Clear Description Confirmed Format is valid Ready for Work Reproduced on 2.2.x Reproduced on 2.3.x

Source

lyusya-nnn

👍3

Most helpful comment

If somebody needs the fix for this issue you can create a plugin for Magento\Theme\Block\Html\Topmenu, so in di.xml you will have something like this:

<type name="Magento\Theme\Block\Html\Topmenu">
<plugin name="catalogTopmenuCache" type="MagentoOverwrite\Catalog\Plugin\Block\Topmenu" />
</type>

And your plugin will look like this:

<?php
namespace MagentoOverwrite\Catalog\Plugin\Block;

use Magento\Catalog\Model\Category;
use Magento\Catalog\Model\Layer\Resolver;

/**
 * Class Topmenu
 *
 */
class Topmenu
{
    /**
     * @var Resolver
     */
    private $layerResolver;

    /**
     * Topmenu constructor.
     * @param Resolver $layerResolver
     */
    public function __construct(
        Resolver $layerResolver
    ) {
        $this->layerResolver = $layerResolver;
    }

    /**
     * Get current Category from catalog layer
     *
     * @return \Magento\Catalog\Model\Category
     */
    private function getCurrentCategory()
    {
        $catalogLayer = $this->layerResolver->get();

        if (!$catalogLayer) {
            return null;
        }

        return $catalogLayer->getCurrentCategory();
    }

    /**
     * Add category id to cache tag
     *
     * @param \Magento\Theme\Block\Html\Topmenu $subject
     * @param array $result
     * @return array
     */
    public function afterGetCacheKeyInfo(\Magento\Theme\Block\Html\Topmenu $subject, $result)
    {
        $category = $this->getCurrentCategory();

        if (!$category || !$category->getId()) {
            return $result;
        }

        $result[] = Category::CACHE_TAG . '_' . $category->getId();

        return $result;
    }
}

lyusya-nnn on 23 Apr 2019

👍4

All 49 comments

Hi @lyusya-nnn. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

[ ] Summary of the issue
[ ] Information on your environment
[ ] Steps to reproduce
[ ] Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento-engcom-team give me 2.3-develop instance - upcoming 2.3.x release

For more details, please, review the Magento Contributor Assistant documentation.

@lyusya-nnn do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?

[ ] yes
[ ] no

m2-assistant[bot] on 18 Apr 2019

Hi @rammohan-krish. Thank you for working on this issue.
In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

[x] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
Details
If the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.

[x] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.
[x] 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
[x] 4. Verify that the issue is reproducible on 2.3-develop branch
Details
- Add the comment @magento-engcom-team give me 2.3-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and _stop verification process here_!
[x] 5. Verify that the issue is reproducible on 2.2-develop branch.
Details
- Add the comment @magento-engcom-team give me 2.2-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x
[ ] 6. Add label Issue: Confirmed once verification is complete.
[ ] 7. Make sure that automatic system confirms that report has been added to the backlog.

m2-assistant[bot] on 18 Apr 2019

@magento-engcom-team give me 2.3-develop instance

ktpl-rammohan on 18 Apr 2019

Hi @rammohan-krish. Thank you for your request. I'm working on Magento 2.3-develop instance for you

magento-engcom-team on 18 Apr 2019

Hi @rammohan-krish, here is your Magento instance.
Admin access: https://i-22407-2-3-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

magento-engcom-team on 18 Apr 2019

@magento-engcom-team give me 2.2-develop instance

ktpl-rammohan on 18 Apr 2019

Hi @rammohan-krish. Thank you for your request. I'm working on Magento 2.2-develop instance for you

magento-engcom-team on 18 Apr 2019

Hi @rammohan-krish, here is your Magento instance.
Admin access: https://i-22407-2-2-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

magento-engcom-team on 18 Apr 2019

:white_check_mark: Confirmed by @rammohan-krish
Thank you for verifying the issue. Based on the provided information internal tickets MAGETWO-99263, MAGETWO-99264 were created

Issue Available: @rammohan-krish, _You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself._

magento-engcom-team on 18 Apr 2019

If somebody needs the fix for this issue you can create a plugin for Magento\Theme\Block\Html\Topmenu, so in di.xml you will have something like this:

<type name="Magento\Theme\Block\Html\Topmenu">
<plugin name="catalogTopmenuCache" type="MagentoOverwrite\Catalog\Plugin\Block\Topmenu" />
</type>

And your plugin will look like this:

<?php
namespace MagentoOverwrite\Catalog\Plugin\Block;

use Magento\Catalog\Model\Category;
use Magento\Catalog\Model\Layer\Resolver;

/**
 * Class Topmenu
 *
 */
class Topmenu
{
    /**
     * @var Resolver
     */
    private $layerResolver;

    /**
     * Topmenu constructor.
     * @param Resolver $layerResolver
     */
    public function __construct(
        Resolver $layerResolver
    ) {
        $this->layerResolver = $layerResolver;
    }

    /**
     * Get current Category from catalog layer
     *
     * @return \Magento\Catalog\Model\Category
     */
    private function getCurrentCategory()
    {
        $catalogLayer = $this->layerResolver->get();

        if (!$catalogLayer) {
            return null;
        }

        return $catalogLayer->getCurrentCategory();
    }

    /**
     * Add category id to cache tag
     *
     * @param \Magento\Theme\Block\Html\Topmenu $subject
     * @param array $result
     * @return array
     */
    public function afterGetCacheKeyInfo(\Magento\Theme\Block\Html\Topmenu $subject, $result)
    {
        $category = $this->getCurrentCategory();

        if (!$category || !$category->getId()) {
            return $result;
        }

        $result[] = Category::CACHE_TAG . '_' . $category->getId();

        return $result;
    }
}

lyusya-nnn on 23 Apr 2019

👍4

It is coming because \Magento\Theme\Block\Html\Topmenu has a cache life time. If we return 0 in getCacheLifetime() it works fine. But is it best possible solution?

sanjaychouhan-webkul on 29 Apr 2019

Also when clicking on any Footer link one category is marked as active.
Screenshot (1)

sanjaychouhan-webkul on 29 Apr 2019

Hi @sanjaychouhan-webkul. Thank you for working on this issue.
Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

[ ] 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

[ ] 2. Verify that the issue is reproducible on 2.3-develop branch
Details
- Add the comment @magento-engcom-team give me 2.3-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and _stop verification process here_!
[ ] 3. Verify that the issue is reproducible on 2.2-develop branch.
Details
- Add the comment @magento-engcom-team give me 2.2-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x
[ ] 4. If the issue is not relevant or is not reproducible any more, feel free to close it.

m2-assistant[bot] on 30 Apr 2019

It can be a temporary solution.
Set cache_lifetime as 1 for the catalog.topnav block.

{YOUR_THEME_DIR}/Magento_Theme/layout/default.xml

<referenceContainer name="page.top">
    <referenceBlock name="catalog.topnav">
        <arguments>
            <argument name="cache_lifetime" xsi:type="number">1</argument>
        </arguments>
    </referenceBlock>
</referenceContainer>

mega667 on 2 May 2019

👎3

I am working on this #dmcdindia1

gauravagarwal1001 on 4 May 2019

Hi @gauravagarwal1001. Thank you for working on this issue.
Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

[ ] 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

[ ] 2. Verify that the issue is reproducible on 2.3-develop branch
Details
- Add the comment @magento-engcom-team give me 2.3-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and _stop verification process here_!
[ ] 3. Verify that the issue is reproducible on 2.2-develop branch.
Details
- Add the comment @magento-engcom-team give me 2.2-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x
[ ] 4. If the issue is not relevant or is not reproducible any more, feel free to close it.

m2-assistant[bot] on 4 May 2019

The main problem is not the cache lifetime - the cached content is the problem:
In some cases a menu with active elements is cached just because of 'deprecated' block functions.

Have a look at:

 Magento\Theme\Block\Html\Topmenu::_getMenuItemClasses(\Magento\Framework\Data\Tree\Node $item)
{
...
}

This function provides item's css classes.
Just remove these lines with the help of a plugin or something like that. There is no need to add these markup classes via php since it's done via js.

if ($item->getIsActive()) {
      $classes[] = 'active';
} elseif ($item->getHasActive()) {
      $classes[] = 'has-active';
}

jkeller92 on 11 Jun 2019

👍2

The main problem is not the cache lifetime - the cached content is the problem:
In some cases a menu with active elements is cached just because of 'deprecated' block functions.

Have a look at:
 Magento\Theme\Block\Html\Topmenu::_getMenuItemClasses(\Magento\Framework\Data\Tree\Node $item)
{
...
}
This function provides item's css classes.
Just remove these lines with the help of a plugin or something like that. There is no need to add these markup classes via php since it's done via js.
if ($item->getIsActive()) {
      $classes[] = 'active';
} elseif ($item->getHasActive()) {
      $classes[] = 'has-active';
}

The problem with doing it this way is that it's down to the javascript to mark as active, so there is a slight, sometimes visible delay, from when the page loads to when the menu is marked active. making the site appear slow.

NetStorm84 on 19 Jun 2019

The problem with doing it this way is that it's down to the javascript to mark as active, so there is a slight, sometimes visible delay, from when the page loads to when the menu is marked active. making the site appear slow.

Is this really a problem?

jkeller92 on 2 Jul 2019

The problem with doing it this way is that it's down to the javascript to mark as active, so there is a slight, sometimes visible delay, from when the page loads to when the menu is marked active. making the site appear slow.

Is this really a problem?

It was for me, the solution from mega667, by setting the cache_lifetime to 1, is a cleaner and more effective solution.

NetStorm84 on 2 Jul 2019

So any updates on this one? This issue is couple of months old now. Is this fixed in 2.3.2?

JeroenGerth on 2 Jul 2019

looks like it was fixed here before this issue was even opened, lol - https://github.com/magento/magento2/commit/bf8af1e7d3781db0921ed530e4f51e6d54ae56e1

but unfortunately there appears to have been a great deal of bikeshedding over test descriptions (until 2 days before this issue was opened!) so it was only merged 6 days ago: https://github.com/magento/magento2/commit/bc0b41245c5e55ecf27496e9ce1204ac6cf02a81

so i suppose you will have to stick with @lyusya-nnn solution until, i dunno... presumably november or something

:roll_eyes: embarrassing

maaarghk on 2 Jul 2019

Hi @rammohan-krish,
Could you double check if this issue was already fixed?
Thx