OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
863 stars 438 forks source link

Fixed cache issue in primary navigation block #4040

Closed empiricompany closed 4 days ago

empiricompany commented 2 weeks ago

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)

https://github.com/OpenMage/magento-lts/issues/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 (*)

fballiano commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

fballiano commented 2 weeks ago

@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 commented 1 week ago

@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 commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

Fix cache issue in navigation block

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

empiricompany commented 1 week ago

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

empiricompany commented 1 week ago

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

fballiano commented 1 week ago

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 commented 4 days ago

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 commented 4 days ago

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.