Description
The upcoming Symfony 5 release allows us to modernize out method signatures by adding return types to them. I was under the impression that adding return types is one of your goals for 5.0, but I haven't found an issue that tracks the progress, so I'm opening one.
Adding return types to non-final methods is a clear BC break: child classes that override such a method are going to break unless they have the same return type. Since we're going to change a lot of code, we should split the work into smaller parts, if possible. My suggestion would therefore be to add types by component, like we did with parameter types in #32179. We could use this issue to track the work on the individual components.
I have already experimented a bit with adding return types to some components. Here's what I've gathered so far.
Tool support
Thanks to the great work by @fancyweb, we already have a deprecation system implemented in our DebugClassLoader, see #30323. With this system in place, we could start to add types to 5.0 and test how well it supports the migration path of real applications.
There's a lot of code to migrate, so automating this as much as possible would be desirable. I've tried to use the following PHP CS Fixer ruleset in my tests:
{
"no_superfluous_phpdoc_tags": {
"allow_mixed": true
},
"phpdoc_align": true,
"phpdoc_to_return_type": true,
"phpdoc_trim": true,
"phpdoc_trim_consecutive_blank_line_separation": true,
"void_return": true
}However, PHP CS Fixer is by design not able to analyze a class inheritance tree. The settings above tells CS Fixer simply to turn all @return annotations into a return type declarations. If a method does not have a @return annotation, e.g. because it uses @inheritdoc instead, PHP CS Fixer will skip it. This will lead to broken code, currently.
Especially when dealing with interfaces that have many implementations, PHPStorm's "change signature" refactoring can be used to add a return type to a method and all its child implementations. As far as I can tell, this has to be done on each method individually. I haven't found a way to automate this feature.
Even with the best tooling, we probably will need to fix some things manually.
Dock blocks Vs. type declarations
Dock blocks might be missing or lying. In that case, they should be added/fixed in 4.4, 4.3 or 3.4. In a perfect world, the return type declaration of a method in 5.0 would match the @return annotation in 4.4.
The doc block might also be more specific than the return type declaration should be, e.g.:
symfony/src/Symfony/Bundle/FrameworkBundle/Client.php
Lines 107 to 114 in bc79cfe
On inherited methods, we must declare the return type of the parent method. The code above will break, if we ran CS Fixer with the config mentioned earlier on this class and its parent.
Inconsistent return points
In php 5, return; and return null; were equivalent, but this has changed with the introduction of return type declarations. Currently, we still mix void return statements and with return statements carrying a value. Those methods will break if we add a return type declaration. See this comment for details.
If adding a return type declaration requires us to fix a method with inconsistent return points, we should apply the necessary change to a lower branch.
The void return type
We need to develop a common understanding of whether we should add the void return type to our methods. I personally would be in favor of it, but especially @nicolas-grekas has expressed his doubts in the past.
Before we start adding return type declarations, we need to agree on if and in what cases we want to add void.
Tests
Our tests should tell us if the new return type declarations break existing behavior. Rule of thumb: If a test needs to be changed, we have changed the behavior behavior of a class which is not the goal of adding return types.
Tests might also break because of broken mocks: The value returned by a mocked method has to match the new return type declaration.
- If no value is specified for a mocked method, PHPUnit will by default return
null. After adding a not-nullable return type, PHPUnit will create a mock for the return type instead. This might change the behavior of the test. - If the test specifies a return value for a mocked method that is incompatible with the new return type declaration, the test will break.
Example (return value is supposed to be an array, mock returns a string):
If a test turns out to be broken, it should be fixed on 4.4, 4.3 or 3.4. We should ideally be able to run the same test on 4.4 and master.
Compatibility of 4.4 and 5.0 components
Adding return types to a component 5.0 might create incompatibilities with 4.4 components. For example, let's assume the array return type is added to EventSubscriberInterface.
Many components bundle non-final event listeners implementing that interface. The 4.4 version of those classes would become incompatible with the 5.0 version of the interface and there's nothing we can do about it.
When we add return types to a component in 5.0, we might at the same time need to adjust composer.json files in 4.4.
Consequences for bundle vendors
A bundle that implements Symfony interfaces will be forced to add return type declarations in order to gain Symfony 5 compatibility. There are on the other hand still bundles out there that still support php 5 (see symfony/monolog-bundle#317). Because return type declarations is a syntax element unavailable in php 5, those bundles would be required to bump their own minimal php requirement to at least 7.0 (7.1 if they need a void or nullable return type, 7.2 is they need the object return type).
We could decide to either live with this (after all, nobody should be using php 5 anymore in 2019/2020 anyway) or to define a set of interfaces commonly used by third-party bundles. That set would not receive return types in order to enable wider compatibility ranges.
tl;dr
I'd like to add return type declarations to Symfony's codebase for 5.0. With this issue, I'd like to track the progress of this goal.