@tsteur opened this Pull Request on March 9th 2020 Member

refs https://github.com/matomo-org/matomo/pull/14617 where this feature was implemented.

We noticed an issue that the app might break when the config cache was used or created in a wrong context.

Typically, the user settings file is an absolute path like PIWIK_DOCUMENT_ROOT . '/config/config.ini.php'. For example /foo/bar/config/config.ini.php.

However, when eg js/tracker.php is opened, then PIWIK_DOCUMENT_ROOT is set to ... So user settings file is ../config/config.ini.php. In some other cases it is set to ../.. etc.

The cached config file includes the config file path like

$this->settingsChain = [
    $globalIniPath = [.... settings ...],
    $commonIniPath = [.... settings ...],
    $localIniPath = [.... settings ...],

A problem is eg when the config cache file does not exist, then js/tracker.php is called. Then the cache will include this

$this->settingsChain = [
    '../config/global.ini.php' = [.... settings ...],
     '../config/common.ini.php'= [.... settings ...],
     '../config/config.ini.php'= [.... settings ...],

But when the actual app is opened, the absolute path is used. This means when we then request the list of default activated plugins from the global ini, it calls:

$iniReader->getFrom('/foo/bar/config/global.ini.php', 'Plugins');

This entry does not exist in $this->settingsChain and therefore the app assumes no plugins exist by default and many core plugins aren't detected as core plugins and considered bogus because they don't have a plugin.json etc.

This PR fixes this by only accepting the cache when the user settings file is an absolute path, meaning it won't be applied in js/tracker.php (which is fine for us), and we also ignore the cache if the cached entry does not actually contain an entry for that $settingsChain[$userConfigFile]. Meaning we only accept a cache when it can be actually used and is valid.

There are heaps of other ways to fix this, such as always using the absolute PIWIK_DOCUMENT_ROOT, or resolving relative paths, etc. For our case as we only use that feature anyway this is the easiest fix and works well for us on production. Other changes could cause maybe regressions and we also can't fully control that all PIWIK_DOCUMENT_ROOT entries are always correct etc. We may want to use consistent PIWIK_DOCUMENT_ROOT on top eventually (in a different PR).

We're using this in production and works nicely. Should be fine to merge if tests pass.

@diosmosis commented on March 10th 2020 Member

Is there a reason we can't just use realpath()?

@tsteur commented on March 11th 2020 Member

I'm not 100% sure how it behaves with symlinks when one of the upper directories is a symlink etc.

For our purpose this PR would work fine. Eventually will ideally in a different PR ensure to use absolute paths in eg js/tracker.php too

This Pull Request was closed on March 11th 2020
Powered by GitHub Issue Mirror