@samjf opened this Pull Request on March 30th 2022 Contributor

Description:

Ref: DEV-2583

When a tracking request supplies and invalid host an empty folder structure will be made in the cache directory if the GLOBAL ENABLE_CONFIG_PHP_CACHE is true. This change removes cleans up the directory once we can determine it is an invalid host.

Notes
Ideally we wouldn't even create this structure to begin with, however, the vendor lib we use for the file cache throws an exception if a directory path isn't supplied. It was easier just to clean up in the case we have an invalid host after the fact. For the time being this will solve arbitrary dirs stacking up in the tmp folder.

Review

@tsteur commented on March 31st 2022 Member

@sgiehl AFAIK we can't because we first need to look into the config to see if it's a valid host before we can know it is a valid host. Although thinking about it now, when using Matomo in a multi tenancy environment where all the config files are in misc/user/$host there would be a way by checking if the userSettingsFile exists. If a config file for a specific host exists, then we can assume it's a valid host. This may be good enough maybe. It be only an issue when using a regular config/config.ini.php. If I see this right then ENABLE_CONFIG_PHP_CACHE is not documented so this could work

diff --git a/core/Config/IniFileChain.php b/core/Config/IniFileChain.php
index 2801cbeb2..970466197 100644
--- a/core/Config/IniFileChain.php
+++ b/core/Config/IniFileChain.php
@@ -215,7 +215,7 @@ class IniFileChain
             $this->resetSettingsChain($defaultSettingsFiles, $userSettingsFile);
         }

-        $hasAbsoluteConfigFile = !empty($userSettingsFile) && strpos($userSettingsFile, DIRECTORY_SEPARATOR) === 0;
+        $hasAbsoluteConfigFile = !empty($userSettingsFile) && strpos($userSettingsFile, DIRECTORY_SEPARATOR) === 0 && is_file($userSettingsFile);
         $useConfigCache = !empty($GLOBALS['ENABLE_CONFIG_PHP_CACHE']) && $hasAbsoluteConfigFile;
@samjf commented on March 31st 2022 Contributor

@tsteur That looks to be a good solution. I'm not sure if we could put it on that line of code because reload seems to be the main way to save a missing cache and $useConfigCache needs to be true for that to happen in a normal situation.

We could add the same check just below:

diff --git a/core/Config/IniFileChain.php b/core/Config/IniFileChain.php
index bc37e84ef3..49efb2f71d 100644
--- a/core/Config/IniFileChain.php
+++ b/core/Config/IniFileChain.php
@@ -218,7 +218,7 @@ class IniFileChain
         $hasAbsoluteConfigFile = !empty($userSettingsFile) && strpos($userSettingsFile, DIRECTORY_SEPARATOR) === 0;
         $useConfigCache = !empty($GLOBALS['ENABLE_CONFIG_PHP_CACHE']) && $hasAbsoluteConfigFile;

-        if ($useConfigCache) {
+        if ($useConfigCache && is_file($userSettingsFile)) {
             $cache = new Cache();
             $values = $cache->doFetch(self::CONFIG_CACHE_KEY);
@tsteur commented on March 31st 2022 Member

I'm not too much into it but would think it pretty much does the same in the end 👍

@samjf commented on April 4th 2022 Contributor

I've stopped the cache from being created in two places in the IniChainSettings code now. The first one was ensuring that the cache being retrieved actually exists as a file. The second one is ensuring that the merged settings has a valid host setting present/set in the config.
This was close to your suggestion @sgiehl , but it doesn't require actually loading the config (as the code does), just ensuring the config structure is as expected which turns out to be enough.

@samjf commented on April 5th 2022 Contributor

Appears I have a few tests that need fixing:
Piwik\Tests\Unit\Config\IniFileChainCacheTest::test_reload_canReadFromCache with data set <a href='/0'>#0</a>
and
Piwik\Tests\Unit\Config\IniFileChainCacheTest::test_reload_canReadFromCache with data set <a href='/1'>#1</a>

@samjf commented on April 6th 2022 Contributor

I needed to update the test so that it didn't delete the original settings file when testing the cache. That step shouldn't be necessary to test that the cache is indeed being read; a new IniFileChain instance should suffice.

However, this will have an affect, if an original INI settings file is deleted then it will not attempt to read the cache for it (due to $userSettingsFile existence check in reload()). This will effectively invalidate it I believe. I didn't think this would be a problem, but let me know if it is troublesome.

The only other way to proceed would be the original cleanup method that originally was built.

This Pull Request was closed on April 12th 2022
Powered by GitHub Issue Mirror