Problem/Motivation

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:

Proposed resolution

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;

return Rector\Config\RectorConfig::configure()
  ->withPaths([
    __DIR__ . '/core/tests',
  ])
  // First level before param and property types are affected.
  ->withTypeCoverageLevel(22)
  ->withSkip([
    // Skip as this adds unwanted phpdoc changes.
    ClassMethod\ReturnTypeFromStrictNewArrayRector::class,
    ClassMethod\ReturnTypeFromMockObjectRector::class,
    TypedPropertyFromCreateMockAssignRector::class,
  ])
  // Additional rules above level 22 that are useful
  ->withRules([
    ClassMethod\ReturnTypeFromStrictTypedCallRector::class,
    ClassMethod\ReturnUnionTypeRector::class,
    ClassMethod\ReturnTypeFromStrictParamRector::class,
    ClassMethod\ReturnTypeFromStrictFluentReturnRector::class,
    ClassMethod\ReturnNeverTypeRector::class,
  ]);

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

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#15 3551596-nr-bot_t7h_ls7m.txt91 bytesneeds-review-queue-bot
#9 3551596-nr-bot_jtn949le.txt125.1 KBneeds-review-queue-bot

Issue fork drupal-3551596

Command icon Show commands

Start 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:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes

mondrake opened merge request !13463

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
nicxvan’s picture

nicxvan commented

I think these are great, I wonder if we can document these rector rules for contrib somewhere central.

dcam’s picture

dcam commented
Status: Needs review » Needs work

I left feedback in the MR.

I didn't make a suggestion for the first couple of ?object return types because in both instances a new use statement will be necessary too.

It made a bunch of lowercase `null` insertions. Not sure what was up with that. I ended up grepping the diff file to find them all. There should be suggestions for all of them.

Aside from those things, this looks ok to me. I read through the entire diff, but I didn't verify every single return type. I assume that the test would blow up with a fatal error if one happened to be wrong.

nicxvan’s picture

nicxvan commented

Is this a beta target change too?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Thank you!

The annotation code is under phpcs ignore so that's why, I assume, phpcbf does not make fixes there.

It made a bunch of lowercase `null` insertions. Not sure what was up with that. I ended up grepping the diff file to find them all. There should be suggestions for all of them.

I really wish we had #3439962: Stop using uppercase TRUE, FALSE, NULL literals. IMHO, spending time on this is waste of resources.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3551596-nr-bot_jtn949le.txt125.1 KB

The 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.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Feedback has been addressed. The additional changes to qualifying MockObjects look good to me too. And the tests are green. So I'm marking this one as RTBC.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Add return types to core tests code via Rector » Add return types to core tests code via Rector - round 1

Let's call this issue 'round 1'. A 'round 2' issue would increase type coverage level to max (I checked and that will add a lot more goodness, including argument typing based on inference). 'Round 3' or more would be a manual check/refactor of the leftovers at that stage.

mstrelan’s picture

mstrelan commented

This may be a duplicate or could be a child issue of #3455548: [meta] Add return typehinting to the test codebase or #3455549: Add return typehints to protected test helper methods.

I think the key consideration in that meta is "first where non-disruptive, then where possibly disruptive (base classes, traits, etc.)"

quietone’s picture

quietone commented
Title: Add return types to core tests code via Rector - round 1 » Add return types to core/tests code via Rector - round 1
Parent issue: #3550884: Add return types to test module code » #3455548: [meta] Add return typehinting to the test codebase

I do think the parent should change. The parent issue here is for test modules but this is for core/tests so I am changing the parent.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3551596-nr-bot_t7h_ls7m.txt91 bytes

The 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.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

rebased and updated baseline, back to RTBC

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Needs rebase.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community

Rebased and reverted changes to DocParserTest, back to RTBC.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +beta target
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

Two nits about a return type that should only be a string, and also why is this reverted/added to the baseline?

+$ignoreErrors[] = [
+       'message' => '#^Method Drupal\\\\Tests\\\\Component\\\\DependencyInjection\\\\Dumper\\\\PhpArrayDumperTest\\:\\:serializeDefinition\\(\\) has no return type specified\\.$#',
+       'identifier' => 'missingType.return',
+       'count' => 1,
+       'path' => __DIR__ . '/tests/Drupal/Tests/Component/DependencyInjection/Dumper/PhpArrayDumperTest.php',
+];
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
Issue tags: -beta target

serializeDefinition(array $service_definition) returns a string in OptimizedPhpArrayDumperTest and an array in its child PhpArrayDumperTest, which is quite weird for a serialization function. However, not in scope here to change the test logic, so typed the return value of both to string|array to catch all.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Would be good to land this one before needs another rebase.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

This is a good time to get sweeping fixes like this in.

Committed and pushed 73792d7654a to 11.x and dc40b4e721f to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • longwave committed dc40b4e7 on 11.3.x 
    task: #3551596 Add return types to core/tests code via Rector - round 1...

longwave closed merge request !13463

  • longwave committed 73792d76 on 11.x 
    task: #3551596 Add return types to core/tests code via Rector - round 1...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.