Re-enable PriceBox block caching

[brucemead]

Description (*)

• Reduce default cache constant that was removed in Backport of MAGETWO-59089 for Magento 2.1: Magento 2.1.1 Problem with change currency #9841 (removed as currency wasn't included in cache key, now is added via this plugin https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Catalog/Block/Category/Plugin/PriceBoxTags.php#L68)
• Increase default cache time to 1 day (86400) this is more that acceptable as the cache key includes the current date so will not load for a new day which could have a new price rule, special price etc...

This issue is present on 2.2.x and 2.3.x (not tested on less than 2.1)

Manual testing scenarios (*)

Prerequisites

1. All caching enabled (especially FPC, Block HTML)

Testing steps

1. Before fix
   1.1. Go to a category page to warm page into cache
   1.2. Trigger a purge of respective category page
   1.3. Revisit page
   1.4. Note TTFB
2. After fix
   2.1. Go to a category page to warm page into cache
   2.2. Trigger a purge of respective category page
   2.3. Revisit page
   2.4. Note TTFB (faster, considerably faster for category pages with complex product types)

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)
• All automated tests passed successfully (all builds on Travis CI are green)

[magento-cicd2]

All committers have signed the CLA.

[magento-engcom-team]

Hi @brucemead. Thank you for your contribution
Here is 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-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[brucemead]

@magento-engcom-team give me test instance

[magento-engcom-team]

Hi @brucemead. Thank you for your request. I'm working on Magento instance for you

[magento-engcom-team]

Hi @brucemead, here is your new Magento instance.
Admin access: https://pr-19897.instances.magento-community.engineering/admin
Login: admin Password: 123123q

[brucemead]

@magento-engcom-team can these instances not come with sample data? Is there a working crontab? Is Varnish enabled on the box?

Thanks!

[VladimirZaets]

Hi @brucemead, thanks for collaboration. Unforchantly, currently, we haven't an ability to configure instances.

[orlangur]

Thanks @brucemead!

QA note: please check that there is no regression of MAGETWO-59089: Problem with change currency.

Unit test should be broken but it seems like Travis is not running tests properly yet.

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-3722 has been created to process this Pull Request

[magento-engcom-team]

@brucemead thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[orlangur]

@sidolov what happened here?

[orlangur]

@p-bystritsky I'm kindly ask you to squash commits before merging.

[p-bystritsky]

@orlangur I'll do it after all tests will pass.

[p-bystritsky]

@orlangur commits number reduced.

[m2-assistant]

Hi @brucemead, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[mekedron]

@brucemead, @magento-engcom-team, @orlangur
We've got more than 1 000 000 products and each pricebox is being cached now.

PriceBox has CAT_P cache tag and it means when any product was saved All the PriceBox caches were cleared. Take a look at the screenshot.

And for example we use Redis as a cache storage and sometimes a query that clears cache by CAT_P tag is getting up to 300-400 mb.
vendor/colinmollenhour/cache-backend-redis/Cm/Cache/Backend/Redis.php:920

The size of the storage is unbelievably big after this fix.
I'm sure it is a bad idea to save cache for the PriceBox renderer because all the parent blocks is being cached.

[ghost]

@durexlovesex unfortunately, only members of the maintainers team are allowed to unassign developers from the pull request

[ghost]

@durexlovesex unfortunately, only members of the maintainers team are allowed to unassign developers from the pull request

[danemacmillan]

This should never have been merged in. What's the point of caching rendered PriceBox HTML in Redis, especially with a FPC like Varnish (or even the built-in one) in place?

Consider a very common architecture:

• You have a group of instances behind a load balancer.
• Each instance runs a Varnish and Redis daemon.
• Given that Magento2 supports purging a group of Varnish instances, this is no problem.
• Update prices, and watch all the Varnish instances receive the PURGE header with the specific tags.
• Refresh page and note that the price is not updated!
• It hasn't refreshed because only Varnish got the message, and because there is no mechanism in place to flush a group of Redis caches.
• The page got rendered again, using the PriceBox HTML cached in Redis.

It's totally pointless to cache this rendered bit of HTML in Redis. Furthermore, it's actually erroneous, especially based on M2's recommended architecture, that places one Redis daemon on each instance. How is one supposed to update prices in such an architecture?

Obviously, if one is only hosting a small shop where all daemons are running on the same machine, they would never understand the grave problem this introduces (@brucemead?).

This was not a problem on the version we have in production (2.2.10), but we're working on upgrading to 2.3.3, and this problem has been a nuisance.

I know personally my codebase will be setting the cache lifetime back to null, as this should never get cached. C'mon, guys, we have a FPC for a reason.

This is all on top of @durexlovesex's very valid concern about memory and performance. My store has 500k unique SKUs, which is already a lot, so doubling that will be ludicrous.