#170 Strict null check of result message in console output #193
Conversation
| @@ -57,7 +57,8 @@ public function onAfterRun(CheckInterface $check, ResultInterface $result, $chec | |||
|
|
|||
| $this->output->write(sprintf(' %s', $check->getLabel())); | |||
|
|
|||
| if ($message = $result->getMessage()) { | |||
| $message = $result->getMessage(); | |||
| if (null !== $message) { | |||
There was a problem hiding this comment.
in theory null should never be returned
https://github.com/zendframework/zend-diagnostics/blob/master/src/Result/ResultInterface.php#L15
@weierophinney: but I suspect that this is just an incorrect docblock?
There was a problem hiding this comment.
I just looked at the AbstractResult, and the $message property is never set to a value if no message is passed. We should likely be initializing it as an empty string. (We really need to update that library to PHP 7.1+, as that would make it easier to identify these signature errors!)
Would you be willing to create a PR against zend-diagnostics that would initialize the message to an empty string, please, mentioning this issue?
There was a problem hiding this comment.
Hi, but even if I modify the docblock in AbstractResult and init the message to empty string by default, then this PR is still needed (empty string evaluates to boolean false).
I can imagine that zend-diagnostics will take forever to merge, but this fix is a quick one.
There was a problem hiding this comment.
Would you be willing to create a PR against zend-diagnostics that would initialize the message to an empty string, please, mentioning this issue?
There was a problem hiding this comment.
I guess with your PR to zend diagnostic this change would no longer be necessary?
There was a problem hiding this comment.
Yes, if it’s going to be accepted. Strictly speaking, the proposed PR in zend is a BC-breaking change and with a very slight chance may break someone else code, so I won’t be surprised if it will be declined in such form.
Any way, thank you for review. I will close this PR and open another one to update zend library as soon as it’s merged and tagged.
There was a problem hiding this comment.
I'm reviewing it now. 😄
In terms of the potential BC breakage:
- It's documented as returning a string.
- None of the shipped result implementations vary from what
AbstractResultdoes. - All reporters assume that method returns a string.
As such, I'm treating it as a bugfix.
There was a problem hiding this comment.
Just released the patch as version zend-diagnostics 1.3.1.
There was a problem hiding this comment.
Thank you for review. I will check the rest of the issues if I have time. Actually just intended to use your bundle cause it’s cool but found a minute to contribute. Good luck :)
|
I will open another one with updated zend-diagnostics library. Update: I've just realised that no PR is required because the bundle is not shipped with composer.lock (which is good of course). So it's up to the users of the bundle to update. |
Sorry for missing unit test, but this class wasn't covered before anyway ¯_(ツ)_/¯