Problem/Motivation

In #3575324: Add return type documentation to ResourceTestBase we added a lot of @return void annotations that required useless descriptions. Now that #3577137: Upgrade to Coder 9 is in we can remove descriptions for @return void, as per #3575389: Allow @return void to skip the description.

At the same time, there are other existing @return void that can be replaced by native return types.

Steps to reproduce

$ git grep "@return void" core
core/lib/Drupal/Component/Annotation/Doctrine/DocParser.php:     * @return void
core/lib/Drupal/Component/Annotation/Doctrine/DocParser.php:     * @return void
core/lib/Drupal/Component/Annotation/Doctrine/DocParser.php:     * @return void
core/lib/Drupal/Component/Annotation/Doctrine/DocParser.php:     * @return void
core/lib/Drupal/Component/Annotation/Doctrine/DocParser.php:     * @return void
core/lib/Drupal/Component/Annotation/Doctrine/DocParser.php:     * @return void
core/lib/Drupal/Component/Annotation/Doctrine/DocParser.php:     * @return void
core/lib/Drupal/Component/Annotation/Doctrine/DocParser.php:     * @return void
core/lib/Drupal/Component/Annotation/Doctrine/SimpleAnnotationReader.php:     * @return void
core/lib/Drupal/Component/Annotation/Doctrine/StaticReflectionParser.php:     * @return void
core/lib/Drupal/Component/Diff/DiffFormatter.php:   * @return void
core/lib/Drupal/Component/Diff/DiffFormatter.php:   * @return void
core/lib/Drupal/Component/Diff/DiffFormatter.php:   * @return void
core/lib/Drupal/Component/Diff/DiffFormatter.php:   * @return void
core/lib/Drupal/Component/Diff/DiffFormatter.php:   * @return void
core/lib/Drupal/Component/Diff/DiffFormatter.php:   * @return void
core/lib/Drupal/Component/Diff/DiffFormatter.php:   * @return void
core/lib/Drupal/Component/Diff/DiffFormatter.php:   * @return void
core/lib/Drupal/Component/Diff/DiffFormatter.php:   * @return void
core/modules/block/src/BlockPluginCollection.php:   * @return void
core/modules/locale/src/LocaleProjectRepository.php:   * @return void
core/modules/media/tests/src/Kernel/OEmbedIframeControllerTest.php:   * @return void
core/modules/media/tests/src/Kernel/OEmbedIframeControllerTest.php:   * @return void
core/modules/media/tests/src/Kernel/OEmbedIframeControllerTest.php:   * @return void
core/modules/node/src/NodeGrantDatabaseStorageInterface.php:   * @return void
core/modules/rest/tests/src/Functional/ResourceTestBase.php:   * @return void
core/modules/rest/tests/src/Functional/ResourceTestBase.php:   * @return void
core/modules/rest/tests/src/Functional/ResourceTestBase.php:   * @return void
core/modules/rest/tests/src/Functional/ResourceTestBase.php:   * @return void
core/modules/rest/tests/src/Functional/ResourceTestBase.php:   * @return void
core/tests/Drupal/KernelTests/Core/Config/ConfigEntityValidationTestBase.php:   * @return void
core/tests/Drupal/KernelTests/Core/Config/ConfigEntityValidationTestBase.php:   * @return void
core/tests/Drupal/TestTools/Extension/InfoWriterTrait.php:   * @return void
core/tests/Drupal/Tests/PerformanceData.php:   * @return void
core/tests/Drupal/Tests/PerformanceTestTrait.php:   * @return void
core/tests/Drupal/Tests/PerformanceTestTrait.php:   * @return void
core/tests/Drupal/Tests/WebAssert.php:   * @return void

Proposed resolution

  • Remove description from void methods in ResourceTestBase
  • Convert remaining methods to use native return types

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3579889

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

mstrelan created an issue. See original summary.

mstrelan’s picture

mstrelan commented
Title: [PP-1] Remove @return void descriptions » Remove @return void descriptions
Status: Postponed » Active

#3577137: Upgrade to Coder 9 is in, setting to active.

mstrelan opened merge request !17731

mstrelan’s picture

mstrelan commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented

Would it cause issue if we dropped the entire @return void line and actually add void the the functions?

mstrelan’s picture

mstrelan commented
Title: Remove @return void descriptions » Replace @return void with native void return types

Let's do it.

Most of these are protected methods or test base classes. The only interface change is NodeGrantDatabaseStorageInterface::alterQuery(), but nothing in contrib extends that. It would break tests in at least 14 contrib projects extending ResourceTestBase. I think a couple projects extending DiffFormatter too. I've added a CR for this.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Reviewed & tested by the community

Is BlockPluginCollection::addInstanceId() also considered a public API? I think we are safe to make this change.

The rest looks straight forward to me.

longwave’s picture

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

I am not sure we should do ResourceTestBase here.

Following #3486376: Extend Symfony DebugClassLoader to report missing cross-module @return types docblock return types should be reported to downstream users in 12.0. This means these users should already get notified of the ResourceTestBase changes here, along with every other missing return type as well. See the change record at https://www.drupal.org/node/3579056

Therefore I think the other ones that are unlikely to be extended are fine to do here, but anything that breaks downstream can wait until Drupal 13 where, in theory, we should be able to add return types to all methods.

smustgrave’s picture

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

Reverted ResourceTestBase.php

smustgrave’s picture

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

JK I messed things up.

mstrelan’s picture

mstrelan commented

Not sure it's worth doing without ResourceTestBase. Maybe we just revert back to original scope of removing the return descriptions for all of these?

smustgrave’s picture

smustgrave commented

Oh man I sincerely apologize I messed this MR up this morning and meant to fix and got distracted. Can fix when I get back home

smustgrave’s picture

smustgrave commented

Fixed my mistake.

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Needs work » Needs review

Updated proposed resolution and CR, pushed an updated MR.

smustgrave’s picture

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

Going to mark. Sorry again for the disruption. Got busy with work and other stuff. That's on me.