New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactored the system check to allow plugins to add new diagnostics #7646
Conversation
(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!) |
|
||
class Updates_2_13_0_b2 extends Updates | ||
{ | ||
static function update() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the non-static Updates::doUpdate method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
$this->assertEquals(DiagnosticResult::STATUS_ERROR, $results[0]->getStatus()); | ||
$this->assertEquals('Warning', $results[1]->getLabel()); | ||
$this->assertEquals(DiagnosticResult::STATUS_WARNING, $results[1]->getStatus()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to also add an Ok diagnostic to the check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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 |
Feedback:
|
That's a good thing right? Currently the directory list has a different font than the rest, with the PR it becomes consistent.
Done. |
Refactored the SystemCheck class into new Diagnostics API that allows plugins to add new checks via dependency injection config. The new API is located in the new Diagnostics plugin and plugin specific diagnostics are included in their respective plugins. Also includes new CLI command to run diagnostics.
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
Diagnostic
interfaceadd it to the
diagnostics.mandatory
ordiagnostics.optional
list, for example: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.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.