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

Fixed cache issue in primary navigation block #4040

Merged
merged 6 commits into from
Jun 27, 2024

Conversation

empiricompany
Copy link
Contributor

@empiricompany empiricompany commented Jun 15, 2024

Description (*)

On fresh block cache if i reload a PDP page the category of the product still marked as active in other pages if there is not a specific current category (homepage, contacts, cms page etc..)

Related Pull Requests

Fixed Issues (if relevant)

#4041

Manual testing scenarios (*)

  1. Flush block cache
  2. Load a PDP
  3. Go to homepage

The category of first PDP loaded still flagged as active.
PS: The RWD theme does not have a CSS rule to highlight the active menu, so you must inspect the HTML.
active

This PR also adds css rule to highlight currente active category.

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added the Component: Catalog Relates to Mage_Catalog label Jun 15, 2024
@fballiano
Copy link
Contributor

I think you fixed a bug affecting 90% of magento1 (and 2) stores, where many disable the HTML_Block cache as a workaround.

will test asap!

@F1Red5
Copy link
Contributor

F1Red5 commented Jun 15, 2024

a bug affecting 90% of magento1 (and 2) stores

... rly?

I thinks its (only) when people want different navigation for non-category CMS pages. Imho not a bug ...

Maybe (untested) if $this->getCurrenCategoryKey() is false/null better add full action path to cache info. (?)

getIsHomePage() is (as it says) for homepage only and a "half-way-fix".

Is this a "common" issue or something that could/should be fixed in an extension?

Imho.

@empiricompany empiricompany marked this pull request as draft June 16, 2024 09:27
@empiricompany
Copy link
Contributor Author

empiricompany commented Jun 16, 2024

I thought I had solved it, but today I find a category selected again on the homepage, so it doesn't work correctly.
I can't figure out how the bug occurs.

@F1Red5 I need to go in-depth with the debug because it doesn't occur on all sites.

@empiricompany
Copy link
Contributor Author

Sorry for the confusion, I've finally figure out how this occur.
I've opened a separate issue and update the description on this PR.

@empiricompany empiricompany marked this pull request as ready for review June 16, 2024 11:28
@empiricompany
Copy link
Contributor Author

block Mage_Catalog_Block_Navigation is only used in catalog list

 <block type="core/text_list" name="top.menu" as="topMenu" translate="label">
    <label>Navigation Bar</label>
    <block type="page/html_topmenu" name="catalog.topnav" template="page/html/topmenu.phtml">
        <block type="page/html_topmenu_renderer" name="catalog.topnav.renderer" template="page/html/topmenu/renderer.phtml"/>
    </block>
</block>

i found Mage_Page_Block_Html_Topmenu read current_entity_key from registry

/**
 * Retrieve current entity key
 *
 * @return int|string
 */
public function getCurrentEntityKey()
{
    if ($this->_currentEntityKey === null) {
        $this->_currentEntityKey = Mage::registry('current_entity_key')
            ? Mage::registry('current_entity_key') : Mage::app()->getStore()->getRootCategoryId();
    }
    return $this->_currentEntityKey;
}

so i've setted in PDP controller current_entity_key with the full category path like in category controller

I know it's intuitive to set current_entity_key with the product ID, but that would create many cache blocks for each product, so I set it with the current path. this way there is one cache foreach category path on PDP

@empiricompany empiricompany changed the title Fix cache issue in navigation block for homepage Fix cache issue in navigation block Jun 17, 2024
@empiricompany
Copy link
Contributor Author

i can confirm it works, also on PDP the category of product it's selected now

@fballiano
Copy link
Contributor

@empiricompany can I test this problem without a full page cache plugin and with the RWD theme? cause I can't reproduce it on a vanilla OM installation (from a super quick test)

@empiricompany
Copy link
Contributor Author

empiricompany commented Jun 17, 2024

@empiricompany can I test this problem without a full page cache plugin and with the RWD theme? cause I can't reproduce it on a vanilla OM installation (from a super quick test)

@fballiano Unfortunately, the latest Docker Desktop update completely destroyed my dev environment, but it's a problem I've always encountered on all installations without an FPC.

Are you sure you followed these steps?

  1. Update the caches
  2. Navigate to a product page (this must be the first page loaded from fresh cache)
  3. Go to the homepage

Issue: The category of the visited product should remain selected.

Additionally, I can say that I use the flat catalog and the config catalog/seo/product_use_categories = 1.

@F1Red5
Copy link
Contributor

F1Red5 commented Jun 17, 2024

@empiricompany never faced that issue and the category path should be in cache-info ...

    public function getCacheKeyInfo()
    {
        $shortCacheId = [
            'CATALOG_NAVIGATION',
            Mage::app()->getStore()->getId(),
            Mage::getDesign()->getPackageName(),
            Mage::getDesign()->getTheme('template'),
            Mage::getSingleton('customer/session')->getCustomerGroupId(),
            'template' => $this->getTemplate(),
            'name' => $this->getNameInLayout(),
            $this->getCurrenCategoryKey()
        ];
        $cacheId = $shortCacheId;

        $shortCacheId = array_values($shortCacheId);
        $shortCacheId = implode('|', $shortCacheId);
        $shortCacheId = md5($shortCacheId);

        $cacheId['category_path'] = $this->getCurrenCategoryKey();
        $cacheId['short_cache_id'] = $shortCacheId;

        return $cacheId;
    }

Maybe try https://github.com/AOEpeople/Aoe_TemplateHints for easier debugging.

@empiricompany
Copy link
Contributor Author

yes, I have used aoe_templatepathints to debug and issue is on top menu block not in navigation, please try with flat catalog and use category path in product url maybe this cause the error

@F1Red5
Copy link
Contributor

F1Red5 commented Jun 17, 2024

Fix cache issue in navigation block

Mhh, can you share a screenshot with AoE-TH?

@empiricompany
Copy link
Contributor Author

empiricompany commented Jun 18, 2024

I've checked it on the latest main and I can confirm the issue.
However, in RWD, there is no CSS rule that highlights the active menu item, so you must inspect the HTML and you can see that the <li> has the class "active".

On sample data:

  1. Fresh cache
  2. Navigate to PDP https://domain.com/accessories/jewelry/pearl-stud-earrings.html
  3. Navigate to homepage

li_selected

@github-actions github-actions bot added the Template : rwd Relates to rwd template label Jun 18, 2024
@empiricompany
Copy link
Contributor Author

In the last commit, I've highlighted the active category.

@fballiano
Copy link
Contributor

no no I knew there was no active category CSS, I was checking the source code for the "active" class, I'm tight with time lately, I'll try to recheck asap

@fballiano
Copy link
Contributor

It happens only when FLAT CATALOG is enabled (confirmed, also tested that your PR works and solves the problem).

I'm thinking, how is this happening? I mean, the fix doesn't seem to be strictly-related to the problem... anyway since it works I'm still approving, waiting a bit before merging to see if we get some other kind of feedback.

@empiricompany
Copy link
Contributor Author

It's difficult to explain, but when you visit a PDP, a block cache of topmenu is stored without the specific tags of the category path. Then, when there is no category selected, like on the homepage, CMS, or contact pages, this cached block is retrieved without tags.

The PR also selects the category on the PDP according to the breadcrumbs.

@fballiano fballiano changed the title Fix cache issue in navigation block Fixed cache issue in primary navigation block Jun 27, 2024
@fballiano fballiano merged commit 95a54fa into OpenMage:main Jun 27, 2024
17 checks passed
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 Template : rwd Relates to rwd template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants