Problem/Motivation

See #3455549: Add return typehints to protected test helper methods

Looks like some bool returns were missed in #3456611: Add bool return typehints to protected test helper methods.

The steps to reproduce below should help to find more.

Steps to reproduce

Add this to phpcs.xml.dist

<rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHint">
  <include-pattern>*/tests/*</include-pattern>
</rule>

Run composer phpcbf

Grep the git diff output for changes that add : bool to protected or private methods:

git diff 11.x | grep -E "\+  (protected|private) function .*)\: bool"

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3470203

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 opened merge request !9324

mstrelan’s picture

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

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

Seems straight forward.

I can't wait till typehints are required.

quietone’s picture

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

Unfortunately, a few more instances have crept into the code. It seems we will be chasing our tail for a while on these. Let's get an update on this and then ping me when it is ready.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Added a few more:

  • \Drupal\Tests\comment\Functional\CommentTestBase::commentExists
  • \Drupal\Tests\comment\Functional\CommentTestBase::commentContactInfoAvailable
  • \Drupal\KernelTests\AssertContentTrait::assertNoFieldByName

I intentionally left out the following:

  • \Drupal\Tests\user\Functional\UserRegistrationRestTest::registerUser because it would add a union type
  • \Drupal\KernelTests\AssertContentTrait::getSelectedItem because the @return annotation is wrong
quietone’s picture

quietone commented
Related issues: +#3129006: Modernize Drupal\KernelTests\AssertContentTrait

Adding where work on AssertContentTrait is being done.

smustgrave’s picture

smustgrave commented

I intentionally left out the following:

\Drupal\Tests\user\Functional\UserRegistrationRestTest::registerUser because it would add a union type
\Drupal\KernelTests\AssertContentTrait::getSelectedItem because the @return annotation is wrong

Should follow ups be made for those?

mstrelan’s picture

mstrelan commented

Should follow ups be made for those?

I've updated the parent / meta to mention union types in the remaining tasks. I think that also covers AssertContentTrait::getSelectedItem as I think it should return string|false.

smustgrave’s picture

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

Sounds like a plan

  • quietone committed acd24cb3 on 11.x 
    Issue #3470203 by mstrelan, smustgrave: Add (more) bool return typehints...
quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Fixed

Committed acd24cb and pushed to 11.x.

Thanks!

quietone closed merge request !9324

Status: Fixed » Closed (fixed)

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