[Console] Add return type to OutputFormatterInterface::format() #42382
Conversation
|
Could you also fix the test i mentioned in #42381 (comment) to make more sense please. |
Signed-off-by: Alexander M. Turek <me@derrabus.de>
9c75bbb to
d75f5e6
Compare
|
Done. I'm really scratching my head over this class. It clearly does not behave as the author of the test thought it would. 🤔 |
| @@ -53,6 +53,8 @@ public function getStyle(string $name); | |||
|
|
|||
| /** | |||
| * Formats a message according to the given styles. | |||
| * | |||
| * @return string|null | |||
There was a problem hiding this comment.
I think this must be @return string and the null formatter should be return $message; instead.
There was a problem hiding this comment.
Keep in mind that this is not an internal class. You are suggesting to significantly change its behavior here while I'm merely documenting the status quo.
There was a problem hiding this comment.
Hmm, indeed. I'm not sure what is the best way forward.
If we officially document this contract as returning string|null, we have to update all code using this function to take care of the null return. Honestly, returning null (void actually) in NullOutputFormatter seems like a mistake and bug to me.
In fact, if code is build to use the normal OutputFormatter, it would be broken if NullOutputFormatter is used in the test. I can't think of any reasonable approach to build code to only work with NullOutputFormatter.
There was a problem hiding this comment.
I understand. But string|null is what currently is returned from the implementations. Once this PR is merged, we can think about how we could narrow it down to just string in 6.0. By the way, I'd also like to do this for the $message parameter.
There was a problem hiding this comment.
Should we depreciate null in 5.4 then? Then we can keep the implied contract, which is string IMHO
There was a problem hiding this comment.
I agree that the real issue is the implementation of NullOutputFormatter which was written before void was introduced. Conceptually, doing nothing was the same as returning null back then. Let's merge this one as is and see if we can deprecate null in a followup PR (same for *message).
|
Thank you @derrabus. |
Spotted while working on #42381. If we look at how
OutputFormatterInterface::format()is used in the codebase, we can see that an implementation of that method is supposed to return something. Yet the interface does not declare a return value and theNullOutputFormatterimplementation even has avoidreturn type which does not make sense at all, imho.This PR attempts to fix that.