-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
33465 Varnish cache adding top menu category ids to all page cache tags #33468
33465 Varnish cache adding top menu category ids to all page cache tags #33468
Conversation
Hi @Jitheesh. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
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. 🕙 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 |
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? |
another pr supprised me with reverted code. Can't wait to hear any news from fastly and internal |
Looks like these issues are also related
To replicate this issue try following steps
|
Maybe it worth fixing the following code as well (convert to int)?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix the following code as well (convert to int)?
$isVarnish = $this->config->getType() === Config::VARNISH; |
@ihor-sviziev: I don't think that is necessary. If the |
@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. |
Hmm you might have a point, I just quickly investigated the Fastly module's code Their But this is a terrible bug in Fastly's code, Magento says in the documentation that the return type of the 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. Let's try to pull in @vvuksan from the Fastly module team: what do you think about this? |
Instead of converting to int, can we skip type comparison ? ie change === to == |
Maybe we can check vanish enable like this way in below file @Jitheesh you can follow above way, We can also update type as hostep suggestion in file config |
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
|
@Jitheesh, yes, please |
@magento run all tests |
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. |
Hi @ihor-sviziev, thank you for the review. |
@engcom-Alfa, any updates about testing of this PR? |
I am picking it now, and will update soon here. |
✔️ QA Passed Preconditions:
Manual testing scenario:
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. |
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. |
Description (*)
magento2/app/code/Magento/PageCache/Model/Layout/LayoutPlugin.php
Line 88 in bf4cdad
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)
Manual testing scenarios (*)
Contribution checklist (*)