Adding return types to runtime code has BC concerns, but adding to test code should be straightforward and can be at least partially automated with rector.
My suggestion would be to do, for core/tests:
Use rector with type coverage on core/tests.
Install rector
$ composer require --dev rector/rector
Configure rector.php file
<?php
declare(strict_types=1);
use Rector\TypeDeclaration\Rector\ClassMethod;
use Rector\TypeDeclaration\Rector\Class_\TypedPropertyFromCreateMockAssignRector;
use Rector\TypeDeclaration\Rector\Class_\TypedPropertyFromDocblockSetUpDefinedRector;
use Rector\TypeDeclaration\Rector\Property\TypedPropertyFromAssignsRector;
use Rector\TypeDeclaration\Rector\Property\TypedPropertyFromStrictConstructorRector;
use Rector\TypeDeclaration\Rector\Property\TypedPropertyFromStrictSetUpRector;
return Rector\Config\RectorConfig::configure()
->withBootstrapFiles([
__DIR__ . '/vendor/palantirnet/drupal-rector/config/drupal-phpunit-bootstrap-file.php',
])
->withPaths([
__DIR__ . '/core/tests',
])
->withPreparedSets(typeDeclarations: true)
->withSkip([
TypedPropertyFromCreateMockAssignRector::class,
TypedPropertyFromDocblockSetUpDefinedRector::class,
TypedPropertyFromAssignsRector::class,
TypedPropertyFromStrictConstructorRector::class,
TypedPropertyFromStrictSetUpRector::class,
])
->withImportNames(
importDocBlockNames: false,
importShortClasses: false,
removeUnusedImports: false,
);
Run rector
$ ./vendor/bin/rector
Fix code style
$ composer phpcbf
Update baseline
$ ./vendor/bin/phpstan analyse -c core/phpstan.neon.dist --generate-baseline=core/.phpstan-baseline.php --memory-limit=2G
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !14078
Comments
Comment #2
mondrakeComment #4
quietone commentedComment #5
mondrakeComment #6
mondrakeCreated the patch as per the IS, then fixed manually a few errors until green.
The only change that goes a bit outside of pure typing is in
core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.phpwhere I simplified access to a protected property using reflection, since the addition of typehints to the closure was causing PHPStan complaints - I think it's more readable now anyway.Comment #7
mondrakeComment #8
mondrakeComment #9
smustgrave commentedIs there concern for breaking contrib with typehints on test traits?
Comment #10
dcam commentedTo reduce the amount of work required to review this I assumed that any individual scalar hint was valid. Also any union of two scalar types. Even then, it took 2-3 hours to go through all the changes, in particular because of the browser lag in GitLab. I focused on checking the unions of multiple types, particularly those that involved classes.
I commented on the unions with 5+ types. I also added notes about a couple of other things I found.
Comment #11
mondrakeThanks for your review @dcam!
Replied inline, made some changes, reverted changes to Doctrine annotation test fixtures.
Comment #12
dcam commentedI don't know the answer to #9. I planned to search all contrib code for the traits, but for some reason I can't. Traits that have functions with new parameter typehints are:
core/tests/Drupal/KernelTests/AssertConfigTrait.phpcore/tests/Drupal/KernelTests/AssertContentTrait.phpcore/tests/Drupal/Tests/Composer/Plugin/ExecTrait.phpcore/tests/Drupal/Tests/Composer/ComposerIntegrationTrait.phpcore/tests/Drupal/Tests/RandomGeneratorTrait.phpcore/tests/Drupal/Tests/UiHelperTrait.phpOtherwise this is looking good to me.
Comment #13
smustgrave commentedThis will need several re-rolls I'm sure but lets see if we can get it landed. Thanks!
Comment #14
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #15
dcam commentedRebased.
Comment #17
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #18
dcam commentedRebased
Comment #19
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #20
mondrakeComment #21
dcam commentedIt needed a rebase due to changes in
core/tests/Drupal/Tests/Component/Plugin/Factory/ReflectionFactoryTest.php.Comment #22
catchNeeds another rebase.
Comment #23
mondrakerebased
Comment #26
catchChanging the title because this also adds param type hints. I noticed that in some cases, rector is adding param type hints and not return type hints, however I think we can just add those in another issue later, main thing here is to tighten things up and reduce the baseline. Because it's test code it's easier to make smaller changes later.
Committed/pushed to main, thanks!