Closed 94noni closed 1 year ago
We've introduced a type patcher utility in Symfony 5.4. This utility either added type declarations, or added PHPdoc return types (to give the community more time to upgrade). See https://github.com/symfony/symfony/blob/6.2/.github/expected-missing-return-types.diff#L1-L7 for more information about this tool and the expected missing types.
Obviously, this tool has missed some methods. We should tackle those in Symfony 6.x, so Symfony 7 is fully typed. I did a little experiment locally, first running the type patcher tool and then running the MissingReturnType check of Psalm. Psalm finds 2632 methods without a return type (excluding tests - we don't care about them).
I'm not sure how we (=Symfony contributors) should add them. We can try to use Psalter to inform the type declarations and then somehow transform them into @return
PHPdoc. Or we can try writing a Rector rule to add the PHPdoc? @nicolas-grekas you added a bulk of return PHPdoc in 5.x, did you use a custom script or something to do this?
Maybe itβs preferable to not add types for specific methods for performance reasons (when they are called ment times)
@wouterj AFAIK, the types added in 5.x were added by the patcher tool (that's why it was written).
Regarding the methods returned by Psalm, is it about running the patcher in force mode (which will convert phpdoc-only return types to native types when possible) ?
closing as not much interest so far on this
That's an interesting topic, let's reopen.
Fyi, I see Psalter has an option to generate missing PHPdoc. I'll try that command soon.
@fabpot @wouterj hi, thx for writing back @wouterj feel free to ping me if you need help/testing the PR etc Regards
I also believe we should visit the return type topic once again. PHP's type system has reached a level where there's little reason to have methods without a declared return type. For Symfony 7 we could even have automated checks that cause a red build if a PR tries to add a method without native return type.
In 3 PRs, we've added many more return types. For anyones information, there are still 1618 methods left that don't yet have a return type or @return
.
there are still 1618 methods left
I think we should wait for the : void
PR to be merged before anyone starts to pick methods from that list. π
I think we should wait for the : void PR to be merged before anyone starts to pick methods from that list. sweat
Agreed (I created this list from the void PR branch, so it should be more or less correct).
In #49347, I manually added @return void
to a few interfaces (DataCollectorInterface, LateDataCollectorInterface and ResetInterface).
I think it would be great to have a PR dedicated to ensuring that all interfaces have explicit return types for all their methods.
Note that ResetInterface::reset()
is an interesting case because we cannot document its return type as mixed|void
.
Note that
ResetInterface::reset()
is an interesting case because we cannot document its return type asmixed|void
.
Why should an implementation of this method return anything anyway?
Some do in the codebase :shrug:
Maybe they should stop doing that. π¬
Alright, current statistics of missing return types:
This might be interesting here...
We tried to use the HttpClient and mock a response for PHPUnit but did so by calling static::createMock(MockResponse::class)
(i know :upside_down_face:). Which lead to:
Fatal error: Method Mock_MockResponse_c3de0118::__destruct() cannot declare a return type in /application/vendor/phpunit/phpunit/src/Framework/MockObject/MockClass.php(51) : eval()'d code on line 276
I guess that PHPUnit's magic copies the type from MockResponse::__destruct()
which is actually:
use TransportResponseTrait {
doDestruct as public __destruct;
}
So should the type (added in 6.1) of the method of the trait be removed again just to be safe?
I would report that as a bug in PHPUnit, as the return type being declared is void
, and that one should be valid on __destruct
And if it is PHP that forbids return types in __destruct
, we should maybe avoid doDestruct as public __destruct;
and instead declaring __destruct
by calling doDestruct
I took care of MockResponse in #49407
*ExceptionInterface::getCode
are false positives, we don't need to document their return type.
These also, because the interface has an @template
annotation:
Symfony\Contracts\HttpClient\ResponseStreamInterface::next
Symfony\Contracts\HttpClient\ResponseStreamInterface::valid
Symfony\Contracts\HttpClient\ResponseStreamInterface::rewind
Might be the case for more Iterator-related methods.
Yes, my script uses ReflectionClass::getMethods()
, which apparently includes methods defined by PHP internal classes as well (as is the case for Exception::getCode()
.
Yes, my script uses
ReflectionClass::getMethods()
, which apparently includes methods defined by PHP internal classes as well (as is the case forException::getCode()
.
Same goes for interfaces extending ArrayAccess
and friends. Can you limit that script to methods declared or overridden by a given interface, ignoring those that are just inherited?
ReflectionClass::getMethods()
indeed includes inherited methods. You need to check the declaring class of the methods
Seeing from the outside, the work done by all of you is very impressive ππ» ππ»
Thanks for the hint @stof.
I've edited my script slightly so we can use it as a CI check enforcing return types on interface: #49439 For now, that PR represents a living list of missing return types :)
To anyone willing to help on this issue: tests don't need to be updated, unless required by the changes on non-test files. We don't care about provider or methods having types in tests.
For everyone helping here: Thank you!
Symfony 6.3 will be released with return types, either PHP ones or PHPdoc ones, for all methods in the bridges and components and for all interfaces of Symfony.
Description
Based on this comment https://github.com/symfony/symfony/pull/47480#issuecomment-1238977713 on a small PR Friendly ping @stof @fabpot as originals reviewers
Questions:
Thank you
Example
Symfony v4/5/6
Symfony v7