[WIP] Zend diagnostics #33

Closed
wants to merge 2 commits into
from

2 participants

@lsmith77
Liip member

requires zendframework/ZendDiagnostics#11
adjust the composer.json to use the feature/runner-tweaks branch

HealthCheckController needs to be updated before merging.

@stof stof commented on an outdated diff Jul 16, 2013
Command/HealthCheckCommand.php
- break;
-
- case CheckResult::WARNING:
- $output->writeln(sprintf('<comment>WARNING</comment> %s', $msg));
- break;
-
- case CheckResult::CRITICAL:
- $error = true;
- $output->writeln(sprintf('<error>CRITICAL %s</error>', $msg));
- break;
-
- case CheckResult::UNKNOWN:
- $output->writeln(sprintf('<error>UNKNOWN<error> %s', $msg));
- break;
+ foreach ($runner->getReporters() as $reporter) {
+ $reporter->setVerbose($output->getVerbosity() > 1);
@stof
stof added a line comment Jul 16, 2013

you should use the constant instead of hardcoding its value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jul 16, 2013
Helper/Reporter.php
+
+use Symfony\Component\Console\Output\Output;
+use ZendDiagnostics\Result\Failure;
+use ZendDiagnostics\Result\ResultInterface;
+use ZendDiagnostics\Result\Success;
+use ZendDiagnostics\Result\Warning;
+use ZendDiagnostics\Runner\Reporter\BasicConsole;
+
+class Reporter extends BasicConsole
+{
+ /**
+ * @var Output
+ */
+ protected $output;
+
+ public function setOutput(Output $output)
@stof
stof added a line comment Jul 16, 2013

you should use the interface

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Jul 16, 2013
Helper/Reporter.php
+
+ public function setOutput(Output $output)
+ {
+ $this->output = $output;
+ }
+
+ protected function consoleWrite($text = '', ResultInterface $result = null)
+ {
+ if ($result instanceof Failure) {
+ $text = "<error>$text</error>";
+ } elseif ($result instanceof Warning) {
+ $text = "<comment>$text</comment>";
+ } elseif ($result instanceof Success) {
+ $text = "<info>$text</info>";
+ } elseif (null !== $result && !$result instanceof Success) {
+ $text = "<warning>$text</warning>";
@stof
stof added a line comment Jul 16, 2013

as the text can be anything (and so potentially containing some text catched by the output formatter as a style), you should use the formatter helper to escape it (the output formatter is permissive about places where the escaping can be omitted, but as you don't control the message, it is safer to go the safe way)

@lsmith77
Liip member
lsmith77 added a line comment Jul 16, 2013

is there an example where i can see this done?

@stof
stof added a line comment Jul 16, 2013

hmm, actually, no need for the helper (which does not escape in the basic format() method anyway). The logic is outside it. Use $text = OutputFormatter::escape($text); before putting it inside the tag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jul 16, 2013
Helper/Reporter.php
+ protected $output;
+
+ public function setOutput(Output $output)
+ {
+ $this->output = $output;
+ }
+
+ protected function consoleWrite($text = '', ResultInterface $result = null)
+ {
+ if ($result instanceof Failure) {
+ $text = "<error>$text</error>";
+ } elseif ($result instanceof Warning) {
+ $text = "<comment>$text</comment>";
+ } elseif ($result instanceof Success) {
+ $text = "<info>$text</info>";
+ } elseif (null !== $result && !$result instanceof Success) {
@stof
stof added a line comment Jul 16, 2013

the second part can be removed as the case where it is a Success is already covered above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jul 16, 2013
composer.json
@@ -20,8 +20,8 @@
],
"require": {
"php": ">=5.3.3",
- "liip/monitor": "1.0.*",
- "symfony/framework-bundle": "~2.0"
+ "symfony/framework-bundle": "~2.0",
+ "zendframework/zenddiagnostics": "dev-master"
@stof
stof added a line comment Jul 16, 2013

requiring dev-master is a bad idea. You should send them a PR adding a branch alias

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Jul 16, 2013
Resources/config/config.yml
public: false
arguments:
- localhost
- 11211
liip_monitor.check.doctrine_dbal:
- class: Liip\Monitor\Check\DoctrineDbalCheck
+ class: ZendDiagnostics\Check\DoctrineDbalCheck
@stof
stof added a line comment Jul 16, 2013

this class does not seem to exist in zendframework/ZendDiagnostics#10

@stof
stof added a line comment Jul 16, 2013

wrong namespace as you added it in the bundle

@lsmith77
Liip member
lsmith77 added a line comment Jul 16, 2013

fixed now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jul 16, 2013
Helper/Reporter.php
+{
+ /**
+ * @var Output
+ */
+ protected $output;
+
+ public function setOutput(OutputInterface $output)
+ {
+ $this->output = $output;
+ }
+
+ protected function consoleWrite($text = '', ResultInterface $result = null)
+ {
+ if ($result instanceof Failure) {
+ $text = "<error>$text</error>";
+ } elseif ($result instanceof Warning) {
@stof
stof added a line comment Jul 16, 2013

All these checks should be done against the interfaces rather than implementations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jul 16, 2013
Check/DoctrineDbalCheck.php
+use ZendDiagnostics\Result\Failure;
+use ZendDiagnostics\Result\Success;
+
+class DoctrineDbalCheck extends AbstractCheck
+{
+ /**
+ * @var ManagerRegistry
+ */
+ protected $manager;
+
+ /**
+ * @var string
+ */
+ protected $connectionName;
+
+ public function __construct(ManagerRegistry $manager, $connectionName = 'default')
@stof
stof added a line comment Jul 16, 2013

you should typehint the interface instead of the implementation

@stof
stof added a line comment Jul 16, 2013

actually, the change is the use statement

@stof
stof added a line comment Jul 16, 2013

and you could event typehint ConnectionRegistry (parent of ManagerRegistry) as you don't care about the manager part

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jul 16, 2013
Check/DoctrineDbalCheck.php
+namespace Liip\MonitorBundle\Check;
+
+use Symfony\Bridge\Doctrine\ManagerRegistry;
+
+use Doctrine\DBAL\Connection;
+
+use ZendDiagnostics\Check\AbstractCheck;
+use ZendDiagnostics\Result\Failure;
+use ZendDiagnostics\Result\Success;
+
+class DoctrineDbalCheck extends AbstractCheck
+{
+ /**
+ * @var ManagerRegistry
+ */
+ protected $manager;
@stof
stof added a line comment Jul 16, 2013

I would rename it to registry. It is a registry of managers (and connections), not a manager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jul 16, 2013
Command/HealthCheckCommand.php
}
}
- $output->writeln('done!');
+ $results = $runner->run($checkName);
// return a negative value if an error occurs
@stof
stof added a line comment Jul 16, 2013

1 is negative ? I need to go to some math lesson again. I must have missed one...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jul 16, 2013
composer.json
@@ -35,4 +35,4 @@
"dev-master": "1.0-dev"
@stof
stof added a line comment Jul 16, 2013

you should bump the branch alias to 2.0-dev IMO as this is a whole new version of the bundle. This way, you won't forget it when merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Jul 16, 2013
Check/DepsEntriesCheck.php
}
- return $result;
+ return new Success();
@stof
stof added a line comment Jul 16, 2013

I would return a warning if the deps file is used with Symfony 2.1+. What do you think about it ?

@lsmith77
Liip member
lsmith77 added a line comment Jul 16, 2013

sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof

The controller needs to be updated

@lsmith77 lsmith77 closed this Jan 17, 2014
@lsmith77 lsmith77 deleted the zend-diagnostics branch Apr 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment