Fix loss of page_cache cache_dir setting from di.xml #22228

Vinai · 2019-04-08T20:34:24Z

Description

In a native Magento 2 installation both the page_cache and the default cache is stored in the var/cache directory. The var/page_cache directory is not used, even, though it is configured in di.xml.

This makes it impossible to use bin/magento cache:flush full_page to only clear the FPC, as it will also flush the cache directory.

The reason is any cache options configured in di.xml are lost if any configuration for a cache frontend is present in app/etc/env.php.

During the Magento installation a cache id_prefix for both the default and the page_cache frontends are written to the app/etc/env.php.

The bug was introduced by the PR: #18641
(note: the issue that was solved by that PR is also relevant for non-filesystem cache backends.)

In the method body of \Magento\Framework\App\Cache\Frontend\Pool::_getCacheSettings the original comment read:

Merging is intentionally implemented through array_merge() instead of array_replace_recursive()
to avoid "inheritance" of the default settings that become irrelevant as soon as cache storage type changes

This reasoning of the original comment applies only if the backend type is changed, but not if an option is added to be used with the default.

Fixed Issues

This PR will merge the deployment configuration and the di.xml configuration using array_replace_recursive instead of array_merge, as cache settings that are not used by other cache backends don't do any harm, and still can be changed by overwriting them when needed.

An alternative implementation of the solution checked if the page_cache backend was set to a different value of the default cache frontend, but that added a lot of additional coupling and in favor of simplicity I believe the solution in this PR is better.

The above fix revealed a bug that was masked before, namely that the when the built in page cache was is used, moving a category does not clean the appropriate cache records.
With varnish this issue does not occur because there the category top navigation is rendered as an ESI block, which is associated with all category ID cache tags (cat_c_<ID>).
The page cache records however are only associated with the generic category cache tag (cat_c).
Moving a category only cleans records that are associated with the category ID cache tag.
The solution is implemented as an event observer for the category_move event, which cleans the generic category cache tag if the built in page cache is used.

Manual testing scenarios

download source Magento (tarball or composer or github clone)
install Magento (ui or cli)
access a frontend or adminhtml page in the browser
With the bug: var/page_cache is empty and var/cache is not empty
Run bin/magento cache:flush full_page
Check var/cache is empty

After the patch is applied var/page_cache will no longer be empty after a page is accessed and bin/magento cache:flush full_page will only flush var/page_cache

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)

m2-assistant · 2019-04-08T20:34:26Z

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

larsroettig

Steps for quality:

✅ Review please see comments
❗️ Travis build
❗️ Manual Testing

dev/tests/integration/testsuite/Magento/Framework/App/Cache/Frontend/PoolTest.php

larsroettig · 2019-04-08T21:35:14Z

dev/tests/integration/testsuite/Magento/Framework/App/Cache/Frontend/PoolTest.php

+
+ $this->assertSame($testData, $pageCache->load($testKey));
+ }
+}


Empty Line missing

Fixed. But I'm curious what is the reason for this rule? It seems very arbitrary and useless to me...

@larsroettig didn't get which line you talked about...

Before I changed the commit line 59 was missing.

@Vinai I don't think it's needed. Basically any missing line is not worth commenting - phpcs should catch all needed.

dev/tests/integration/testsuite/Magento/Framework/App/Cache/Frontend/PoolTest.php

Vinai · 2019-04-09T03:44:52Z

Thanks for the review @larsroettig, sorry for being a little bitter sometimes. I really do appreciate you taking the time to look it over 👍

Vinai · 2019-04-09T09:53:30Z

The Codacy warnings are related only to outdated rule names in the ruleset:

The "WordPress.XSS.EscapeOutput" sniff has been renamed to "WordPress.Security.EscapeOutput". Please update your custom ruleset.
The "WordPress.CSRF.NonceVerification" sniff has been renamed to "WordPress.Security.NonceVerification". Please update your custom ruleset.
The "WordPress.WP.PreparedSQL" sniff has been renamed to "WordPress.DB.PreparedSQL". Please update your custom ruleset.

orlangur

Thanks @Vinai!

Codacy can be completely ignored, I heard about plans to turn it off but not sure why it's still here.

orlangur · 2019-04-09T11:39:11Z

dev/tests/integration/testsuite/Magento/Framework/App/Cache/Frontend/PoolTest.php

+use PHPUnit\Framework\TestCase;
+
+/**
+ * This is prime example for a senseless comment that only exists to satisfy the coding standard.


I feel your pain, this is one of mine top priorities in phpcs-related stuff, annoys me since @return void was introduced.

unfortunately Magento code is there for everybody to see and interpret and we have to do yet better, meaningful, rhetorical comments

Thankfully it's no longer necessary to add useless comments due to the updated phpdoc coding style guide!
I'll remove the docblock.

orlangur · 2019-04-09T11:40:38Z

dev/tests/integration/testsuite/Magento/Framework/App/Cache/Frontend/PoolTest.php

+
+ $this->assertSame($testData, $pageCache->load($testKey));
+ }
+}


@larsroettig didn't get which line you talked about...

orlangur · 2019-04-09T11:42:47Z

lib/internal/Magento/Framework/App/Cache/Frontend/Pool.php

 */
 $cacheInfo = $this->deploymentConfig->getConfigData(FrontendPool::KEY_CACHE);
 if (null !== $cacheInfo) {
- return array_merge($this->_frontendSettings, $cacheInfo[FrontendPool::KEY_FRONTEND_CACHE]);
+ return array_replace_recursive($this->_frontendSettings, $cacheInfo[FrontendPool::KEY_FRONTEND_CACHE]);


Let's see if it won't break anything else.

magento-engcom-team · 2019-04-09T11:45:14Z

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

VasylShvorak · 2019-04-12T10:53:12Z

✔️ QA passed

nmalevanec · 2019-04-15T10:05:56Z

@Vinai, could you please check the following failures in unit tests?
https://travis-ci.org/magento/magento2/jobs/517594558#L1195

Vinai · 2019-04-15T12:40:03Z

@nmalevanec Thank you for notifying me - I fixed and expanded the unit test cases.

nmalevanec · 2019-04-16T10:21:03Z

@Vinai, unfortunately we've encountered some problem after your fix. Could you please take a look at such scenario:

Navigate to admin area
Create two categories with structure like:

Observe storefront index page

Move cat2 to the same level as cat1

Observe storefront index page

Expected result

Actual result

Vinai · 2019-04-16T14:22:38Z

Good catch! My guess is the fix uncovered another bug that was masked by the regular cache and the page cache sharing the storage folder. I'll check what is causing the issue.

magento-engcom-team · 2019-08-08T16:21:41Z

Hi @sivaschenko, thank you for the review.
ENGCOM-4717 has been created to process this Pull Request
✳️ @sivaschenko, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

engcom-Delta · 2019-08-09T07:42:26Z

✔️ QA passed

ihor-sviziev · 2019-09-16T10:31:35Z

Hi @sidolov,
Is there any updates since August 9? Why it's still not merged?

sidolov · 2019-09-16T15:49:57Z

Hi @ihor-sviziev , we faced the build failures with provided changes. I'll add more info about failures as soon as possible

Vinai · 2019-09-17T04:04:28Z

@ihor-sviziev @sidolov Thanks for the follow up! I'm quite curious hear what issues occurred during the build and am happy to help fixing them if I can.

m2-assistant · 2019-10-01T22:12:13Z

Hi @Vinai, 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.

magento-engcom-team added Component: App Release Line: 2.3 labels Apr 8, 2019

Vinai requested review from larsroettig and orlangur April 8, 2019 20:35

larsroettig requested changes Apr 8, 2019

View reviewed changes

ghost assigned larsroettig Apr 8, 2019

ghost added the Progress: needs update label Apr 8, 2019

Vinai force-pushed the fix-fs-page_cache branch from 4d52753 to cb5c342 Compare April 9, 2019 03:43

orlangur approved these changes Apr 9, 2019

View reviewed changes

ghost assigned orlangur Apr 9, 2019

ghost added Progress: ready for testing and removed Progress: needs update labels Apr 9, 2019

VasylShvorak self-assigned this Apr 12, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Apr 12, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Apr 12, 2019

nmalevanec self-assigned this Apr 12, 2019

ghost added Progress: needs update and removed Progress: extended testing labels Apr 15, 2019

Vinai force-pushed the fix-fs-page_cache branch from cb5c342 to 78c825a Compare April 15, 2019 12:39

Vinai force-pushed the fix-fs-page_cache branch from 38fdf31 to 660c989 Compare July 26, 2019 19:05

sivaschenko approved these changes Aug 8, 2019

View reviewed changes

ghost assigned sivaschenko Aug 8, 2019

ghost added the Progress: ready for testing label Aug 8, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Aug 9, 2019

ghost added Progress: extended testing Progress: accept and removed Progress: testing in progress Progress: extended testing labels Aug 9, 2019

Vinai mentioned this pull request Sep 3, 2019

cache-clean -w Doesn't clean the cache but it says cleaned mage2tv/magento-cache-clean#44

Closed

ihor-sviziev added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Sep 16, 2019

sidolov added Award: test coverage Award: special achievement labels Sep 16, 2019

magento#22228: Suppress PHPCS violations in test

44569d5

sidolov requested review from akaplya and buskamuza as code owners September 30, 2019 19:12

magento#22228: Suppress PHPCS violations in test

989058c

magento-engcom-team pushed a commit that referenced this pull request Oct 1, 2019

ENGCOM-4717: Fix loss of page_cache cache_dir setting from di.xml #22228

a6b50b6

magento-engcom-team merged commit 989058c into magento:2.3-develop Oct 1, 2019

magento-engcom-team added this to the Release: 2.3.4 milestone Oct 1, 2019

Vinai deleted the fix-fs-page_cache branch October 2, 2019 09:01

This was referenced Jan 21, 2020

Cache cleaner detects file change, but GUI is unchanged? mage2tv/magento-cache-clean#67

Closed

Changing app/design/frontend/Mytheme/default/etc/view.xml - gallery var does not clear the necessary cache mage2tv/magento-cache-clean#48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix loss of page_cache cache_dir setting from di.xml #22228

Fix loss of page_cache cache_dir setting from di.xml #22228

Vinai commented Apr 8, 2019 •

edited

Loading

m2-assistant bot commented Apr 8, 2019

larsroettig left a comment

larsroettig Apr 8, 2019

Vinai Apr 9, 2019

orlangur Apr 9, 2019

Vinai Apr 9, 2019

orlangur Apr 9, 2019

Vinai commented Apr 9, 2019

Vinai commented Apr 9, 2019

orlangur left a comment

orlangur Apr 9, 2019

cpartica May 22, 2019

Vinai Jul 22, 2019

orlangur Apr 9, 2019

orlangur Apr 9, 2019

magento-engcom-team commented Apr 9, 2019

VasylShvorak commented Apr 12, 2019

nmalevanec commented Apr 15, 2019 •

edited

Loading

Vinai commented Apr 15, 2019

nmalevanec commented Apr 16, 2019

Vinai commented Apr 16, 2019

magento-engcom-team commented Aug 8, 2019

engcom-Delta commented Aug 9, 2019

ihor-sviziev commented Sep 16, 2019 •

edited

Loading

sidolov commented Sep 16, 2019

Vinai commented Sep 17, 2019

m2-assistant bot commented Oct 1, 2019

Fix loss of page_cache cache_dir setting from di.xml #22228

Fix loss of page_cache cache_dir setting from di.xml #22228

Conversation

Vinai commented Apr 8, 2019 • edited Loading

Description

Fixed Issues

Manual testing scenarios

Contribution checklist

m2-assistant bot commented Apr 8, 2019

larsroettig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vinai commented Apr 9, 2019

Vinai commented Apr 9, 2019

orlangur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magento-engcom-team commented Apr 9, 2019

VasylShvorak commented Apr 12, 2019

nmalevanec commented Apr 15, 2019 • edited Loading

Vinai commented Apr 15, 2019

nmalevanec commented Apr 16, 2019

Expected result

Actual result

Vinai commented Apr 16, 2019

magento-engcom-team commented Aug 8, 2019

engcom-Delta commented Aug 9, 2019

ihor-sviziev commented Sep 16, 2019 • edited Loading

sidolov commented Sep 16, 2019

Vinai commented Sep 17, 2019

m2-assistant bot commented Oct 1, 2019

Vinai commented Apr 8, 2019 •

edited

Loading

nmalevanec commented Apr 15, 2019 •

edited

Loading

ihor-sviziev commented Sep 16, 2019 •

edited

Loading