Varnish 6 VCL optimization

[peterjaap]

This is a draft PR created in cooperation with @ThijsFeryn from Varnish Software and @IvanChepurnyi to optimize Magento's built in VCL. We will update the PR later on to explain why we made certain decisions.

[m2-assistant]

Hi @peterjaap. 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

ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, 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, join the Community Contributions Triage session to discuss the appropriate ticket.

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

[ihor-sviziev]

Could you please explain this change? Why you decided to remove this part?

[ThijsFeryn]

@peterjaap What was the deal again with this one? Did you say that Magento already returns Cache-Control: private, no-cache, no-store for these paths? Please clarify, I don't remember the exact reason.

[peterjaap]

Yes exactly

[ThijsFeryn]

@ihor-sviziev is that an acceptable answer?

Long story short: no need to write code to bypass the cache if Magento prevents these pages from ending up in the cache.

[ihor-sviziev]

@peterjaap @ThijsFeryn Make sense, thx.
Please point me to where this code is located. Does it works based on the path, or it's inside the controllers?
If that's the 2nd option than people might add custom controllers in their modules that reuse these front names and rely on this behavior.

[ihor-sviziev]

@ThijsFeryn @peterjaap any updates?

[ThijsFeryn]

@peterjaap this is your domain, can you provide an answer to @ihor-sviziev please?

[peterjaap]

@ihor-sviziev here:

• magento2/lib/internal/Magento/Framework/App/Response/Http.php

  Line 169 in 4a41ec4

  $this->setHeader('cache-control', 'no-store, no-cache, must-revalidate, max-age=0', true);

  (https://github.com/search?q=repo%3Amagento%2Fmagento2%20setNoCacheHeaders&type=code)

• magento2/app/code/Magento/PageCache/Plugin/AppendNoStoreCacheHeader.php

  Line 27 in 4a41ec4

  $response->setHeader('Cache-Control', 'no-store');

[engcom-Hotel]

Hello @peterjaap @ThijsFeryn @IvanChepurnyi,

As I have gone through with the PR, It seems that it is still in development. So please let us know if it is ready for review.

Thanks

[peterjaap]

@engcom-Hotel I guess we're still waiting for @cpartica to take a look at the GraphQL part

[engcom-Hotel]

Thanks @peterjaap for the reply!

@cpartica As @peterjaap mentioned here, we request you to please review it.

Till the time we are moving this PR in draft status.

Thanks

[onlinebizsoft]

@peterjaap consider to remove "no-store" from response header on cacheable pages to allow browsers caching the content and send validation request only for repeated visits.

[hostep]

@peterjaap: it looks like there are merge conflicts, caused by these recent changes: 4f23095#diff-09a0a5ee7d8c56ee87cac97b0f291d7e416a810510d2719cdff73be3b3785740 (recent you may ask because the commit date is pretty old, but it looks like the commit was only merged 6 days ago)

Can you try to figure out how to solve these conflicts in this PR?

Thanks!

[peterjaap]

@ThijsFeryn I remember you talking about collapsing during our call, could you take a look at this? 4f23095#diff-09a0a5ee7d8c56ee87cac97b0f291d7e416a810510d2719cdff73be3b3785740

[peterjaap]

@ThijsFeryn another question, I ran into this customized VCL: https://gist.github.com/henkvalk/f3b1cee66c1daf65b69630a01b8b096e

It uses xkey to softpurge instead of ban. We could introduce a boolean selector in the Magento admin to enable this feature for the installs that have the xkey mod installed. Any thoughts on that?

[ThijsFeryn]

@ThijsFeryn another question, I ran into this customized VCL: https://gist.github.com/henkvalk/f3b1cee66c1daf65b69630a01b8b096e

It uses xkey to softpurge instead of ban. We could introduce a boolean selector in the Magento admin to enable this feature for the installs that have the xkey mod installed. Any thoughts on that?

@peterjaap Running xkey requires compiling https://github.com/varnish/varnish-modules from source or installing Varnish packages provided by specific distributions.

It's tricky to depend on this VMOD. Until we decide to package it officially, I recommend using bans.

[engcom-Hotel]

Hello @peterjaap @ThijsFeryn,

Are we ready to pick this PR for review? As I can see the PR is in a conflict state. So I request you to please resolve the conflict.

Thanks

[engcom-Hotel]

Hi @peterjaap @ThijsFeryn,

Thank you for your contribution!

As per #36796 (comment) comment, in order to move further with this PR, resolving conflicts is necessary. Can you please look into them?

Till then we are closing this PR.

Please feel free to reopen or let us know when you are ready to work on it again, and we will be happy to re-open it.

Thank you!

[robinero]

@peterjaap, @ThijsFeryn,

Can we try and get this merge conflict resolved, it would be a shame to let all your work get undone.

4f23095#diff-09a0a5ee7d8c56ee87cac97b0f291d7e416a810510d2719cdff73be3b3785740

This commit makes sure that discounted prices do not get cached and shown to logged out users when the "X-Magento-Vary" cookie gets removed from the client.

The way to reproduce this:

1. Add a product with a normal price and a discounted price applied to a customer group.
2. Login as the customer with the discounted customer group price.
3. Visit the product page and remove "X-Magento-Vary" cookie,
   refresh the page so that the product page gets "cached without the X-Magento-Vary cookie".
4. Visit the product page as a logged out user.

Without the fix:
The product page shows the discounted price to the logged out user.

With the fix:
The product page shows the normal price to the logged out user.

I'm not sure if the way that it's currently implemented is the best way of avoiding it, but we can apply the same fix to your optimized VCL.