[Console] Add the typehint on Command class, to be coherent with BC Break on 4.4 #39182
Conversation
| @@ -159,7 +159,7 @@ protected function configure() | |||
| * | |||
| * @see setCode() | |||
| */ | |||
| protected function execute(InputInterface $input, OutputInterface $output) | |||
| protected function execute(InputInterface $input, OutputInterface $output): int | |||
There was a problem hiding this comment.
This is a BC break. We cannot do this before Symfony 6.
There was a problem hiding this comment.
Yep, but as I said, it was already deprecated in 4.4 : see #33775
I think it was forgotten when the switch was made.
There was a problem hiding this comment.
it was deprecated to return null indeed. And that's indeed forbidden in 5.1.0. But it is perfectly valid to write code in 5.1.0 which always return an int but without the return type. and that's where the BC break is.
There was a problem hiding this comment.
Fair enough. I'll wait to propose this for 6 then (if I didn't forgot 😂 ).
There was a problem hiding this comment.
we won't because there are many methods that will benefit from a similar change and are waiting for v6
|
The issue is that this is technically a BC break, as it is possible to write code on 5.1.0 without the typehint applied (and child classes are allowed to restrict the return type, but not to widen it). |
|
Yep, but as it was already mentionned and documented on 4.4, shouldn't it be fine ? Otherwise this is just pendantics IMO and can wait for 6 |
|
I am wondering why it wasn't done in 5.0 in the first place, since it was deprecated in 4.4 and it was perfect moment for changing signature 🤔 But as far as I see it wasn't added in 6.* too... 🙄 |
|
This was not done in 5.0, because adding native return types in a way allowing to support multiple versions of Symfony requires the variance rules added in PHP 7.2, and Symfony 4.4 was supporting PHP 7.1 (meaning that it was not possible to write code working on PHP 7.1 and Symfony 4.4 that was also supporting the return type added in 5.0 if we were adding it). |
|
Thanks @stof for the info. I suppose you're talking about external packages having I imagine this would be edge case requiring at least 3 levels of dependency: project requiring package, that requires Symfony with multiple versions supported, am I right? So intermediate package does not decide about PHP version where it's used and about Symfony version resolved. In this case, having return type in Symfony v5 would block interim packages from supporting multiple versions of Symfony and in the end those would need to drop support of some PHP versions. Correct me if I'm wrong, I'm just thinking out loud because I want to understand it properly 🙂 |
There was a deprecation on 4.4, stating that the
executemethod should only return an int (0for success). It wasn't applied on 5.1, so here goes.I also removed the
TypeErrorif theexecutemethod doesn't return anint, as php should do the job if the typehint is not set.