Problem/Motivation

In #3107732: Add return typehints to setUp/tearDown methods in concrete test classes we added return type hints to concrete test classes. Unfortunately we also accidentally included Drupal\Tests\migrate\Unit\process\MigrateProcessTestCase which is an abstract base class used in contrib, plus a few other abstract classes that might be extended elsewhere.

Proposed resolution

Remove void type hints all abstract test classes.

Remaining tasks

Discover the full set of classes.

User interface changes

API changes

Downstream users of MigrateProcessTestCase will no longer need to implement typehints.

Downstream users of other abstract classes will no longer need to implement typehints.

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 interdiff-5-10.txt568 bytesjungle
#10 3131402-10.patch4.28 KBjungle
#5 3131402-5.patch3.34 KBlongwave
#3 3131402.patch589 byteslongwave

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Parent issue: » #3107732: Add return typehints to setUp/tearDown methods in concrete test classes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 3131402.patch589 bytes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: Remove return type hints from MigrateProcessTestCase » Remove return type hints from abstract test classes
Issue summary: View changes

Scope widened to include more classes, as per the below list - some false positives here though

$ grep -rl 'setUp(): void' core|xargs grep ^abstract
core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php:abstract class SqlContentEntityStorageTestEntityInterface implements EntityInterface {
core/tests/Drupal/Tests/Core/Entity/EntityFieldManagerTest.php:abstract class EntityTypeManagerTestEntity implements \Iterator, ContentEntityInterface {
core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php:abstract class TestEntityHandlerBase extends EntityHandlerBase {
core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php:abstract class MTimeProtectedFileStorageBase extends PhpStorageTestBase {
core/modules/migrate/tests/src/Unit/process/MigrationLookupTestCase.php:abstract class MigrationLookupTestCase extends MigrateProcessTestCase {
core/modules/tour/tests/src/Functional/TourTestBasic.php:abstract class TourTestBasic extends TourTestBase {
core/modules/locale/tests/src/Functional/LocaleUpdateBase.php:abstract class LocaleUpdateBase extends BrowserTestBase {
core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php:abstract class MediaSourceTestBase extends MediaJavascriptTestBase {
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 3131402-5.patch3.34 KB
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/tour/tests/src/Functional/TourTestBasic.php
@@ -39,7 +39,7 @@
 
-  protected function setUp(): void {
+  protected function setUp() {
     parent::setUp();
 
     // Make sure we are using distinct default and administrative themes for
diff --git a/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php b/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php

diff --git a/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php b/core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php
index 166ad9b57f..cd68de55a3 100644

this one is a bit strange as there's only a single subclass, but doesn't matter too much. Better one too much where we don't add the return type than the opposite and breaking some contrib.

List of changed classes makes sense based on the output above. RTBC if it passes tests.

As maintainer of paragraphs, I'd appreciate this being committed as I'm otherwise forced to add : void to paragraphs.module which would require PHP 7.1.

As discused in slack, removing them again should not be an issue, even if some modules already updated their sbuclasses. Subclasses can define the return type, or the original issue of adding it to all non-base classes wouldn't have worked in the first place.

heddn’s picture

heddn
Primary language English
Location Illinois
commented

+1 on RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 3131402-5.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

need to remove it from \Drupal\Tests\media\Functional\MediaFunctionalTestTrait::setUp too - which imo makes sense.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs work » Needs review
StatusFileSize
new 3131402-10.patch4.28 KB
new interdiff-5-10.txt568 bytes

Addressed #9. Testing failure should be fixed.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs review » Reviewed & tested by the community

As testings passed, per comments in #6 and #7, I am setting this back to RTBC. If this is counted as self-RTBC, please feel free to revert it back. Thanks!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

RTBC +1 :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 9632fb98a1 to 9.1.x and ef544cd00b to 9.0.x. Thanks!

  • alexpott committed 9632fb9 on 9.1.x 
    Issue #3131402 by longwave, jungle, Berdir: Remove return type hints...

  • alexpott committed ef544cd on 9.0.x 
    Issue #3131402 by longwave, jungle, Berdir: Remove return type hints...

Status: Fixed » Closed (fixed)

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