-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add phpdoc return type to v6 codebase to prepare v7 typehint #47551
Comments
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 |
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. |
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. |
This PR was merged into the 6.3 branch. Discussion ---------- Add many missing PHPdoc return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | yes | Tickets | Ref #47551, continues #49342 | License | MIT | Doc PR | - This PR adds lots of ``@return`` PHPdoc that we missed in the 5.4 branch. These will produce new deprecation warnings, if applications override one of these methods. It will not break backwards compatibility. Adding these PHPdoc in 6.3 already allow the community to test all these annotations for 6 months in stable version, before committing to adding them as PHP types in 7.0. This way, I hope we patch out any wrong type. This PR is done using a slightly modified Psalter script, based on Psalm's type inference. See #49348 for the counter PR adding real PHP types to private, final or internal methods. Commits ------- e23d8be Add missing PHPdoc return types
This PR was merged into the 6.3 branch. Discussion ---------- Add PHP types to private methods and functions | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | yes | Tickets | Ref #47551, continues #49342 | License | MIT | Doc PR | - This PR adds real PHP types to private, internal or final (excluding ``@final``) methods based on Psalm's type inference. Many more return types are missing still, but let's do them in other contributions. This change can result in BC breaks in multiple ways, so we should carefully review the changes: * A method may wrongly be considered "safe" by Psalter. If the method is unsafe (e.g. ``@final``, but not yet real `final`), we should instead add a PHPdoc return type (see #49349) * Psalm may not infer the correct set of return types. If an unexpected type is returned, this will result in a PHP error. Commits ------- 384c9a6 Add PHP types to private methods and functions
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 full list of method names
|
I think we should wait for the |
Agreed (I created this list from the void PR branch, so it should be more or less correct). |
In #49347, I manually added 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 |
Why should an implementation of this method return anything anyway? |
Some do in the codebase 🤷 |
Maybe they should stop doing that. 😬 |
This PR was merged into the 6.3 branch. Discussion ---------- Add void (PHPdoc) return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Ref #47551, continues #49342 | License | MIT | Doc PR | - This introduces lots of void return types to the codebase, following our policy: * Adding ``@return` void` to any methods that return void is OK * Adding `void` PHP return type is only OK if the method is internal, private or final (excluding ``@final``). The changes in this PR are automated through a modified Psalter script. We should particularly review if ``@final`` classes/methods didn't get real PHP void return types, as this is a BC break. Commits ------- 508ed81 Add void return types
Alright, current statistics of missing return types: 360 interface methods, implemented by 905 class methods
243 standalone class methods
|
This might be interesting here...
I guess that PHPUnit's magic copies the type from
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 |
…urn types (wouterj) This PR was merged into the 6.3 branch. Discussion ---------- [HttpFoundation][HttpKernel] Add missing void PHPdoc return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no (void return types don't get a deprecation notice) | Tickets | Fix part of #47551 , ref #49439 | License | MIT | Doc PR | - Commits ------- 9bb72d7 [HttpFoundation][HttpKernel] Add missing void PHPdoc return types
This PR was merged into the 6.3 branch. Discussion ---------- [Security] Add missing void PHPdoc return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | contribution to #47551 | License | MIT | Doc PR | - Add missing void PHPdoc return types. Not sure if in the right place, but remove return portage of FirewallListenerInterface::authenticate() in WrappedLazyListener::authenticate(), since the return is supposed to be void. Commits ------- b2e9511 [Security] Add missing void PHPdoc return types
This PR was merged into the 6.3 branch. Discussion ---------- [Validator] Add PHPDoc void return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | contribution to #47551 | License | MIT | Doc PR | - Add missing void PHPdoc return types for the Validator component Commits ------- e70241d [Validator] Add PHPDoc void return types
This PR was squashed before being merged into the 6.3 branch. Discussion ---------- [Serializer] Add missing return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Contribution for #47551 | License | MIT | Doc PR | Contribution on the issue #47551 concerning the **Serializer** component. The type is added on PHPDoc when BC risk is present, else native type is used (private methods / tests). Commits ------- d9f2365 [Serializer] Add missing return types
This PR was squashed before being merged into the 6.3 branch. Discussion ---------- [VarDumper] Add missing return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Contribution for #47551 | License | MIT | Doc PR | Contribution on the issue #47551 concerning the **VarDumper** component. The type is added on PHPDoc when BC risk is present, else native type is used (private methods / tests). Commits ------- 208df61 [VarDumper] Add 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. |
This PR was merged into the 6.3 branch. Discussion ---------- [Workflow] Adding PHP docs return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | contribution to #47551 | License | MIT | Doc PR | - Add missing PHPdoc return types for the Workflow component. This is my first contribution, be kind 😇 Commits ------- 29035b4 [Workflow] Adding more return types
This PR was merged into the 6.3 branch. Discussion ---------- [Lock] Add the void return type in Lock interfaces | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | n/a Contributes to #47551 Commits ------- 66dd1ad Add the void return type in Lock interfaces
This PR was merged into the 6.3 branch. Discussion ---------- [Lock] Add the void return type in Lock interfaces | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | n/a Contributes to #47551 Commits ------- c7eab24 Add the void return type in Lock interfaces
…stof) This PR was merged into the 6.3 branch. Discussion ---------- Add the void return type in the Translation interfaces | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | n/a | License | MIT | Doc PR | n/a Contributes to #47551 Commits ------- d96c0f0 Add the void return type in the Translation interfaces
…erj) This PR was squashed before being merged into the 6.3 branch. Discussion ---------- Add CI check ensuring interfaces have return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Ref #47551 | License | MIT | Doc PR | - This adds a CI check to enforce all methods on an interface to have a return type (native or PHPdoc). For now, this is a living version of the list I published in #47551. I'll rebase this PR whenever we add more types. Commits ------- 29ea860 Add CI check ensuring interfaces have return types
…erj) This PR was merged into the 6.3 branch. Discussion ---------- Add remaining missing return types to safe methods | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Ref #47551 | License | MIT | Doc PR | - This adds return types to remaining methods that are safe to receive a return type (final, internal or private ones), except from methods of `DataCollector` classes (which are missing quite a lot of types). I couldn't figure out whether they all return `Data`, the actual value or a union of both, so I've left them out for now. Asides from these, we're missing 121 more return types on "non-safe" methods. If I have time, I'll create some smaller PRs for them to allow easier reviews. <details> <summary>Full list</summary> ```120 missing return types on interfaces Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass\RegisterEventListenersAndSubscribersPass::process Symfony\Bridge\Doctrine\Form\DoctrineOrmTypeGuesser::getMetadata Symfony\Bridge\Monolog\Handler\ConsoleHandler::setOutput Symfony\Bridge\Monolog\Handler\ConsoleHandler::onCommand Symfony\Bridge\Monolog\Handler\ConsoleHandler::onTerminate Symfony\Bridge\Monolog\Handler\ServerLogHandler::createSocket Symfony\Bundle\FrameworkBundle\Command\AbstractConfigCommand::listBundles Symfony\Bundle\FrameworkBundle\Command\AbstractConfigCommand::validateConfiguration Symfony\Bundle\FrameworkBundle\DependencyInjection\Configuration::addHttpClientRetrySection Symfony\Bundle\FrameworkBundle\DependencyInjection\Configuration::addRemoteEventSection Symfony\Bundle\FrameworkBundle\DependencyInjection\FrameworkExtension::registerWebhookConfiguration Symfony\Bundle\FrameworkBundle\DependencyInjection\FrameworkExtension::registerRemoteEventConfiguration Symfony\Bundle\SecurityBundle\DependencyInjection\SecurityExtension::createContextListener Symfony\Component\BrowserKit\AbstractBrowser::getScript Symfony\Component\Cache\Adapter\ArrayAdapter::freeze Symfony\Component\Cache\Adapter\ArrayAdapter::unfreeze Symfony\Component\Cache\Adapter\MemcachedAdapter::checkResultCode Symfony\Component\Cache\DependencyInjection\CachePoolPass::process Symfony\Component\Config\Definition\BaseNode::validateType Symfony\Component\Config\Definition\PrototypedArrayNode::addChild Symfony\Component\Config\FileLocator::locate Symfony\Component\Config\Loader\FileLoader::setCurrentDir Symfony\Component\Config\Loader\FileLoader::import Symfony\Component\Config\Loader\FileLoader::doImport Symfony\Component\Config\ResourceCheckerConfigCache::safelyUnserialize Symfony\Component\Console\Command\LockableTrait::release Symfony\Component\Console\Formatter\OutputFormatter::formatAndWrap Symfony\Component\Console\Helper\Helper::formatTime Symfony\Component\Console\Helper\Table::setStyleDefinition Symfony\Component\Console\Helper\Table::render Symfony\Component\Console\Helper\Table::renderRowSeparator Symfony\Component\Console\Helper\Table::renderRow Symfony\Component\Console\Helper\Table::calculateNumberOfColumns Symfony\Component\Console\Helper\Table::calculateColumnsWidth Symfony\Component\Console\Helper\Table::cleanup Symfony\Component\Console\Input\Input::parse Symfony\Component\Console\Input\Input::getStream Symfony\Component\Console\Output\Output::doWrite Symfony\Component\DependencyInjection\Argument\ReferenceSetArgumentTrait::setValues Symfony\Component\DependencyInjection\Container::make Symfony\Component\DependencyInjection\Container::load Symfony\Component\DependencyInjection\Extension\Extension::getXsdValidationBasePath Symfony\Component\DependencyInjection\Extension\Extension::getNamespace Symfony\Component\DependencyInjection\Extension\Extension::getConfiguration Symfony\Component\DependencyInjection\Loader\YamlFileLoader::parseDefinition Symfony\Component\DependencyInjection\ParameterBag\FrozenParameterBag::clear Symfony\Component\DependencyInjection\ParameterBag\FrozenParameterBag::add Symfony\Component\DependencyInjection\ParameterBag\FrozenParameterBag::set Symfony\Component\DependencyInjection\ParameterBag\FrozenParameterBag::deprecate Symfony\Component\DependencyInjection\ParameterBag\FrozenParameterBag::remove Symfony\Component\DependencyInjection\ParameterBag\ParameterBag::isResolved Symfony\Component\DomCrawler\AbstractUriElement::setNode Symfony\Component\DomCrawler\Field\FormField::initialize Symfony\Component\ExpressionLanguage\Compiler::getFunction Symfony\Component\ExpressionLanguage\Node\Node::toArray Symfony\Component\ExpressionLanguage\Parser::parseExpression Symfony\Component\ExpressionLanguage\Parser::getPrimary Symfony\Component\ExpressionLanguage\SerializedParsedExpression::getNodes Symfony\Component\Filesystem\Filesystem::linkException Symfony\Component\Form\AbstractType::getBlockPrefix Symfony\Component\Form\AbstractType::getParent Symfony\Component\Form\ButtonBuilder::setFormFactory Symfony\Component\Form\Extension\Core\DataAccessor\PropertyPathAccessor::getPropertyValue Symfony\Component\Form\Extension\Core\Type\ChoiceType::buildForm Symfony\Component\Form\Extension\Core\Type\ChoiceType::buildView Symfony\Component\Form\Extension\Core\Type\ChoiceType::finishView Symfony\Component\Form\Extension\Core\Type\ChoiceType::configureOptions Symfony\Component\Form\Extension\Core\Type\ChoiceType::addSubForms Symfony\Component\Form\Extension\Core\Type\ChoiceType::addSubForm Symfony\Component\Form\Extension\Core\Type\DateType::buildForm Symfony\Component\Form\Extension\Core\Type\DateType::finishView Symfony\Component\Form\Extension\Core\Type\DateType::configureOptions Symfony\Component\Form\Extension\Core\Type\FileType::buildForm Symfony\Component\Form\Extension\Core\Type\FileType::buildView Symfony\Component\Form\Extension\Core\Type\FileType::finishView Symfony\Component\Form\Extension\Core\Type\FileType::configureOptions Symfony\Component\Form\Extension\Core\Type\MoneyType::getPattern Symfony\Component\Form\Extension\Validator\ViolationMapper\ViolationPathIterator::mapsForm Symfony\Component\HttpClient\Response\StreamWrapper::stream_cast Symfony\Component\HttpFoundation\Request::prepareRequestUri Symfony\Component\HttpKernel\DependencyInjection\ConfigurableExtension::loadInternal Symfony\Component\Ldap\Security\LdapUserProvider::getAttributeValue Symfony\Component\Mailer\Bridge\Mailjet\Transport\MailjetApiTransport::castCustomHeader Symfony\Component\Mime\Email::ensureValidity Symfony\Component\Process\Exception\ProcessFailedException::getProcess Symfony\Component\Process\Exception\ProcessTimedOutException::getProcess Symfony\Component\Process\Exception\ProcessTimedOutException::getExceededTimeout Symfony\Component\Process\Process::start Symfony\Component\Process\Process::checkTimeout Symfony\Component\Process\Process::setOptions Symfony\Component\Process\Process::updateStatus Symfony\Component\Process\Process::readPipesForOutput Symfony\Component\Process\Process::readPipes Symfony\Component\Process\Process::resetProcessData Symfony\Component\Process\Process::requireProcessIsStarted Symfony\Component\Process\Process::requireProcessIsTerminated Symfony\Component\Routing\Loader\AnnotationClassLoader::configureRoute Symfony\Component\Routing\Matcher\UrlMatcher::getExpressionLanguage Symfony\Component\Routing\Router::getRouteCollection Symfony\Component\Security\Core\User\InMemoryUserProvider::createUser Symfony\Component\Security\Http\Firewall::getSubscribedEvents Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::supportsNormalization Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::normalize Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::instantiateObject Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::supportsDenormalization Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::denormalize Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::setAttributeValue Symfony\Component\Translation\Catalogue\AbstractOperation::processDomain Symfony\Component\Translation\Command\XliffLintCommand::display Symfony\Component\Translation\Command\XliffLintCommand::displayJson Symfony\Component\Translation\Command\XliffLintCommand::getDirectoryIterator Symfony\Component\Translation\Command\XliffLintCommand::isReadable Symfony\Component\Translation\Extractor\PhpExtractor::seekToNextRelevantToken Symfony\Component\Translation\Extractor\PhpExtractor::skipMethodArgument Symfony\Component\Translation\Extractor\PhpExtractor::parseTokens Symfony\Component\Translation\Util\ArrayConverter::getElementByPath Symfony\Component\Validator\Constraints\IsbnValidator::getMessage Symfony\Component\Validator\Exception\ValidationFailedException::getValue Symfony\Component\VarDumper\Test\VarDumperTestTrait::assertDumpEquals Symfony\Component\VarDumper\Test\VarDumperTestTrait::assertDumpMatchesFormat ``` </details> Commits ------- 4f80606 Add remaining missing return types to safe methods
…types (wouterj) This PR was merged into the 6.3 branch. Discussion ---------- [BrowserKit][Cache][Config][Console] Add missing return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Ref #47551 | License | MIT | Doc PR | - Commits ------- f0b9511 Add missing return types
…types (wouterj) This PR was merged into the 6.3 branch. Discussion ---------- [BrowserKit][Cache][Config][Console] Add missing return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Ref symfony/symfony#47551 | License | MIT | Doc PR | - Commits ------- f0b9511474 Add missing return types
…lesystem][Form] Add missing return types (wouterj) This PR was merged into the 6.3 branch. Discussion ---------- [DependencyInjection][DomCrawler][ExpressionLanguage][Filesystem][Form] Add missing return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Ref #47551 | License | MIT | Doc PR | - Commits ------- 66c2147 Add missing return types
…ime][Process][Routing][Security][Serializer][Translation][Validator] Add missing return types (wouterj) This PR was merged into the 6.3 branch. Discussion ---------- [HttpClient][HttpFoundation][HttpKernel][Ldap][Mailer][Mime][Process][Routing][Security][Serializer][Translation][Validator] Add missing return types | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | yes | Tickets | Ref #47551 | License | MIT | Doc PR | - Last one, for the components. Only some bundle methods are missing now. Commits ------- f86dbbd Add missing return types
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 #47480 (comment) on a small PR
Friendly ping @stof @fabpot as originals reviewers
Questions:
Thank you
Example
Symfony v4/5/6
Symfony v7
The text was updated successfully, but these errors were encountered: