33465 Varnish cache adding top menu category ids to all page cache tags

[Jitheesh]

Description (*)

magento2/app/code/Magento/PageCache/Model/Layout/LayoutPlugin.php

Line 88 in bf4cdad

$isVarnish = $this->config->getType() === Config::VARNISH;

Magento is using type comparison for determining varnish cache is enabled or not. But it returns false even if varnish is enabled.

This is because of following comparison; "2" === 2

Based on following code, I think it expected to skip adding tags for a top menu block
$isEsiBlock = $block->getTtl() > 0; if ($isVarnish && $isEsiBlock) { continue; }

For Topmenu block we have TTL value so that $isEsiBlock become true and with my fix, $isVarnish will also become true.

Fixed Issues (if relevant)

1. Fixes Varnish cache adding top menu category ids to all page cache tags #33465

Manual testing scenarios (*)

1. Enable varnish cache and make sure it is running
2. Open home page or any CMS page example: /about-us
3. Check Magento cache Tags.
4. Cache tags won't include top category IDs after merging this PR

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)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @Jitheesh. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

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

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[hostep]

Auwch, this was initially implemented correctly by b59dc72 (#28992), but then reverted partially by 1778c0f

@gabrieldagama: is there news from the Fastly module around this, maybe they fixed something at their end to get their module working with this change by now?

[mrtuvn]

another pr supprised me with reverted code. Can't wait to hear any news from fastly and internal

[Jitheesh]

Looks like these issues are also related

• Varnish cache is cleared for the product and categories after an order is placed #30128
• Indexer cronjobs flushes all category and product pages in FPC / Varnish #29806
• FPC for unrelated products being cleared after editing a single product #25670

To replicate this issue try following steps

• Open a CMS page and make sure it is delivered from cache
• Edit price and stock status of a single product from top level category
• Run indexers
• Validate PURGE requests sent by Magento, it will contains tags like cat_c_
• Refresh CMS page, it will served from server instead of cache

[ihor-sviziev]

Maybe it worth fixing the following code as well (convert to int)?

magento2/app/code/Magento/PageCache/Model/Layout/LayoutPlugin.php

Line 88 in bf4cdad

$isVarnish = $this->config->getType() === Config::VARNISH;

[ihor-sviziev]

Could you fix the following code as well (convert to int)?

magento2/app/code/Magento/PageCache/Model/Layout/LayoutPlugin.php

Line 88 in bf4cdad

$isVarnish = $this->config->getType() === Config::VARNISH;

[hostep]

@ihor-sviziev: I don't think that is necessary. If the getType method always returns an int, why would we then need to cast it again to an int?

[ihor-sviziev]

@hostep, unfortunately, we already had a case when conversion to int was reverted for some reason, and I don't want to get it back. The missing return type can't guarantee that we will have int. I think it's better to add such conversion where it's used to make sure this issue won't appear again.

[hostep]

Hmm you might have a point, I just quickly investigated the Fastly module's code

https://github.com/fastly/fastly-magento2/blob/707ffb3779a1d3c7921b81a88a69c5613490a702/Observer/FlushAllCacheObserver.php#L61

Their Config::FASTLY constant is not an int but a string: https://github.com/fastly/fastly-magento2/blob/707ffb3779a1d3c7921b81a88a69c5613490a702/Model/Config.php#L48

But this is a terrible bug in Fastly's code, Magento says in the documentation that the return type of the getType() method should be an int and they ignore that.

So either Fastly needs to rewrite a bunch of their code, or Magento needs to accomodate for that and since Magento Cloud is always using that Fastly module, Adobe will probably not accept this PR as is.
So we'll probably need to take @ihor-sviziev's advice. And don't cast to an int in the getType() method unfortunately, and maybe also need to change the phpdoc to @return int|string for that method?

Let's try to pull in @vvuksan from the Fastly module team: what do you think about this?

[Jitheesh]

Could you fix the following code as well (convert to int)?

magento2/app/code/Magento/PageCache/Model/Layout/LayoutPlugin.php

Line 88 in bf4cdad

$isVarnish = $this->config->getType() === Config::VARNISH;

Instead of converting to int, can we skip type comparison ? ie change === to ==

[mrtuvn]

Maybe we can check vanish enable like this way in below file

https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/PageCache/Observer/ProcessLayoutRenderElement.php#L123

@Jitheesh you can follow above way, We can also update type as hostep suggestion in file config
$isVarnish = ($this->config->getType() == Config::VARNISH);

[ihor-sviziev]

Could you fix the following code as well (convert to int)?

magento2/app/code/Magento/PageCache/Model/Layout/LayoutPlugin.php

Line 88 in bf4cdad

$isVarnish = $this->config->getType() === Config::VARNISH;

Instead of converting to int, can we skip type comparison ? ie change === to ==

TBH I don't think it's a good option, as not strict comparison might cause additional not expected issues.

[Jitheesh]

Could you fix the following code as well (convert to int)?

magento2/app/code/Magento/PageCache/Model/Layout/LayoutPlugin.php

Line 88 in bf4cdad

$isVarnish = $this->config->getType() === Config::VARNISH;

Instead of converting to int, can we skip type comparison ? ie change === to ==

TBH I don't think it's a good option, as not strict comparison might cause additional not expected issues.

In this case, should we update other places reported by @mrtuvn

magento2/app/code/Magento/PageCache/Observer/ProcessLayoutRenderElement.php

Line 123 in 6e47ef3

$this->isVarnishEnabled = ($this->_config->getType() == \Magento\PageCache\Model\Config::VARNISH);

[ihor-sviziev]

@Jitheesh, yes, please

[ihor-sviziev]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[vvuksan]

@hostep if something needs to be fixed in the Fastly module please open up an issue and we'll take a look.

[hostep]

@vvuksan: I'm not using the Fastly module, so I'm not interested in reporting bugs.
I'm just getting you involved because a bug was introduced in core Magento to prevent another bug to happen in the Fastly module, by this commit: 1778c0f (it would have been better to fix this in the Fastly module than to revert that change in core Magento which introduced the problem that this PR is trying to fix).
If you are interested in this, please read through the entire thread, I think the info is clear enough to understand what is going on.

[mrtuvn]

@magento run Unit Tests, Functional Tests B2B

[engcom-Bravo]

Hi @mrtuvn

I have updated the PR with requested changes.
Can you please review PR.

[ihor-sviziev]

The unit test is fixed. Others look like they are false positives. Let's try to re-run them

@magento run Functional Tests B2B, Functional Tests CE, Integration Tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[magento-engcom-team]

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

[ihor-sviziev]

@engcom-Alfa, any updates about testing of this PR?

[engcom-Alfa]

@engcom-Alfa, any updates about testing of this PR?

I am picking it now, and will update soon here.
Thanks for your patience!

[engcom-Alfa]

✔️ QA Passed

Preconditions:

1. Install a Magento with sample data
2. Install Varnish cache and make sure it is configured to Magento

Manual testing scenario:

1. Open the frontend and be in home page.

2. Inspect the page and be in network tab of the developer tool in the browser

3. Refresh the page and check the response headers' cache tags parameters

4. Repeat the same steps by navigating to About-us or any other CMS page

Before: ✖️ cat_c were getting included in the Cache-tags

After: ✔️ cat_c are getting excluded in the Cache-tags

Since it is relevant to getting the response from cache and so there is no additional testing is requited as part of regression.

[kestraly]

In the PR - Changes made to app/code/Magento/PageCache/Model/App/FrontController/BuiltinPlugin.php f9b9ec1 seems slow down loading times to uncached levels of speed at TTFB 1.5s with this change in place.

I'm wondering if it's a combination of using Redis for FPC for this instance or not, so I'll do some experimenting.

Reverting back to the original method of != in that file brings cached pages TTFB back down to 200ms.