Addresses #7235 and #6726
I have refactored the system check: each check goes into a separate class, thus breaking the big (500 lines long)
SystemCheck class + big (400 lines) Twig template into small classes (the twig template is now dead simple).
It allows plugins to provide their own
Diagnostics, as they can be configured through the container (this is a good illustration of the possibilities of the DI config, if there are issues with this we could consider an alternative solution). To give you an idea, here is how you create a new diagnostic:
- implement the
- add it to the
diagnostics.optional list, for example:
'diagnostics.optional' => DI\add(array(
As such I have moved a few diagnostics to the plugin they belong to (e.g. "update over HTTPS" check is in the CoreUpdater now, as shown in the example above).
Given the logic is no longer split between the controller + view +
SystemCheck class, it was very easy to implement a console command:
diagnostic:run that runs the system check in the command line.
$ ./console diagnostic:run # will show only the warnings and errors $ ./console diagnostic:run --all # will show all diagnostics
I have also discussed with Michal and that command will replace the existing
enterpriseadmin:systemcheck (BC kept with an alias) which was kind of hacking the current implementation to allow it to run in CLI. I have a pull request ready for EnterpriseAdmin.
(I didn't look at the code but all of Piwik developers have thought one day that a new solution was needed and this is a good example of removing technical debt with high value!)
FYI, the SystemCheck UI test appears to have some small styling differences (font stuff + an italics change) & regression (Filesystem check isn't Ok). See http://builds-artifacts.piwik.org/ui-tests.diagnostics/11706.7/screenshot-diffs/diffviewer.html
Yep it's expected. Each line in the table had different styling (e.g. some stuff was in italic, some wasn't) or inconsistent output (e.g. "Filesystem check isn't Ok" is actually OK, it was just redundantly showing "OK" next to the green check, which was inconsistent with all the other checks).
So all is good I have just made things consistent, if we want to improve the design to make it look better let's do it in a separate pull request.
Ok, I get the Filesystem change is expected. I think the styling changes are ok, the italics, eg, don't seem to add anything. @mattab can you confirm the styling changes to the system check page here: http://builds-artifacts.piwik.org/ui-tests.diagnostics/11706.7/screenshot-diffs/diffviewer.html are not important?
Follow up issue for after this is merged: In QueuedTracking use diagnostics API instead of the existing SystemCheck class
can you confirm the styling changes to the system check page here: http://builds-artifacts.piwik.org/ui-tests.diagnostics/11706.7/screenshot-diffs/diffviewer.html are not important?
Feedback: - Installation_system_check -> the font used in the list of "Directories with write access" looks different in the processed - maybe the font family was lost? - UIIntegrationTest_admin_plugins -> add a description to the plugin - (UIIntegrationTest_ecommerce_log is a random failure (#6693))
Installation_system_check -> the font used in the list of "Directories with write access" looks different in the processed - maybe the font family was lost?
That's a good thing right? Currently the directory list has a different font than the rest, with the PR it becomes consistent.
UIIntegrationTest_admin_plugins -> add a description to the plugin