Check config file was written correctly #18024
Conversation
… retry failed write once more before logging
core/Config.php
Outdated
| @@ -410,13 +410,15 @@ protected function writeConfig() | |||
| throw $this->getConfigNotWritableException(); | |||
| } | |||
|
|
|||
| if (!$this->sanityCheck($localPath, $output)) { | |||
| // If sanity check fails, try to write the contents once more before logging the issue. | |||
| if (@file_put_contents($localPath, $output, LOCK_EX === false)) { | |||
There was a problem hiding this comment.
@JasonMortonNZ I think there's a typo re the === false which should be outside the function call?
Was also wondering if we should maybe check again the if to also do the sanity check again but not 100% sure. It may be useful I suppose but not sure if there could be any issues with that.
if (@file_put_contents($localPath, $output, LOCK_EX) === false
|| !$this->sanityCheck($localPath, $output)) {| if (!$this->sanityCheck($localPath, $output)) { | ||
| // If sanity check fails, try to write the contents once more before logging the issue. | ||
| if (@file_put_contents($localPath, $output, LOCK_EX === false)) { | ||
| StaticContainer::get(LoggerInterface::class)->info("The configuration file {$localPath} did not write correctly."); |
There was a problem hiding this comment.
I think we will want to post the event here before the log call in case we adjust the above if statement to also check for the sanityCheck. Otherwise we would post the event twice. I suppose putting the event here would make it harder to test if the event was triggered and it be no longer possible to properly test it I suppose.
There was a problem hiding this comment.
@tsteur yes, I was thinking about sanity check in this above if too and moving the event to be posted within, but it was tricky for the tests. I have added an optional 3rd argument $notify to the sanityCheck() function which allows us to set this to true on the second try, and this will trigger the event to be posted. This should prevent the event being posted twice, and also keep testability.
core/Config.php
Outdated
| $content = @file_get_contents($localPath); | ||
|
|
||
| if (trim($content) !== trim($expectedContent)) { | ||
| Piwik::postEvent('Core.configFileSanityCheckFailed', [$localPath]); |
There was a problem hiding this comment.
For the event it would be great to also add a little bit of text through a comment. The event will then be documented automatically on https://developer.matomo.org/api-reference/events
There was a problem hiding this comment.
@tsteur does the comment have to follow a specific format for it to be automatically documented?
There was a problem hiding this comment.
@JasonMortonNZ something similar to https://github.com/matomo-org/matomo/pull/18024/files#diff-8edb7d933e422f060b96277680eaada01d6ed1a7139e20963363b9973d625d12L415 where the comment is for the configFileChanged event. Basically, each variable in the array should be documented using @param and otherwise just regular comment AFAIK
Description:
Issue: dev-2272
Whenever we save the config file, it be great to double check if the config file was written correctly. This PR adds a couple of small sanity checks on the config file upon writing its content and writes a log entry should the sanity checks not pass.
Review