magento2 icon indicating copy to clipboard operation
magento2 copied to clipboard

Published 20 hours ago • verified publisher icon magento

Remove all marketing get parameters to minimize the cache

Open rogerdz opened this issue 1 year ago • 11 comments

Description (*)

Same logic when use varnish (https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/PageCache/etc/varnish7.vcl#L84) but for build-in fpc

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes magento/magento2#<issue_number>

Manual testing scenarios (*)

  1. Clear cache: bin/magento cache:clean
  2. Run command: curl -I --location-trusted 'https://magento2.test/gear/bags.html?test=111&utm_source=123&gclid=abc' | grep -i X-Magento-Cache-Debug, it return x-magento-cache-debug: MISS => correct
  3. Run command: curl -I --location-trusted 'https://magento2.test/gear/bags.html?test=111&utm_source=123&gclid=abc' | grep -i X-Magento-Cache-Debug, it return x-magento-cache-debug: HIT => correct
  4. Run command: curl -I --location-trusted 'https://magento2.test/gear/bags.html?test=111' | grep -i X-Magento-Cache-Debug => it should return x-magento-cache-debug: HIT

Questions or comments

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)

Resolved issues:

  1. [x] resolves magento/magento2#39266: Remove all marketing get parameters to minimize the cache

rogerdz avatar Aug 23 '24 08:08 rogerdz

Hi @rogerdz. Thank you for your contribution! Here are some useful tips on 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

:exclamation: 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 :information_source: Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Code Contributions documentation. Join Magento Community Engineering Slack and ask your questions in #github channel.

m2-assistant[bot] avatar Aug 23 '24 08:08 m2-assistant[bot]

@magento run all tests

rogerdz avatar Aug 23 '24 08:08 rogerdz

@magento run all tests

rogerdz avatar Aug 23 '24 15:08 rogerdz

Nice. Maybe you want to update the parameters based on https://github.com/magento/magento2/pull/39188/files#diff-40e00b332f8dd95fbf48a312b9b8cfeee0d4e70bd7528c0e794c84c9e00c113fR85.

sprankhub avatar Oct 07 '24 15:10 sprankhub

@magento create issue

engcom-Hotel avatar Oct 16 '24 06:10 engcom-Hotel

@magento run all tests

engcom-Hotel avatar Oct 16 '24 06:10 engcom-Hotel

@magento run Database Compare, Functional Tests B2B, Functional Tests CE

engcom-Hotel avatar Oct 16 '24 09:10 engcom-Hotel

Please mind that https://github.com/magento/magento2/pull/39188 is also community-picked and is more complete. Specifically, it includes the changes from this PR. Hence, I think the solution from https://github.com/magento/magento2/pull/39188 should be merged.

sprankhub avatar Oct 16 '24 11:10 sprankhub

@sprankhub, I think you got confused and meant to comment on https://github.com/magento/magento2/pull/35228 This PR here adds the extra parameters to the non-Varnish based FPC solution from Magento. But I believe the list should be updated with the list from https://github.com/magento/magento2/pull/39188 so they match 100%.

Ideally we should have one place where the entire list is defined and can be used both by the Varnish VCL file generator and the non-Varnish based FPC solution. Maybe using a backoffice configuration field? We have a new request for that since today BTW: https://github.com/magento/magento2/issues/39268 (even though the example given there is a very bad idea)

hostep avatar Oct 16 '24 18:10 hostep

Sorry, looks like I got confused a bit. But still, it’s not clear if it worth fixing built in full page cache. Does anyone using it in production? If no - why would we support the list in 2 places?

ihor-sviziev avatar Oct 17 '24 12:10 ihor-sviziev

Yes, people use Magento shops without Varnish in production as Varnish requires a big amount of memory and certain shops have soo little traffic it's not worth paying the extra cash for extra resources. In that case the built-in FPC using filesystem as storage is more then adequate enough. Some of our clients use this sort of setup.

hostep avatar Oct 17 '24 16:10 hostep

@magento run all tests

rogerdz avatar Oct 28 '24 06:10 rogerdz

Could you please cover your changes with unit tests?

I updated unit test + marketing parameters

rogerdz avatar Oct 28 '24 08:10 rogerdz

Hello @rogerdz,

Thanks for the contributions!

Could you please address following comments: https://github.com/magento/magento2/pull/39099#discussion_r1819089463 and https://github.com/magento/magento2/pull/39099#discussion_r1819095028? Additionally, please update the copyright tag as suggested here.

If you're unable to make these changes, please let me know, and I will take care of it.

Thanks again!

engcom-Dash avatar Nov 13 '24 09:11 engcom-Dash

@magento run all tests

engcom-Dash avatar Nov 19 '24 12:11 engcom-Dash

@magento run all tests

engcom-Dash avatar Nov 19 '24 12:11 engcom-Dash

@magento run Functional Tests CE, Unit Tests

engcom-Hotel avatar Nov 20 '24 08:11 engcom-Hotel

@magento run all tests

engcom-Dash avatar Nov 20 '24 12:11 engcom-Dash

Hi @rogerdz,

Thanks for your Contribution!!.

We have verified the issue in Latest 2.4-develop Magento Instance and the issue is not reproducible.Kindly refer the screenshots.

Screenshot 2024-11-22 at 12 16 46

The last request also showing Hit.Could you please let us know if we are missing anything.

Thanks.

engcom-Bravo avatar Nov 22 '24 11:11 engcom-Bravo

@engcom-Bravo, are you testing on built-in FPC without Varnish?

ihor-sviziev avatar Nov 22 '24 11:11 ihor-sviziev

Hi @ihor-sviziev,

Thanks for your update!!.

We have tested with Full Page Cache(built in) without varnish.

Screenshot 2024-11-22 at 18 19 24

Thanks.

engcom-Bravo avatar Nov 22 '24 12:11 engcom-Bravo

@engcom-Bravo last request must call url without marketing parameter

rogerdz avatar Nov 22 '24 12:11 rogerdz

@magento run all tests

engcom-Bravo avatar Nov 25 '24 04:11 engcom-Bravo

Hi @rogerdz,

Thanks for the collaboration & contribution!

:heavy_check_mark: QA Passed

Preconditions:

  • Install fresh Magento 2.4-develop

Steps to reproduce

  • Clear cache: bin/magento cache:clean
  • Run command: curl -I --location-trusted 'https://magento2.test/gear/bags.html?test=111&utm_source=123&gclid=abc' | grep -i X-Magento-Cache-Debug, it return x-magento-cache-debug: MISS => correct
  • Run command: curl -I --location-trusted 'https://magento2.test/gear/bags.html?test=111&utm_source=123&gclid=abc' | grep -i X-Magento-Cache-Debug, it return x-magento-cache-debug: HIT => correct
  • Run command: curl -I --location-trusted 'https://magento2.test/gear/bags.html?test=111' | grep -i X-Magento-Cache-Debug => it should return x-magento-cache-debug: HIT

Before: :heavy_multiplication_x: 

Screenshot 2024-11-25 at 09 45 49

After: :heavy_check_mark:

Screenshot 2024-11-25 at 09 56 33

Builds are failed. Hence, moving this PR to Extended Testing.

Thanks.

engcom-Bravo avatar Nov 25 '24 11:11 engcom-Bravo

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

engcom-Dash avatar Nov 27 '24 06:11 engcom-Dash

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

engcom-Dash avatar Nov 27 '24 12:11 engcom-Dash

The Functional B2B and CE test failures seem flaky and some of these failures are known issues. They are neither part of PR nor failing because of PR changes. Hence moving it to Merge in Progress.

Functional Tests B2B Run 1: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/39099/1875e843e30894817894cbc522dd0261/Functional/allure-report-b2b/index.html#categories/3a3a8306986cbb01e5245b23e6e5b238/6e05a95598af89e8/ image

Run 2: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/39099/66f491bf0b44bceef57107d51d8cdf55/Functional/allure-report-b2b/index.html#categories/3a3a8306986cbb01e5245b23e6e5b238/5d0fa8a0b7cc1e95/ image

Known issues: MultiShippingWithCreationNewCustomerAndAddressesDuringCheckoutTest: ACQE-7229 StorefrontReuseCouponCodeAfterOrderCanceledTest: ACQE-7230

Functional Tests CE Run 1: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/39099/977f45eeeb8f1049aa14572e5d9f74fc/Functional/allure-report-ce/index.html#categories/6d7e37752c3cc83cb3d2063131f608bb/30a31853636a9131/ image

Run 2: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/39099/5eff451448e049fcb61454043d729cc1/Functional/allure-report-ce/index.html#categories/6d7e37752c3cc83cb3d2063131f608bb/476c47e6a72b24e0/ image

Known issue: VerifyDefaultWYSIWYGToolbarOnProductTestCest: ACQE-7227

engcom-Dash avatar Nov 27 '24 15:11 engcom-Dash