Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Feature/remove vcl hit #72

Open
wants to merge 5 commits into
base: 2.4-develop
Choose a base branch
from

Conversation

toonvd
Copy link

@toonvd toonvd commented Dec 9, 2023

Description

This PR is a rework of the Magento VCL (the vcl_hit part) PR found here:
https://github.com/magento/magento2/pull/36796/files

All credits go to @peterjaap and @ThijsFeryn, I am simpling slicing this up and removing controversial parts to get early approval.

Related Pull Requests

#62
#69
#70
#71

Manual testing scenarios

  • Load the new VCL
  • Test generic Magento functionality

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)

@toonvd toonvd requested a review from a team as a code owner December 9, 2023 21:17
beresp.status != 404 &&
beresp.http.Cache-Control ~ "private") {
# Only cache HTTP 200 and HTTP 404 responses
if (beresp.status != 200 && beresp.status != 404) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toonvd Do we want to cache 301 & 302 redirects?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good remark, I usually refrain from caching them since it causes confusion for developers / end users. (especially 302)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps @damienwebdev and/or @danslo have an opinion on this

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants