Problem/Motivation

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

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 : int to protected or private methods

Proposed resolution

Commit the changes identified in the steps to reproduce

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3458426

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.

abarrio made their first commit to this issue’s fork.

abarrio’s picture

abarrio
Primary language Spanish
Location Seville
commented

I made a part of the development.

mstrelan’s picture

mstrelan commented
Status: Active » Needs work

@abarrio thanks for working on this. The scope should be limited to tests only, so in paths like core/tests/* and core/*/*/tests. We also don't want to add union types in this issue, to keep the scope down, so should be limited to int returns only.

mstrelan opened merge request !9323

mstrelan’s picture

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

catch made their first commit to this issue’s fork.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Not sure why cspell is showing up but pipeline failed.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

cspell was due to catch and I debugging some pipeline cache issues, let's move that to #3470641: gitlab artifact caching doesn't work due to differences with project ID and build directories. Have reverted and rebased.

smustgrave’s picture

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

Nice, in that case change seems fine.

quietone’s picture

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

I applied the diff and tested.

My results show two methods that have not been changed

+  protected function getUnapprovedComment($subject): int {
+  protected function doEditStep($active_langcode, $default_revision, $untranslatable_update = FALSE, $valid = TRUE): int {
mstrelan’s picture

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

Thanks @quietone, I should have indicated why these were skipped. Both of these claim to return an int but actually return a string.

The two methods are:

\Drupal\Tests\comment\Functional\CommentTestBase::getUnapprovedComment
\Drupal\KernelTests\Core\Entity\EntityDecoupledTranslationRevisionsTest

I've added a note to fix these in the parent issue. Not opening individual issues for now so we can figure out appropriate scope for these little fixes.

Setting back to RTBC as there is nothing to change here.

  • quietone committed d433d966 on 11.x 
    Issue #3458426 by mstrelan, catch, smustgrave: Add int return typehints...
quietone’s picture

quietone commented

@mstrelan, thanks for the explanation.

I reviewed the MR and I think it is ready for commit. There is JavaScript here, which I no little of. So I confirmed that transfersize returns an int. So, this is good to go.

quietone’s picture

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

Committed d433d96 and pushed to 11.x. Thanks!

quietone’s picture

quietone commented

I tried to find the commits made by @abarrio but wasn't able to. So, I DMed mstrelan who confirmed the contribution was valid, it just needed to be de-scoped. Therefor, adding credit.

  • quietone committed 2a549f16 on 11.x 
    Revert "Issue #3458426 by mstrelan, catch, smustgrave: Add int return...
quietone’s picture

quietone commented
Status: Fixed » Needs work

This is causing test failures on HEAD, from \Drupal\migrate_drupal\Tests\StubTestTrait::createEntityStub. Although, I don't understand why tests passed in the lastest pipeline here though.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

I investigated the fail mentioned in #18 and it was due to #3400434: Enforce strict types in tests. This uncovered that sometimes ::createEntityStub returns an int and sometimes a string. I've added this one to the list of return types to be fixed in the parent issue. I've removed the return type from this method in the MR, setting back to NR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Appears to have test failures.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Yes, but those are in tests that are not changed in the MR.

smustgrave’s picture

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

Rebase seems good and all green!

quietone’s picture

quietone commented
Assigned: Unassigned » quietone

Thanks!

Assigning to myself to commit within 24 hours.

  • quietone committed 2319f339 on 11.x 
    Issue #3458426 by mstrelan, catch, abarrio, quietone, smustgrave: Add...
quietone’s picture

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

Thanks! Let's try this again.

Committed 2319f33 and pushed to 11.x. Thanks!

quietone closed merge request !9323

Status: Fixed » Closed (fixed)

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