Problem/Motivation

This is a child issue of #3402007: Fix strict type errors in test modules. After fixing strict type issues we can add the strict_types declaration to all test modules.

Steps to reproduce

Add to phpcs.xml.dist:

  <rule ref="SlevomatCodingStandard.TypeHints.DeclareStrictTypes">
    <properties>
      <property name="spacesCountAroundEqualsSign" value="0" />
    </properties>
    <include-pattern>*/tests/modules/*</include-pattern>
    <include-pattern>*/tests/*_test/*</include-pattern>
  </rule>

Run phpcs:

composer phpcs

Proposed resolution

Run phpcbf:

composer phpcbf

Remaining tasks

Review individual commits of this MR to highlight the additional changes needed to make it pass.

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3400334

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 !5309

mstrelan’s picture

mstrelan commented
Issue summary: View changes
mstrelan’s picture

mstrelan commented
Issue summary: View changes
Parent issue: #3376057: [META] Add declare(strict_types=1) to all tests » #3402007: Fix strict type errors in test modules
mstrelan’s picture

mstrelan commented
Issue summary: View changes
mstrelan’s picture

mstrelan commented
Title: Add declare(strict_types=1) to all test modules » [PP-2] Add declare(strict_types=1) to all test modules
Issue summary: View changes

Also postponed on #3439800: [pp-1] Standardize location of test modules

mstrelan’s picture

mstrelan commented
Title: [PP-2] Add declare(strict_types=1) to all test modules » [PP-1] Add declare(strict_types=1) to all test modules

#3439800: [pp-1] Standardize location of test modules is likely a won't fix, so we can continue with just the modules that match the patterns in the issue summary.

mstrelan’s picture

mstrelan commented
Title: [PP-1] Add declare(strict_types=1) to all test modules » Add declare(strict_types=1) to all test modules
Status: Postponed » Needs work
mstrelan’s picture

mstrelan commented

We can just about get away with combining this with #3400434: Enforce strict types in tests. I've updated the include/exclude paths as below:

<include-pattern>*/tests/*</include-pattern>
<exclude-pattern>*/fixture/*</exclude-pattern>
<exclude-pattern>*/fixtures/*</exclude-pattern>

I noticed the following additional files that I guess are technically out of scope for this issue, there may be a few others. Maybe we manually exclude them and address them in #3400434: Enforce strict types in tests?

core/modules/comment/src/Tests/CommentTestTrait.php
core/modules/contact/tests/drupal-7.contact.database.php
core/modules/system/tests/http.php
core/modules/system/tests/https.php
core/tests/bootstrap.php

Let's see what the testbot thinks anyway, there could be new errors introduced recently.

mstrelan’s picture

mstrelan commented

Well #9 was a bit ambitious. There are a few other traits and such that are problematic. Reverted to original scope. Looks like there are about 3 more tests to fix.

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Needs work » Needs review
smustgrave’s picture

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

Large MR! I skimmed a few dozen files and does appear to just be added strict_type.

The deletions appear to be replacing few instances of space around the = sign.

Question for committers but seem it also added

/**
* @file
*/

To some test module files. Do we care if there is no description? Should a novice issue be opened to address those?

mstrelan’s picture

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

Thanks for pointing out those @file things. Phpcbf seems to get confused adding strict types alongside @file blocks. I'll fix those up manually.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Fixed all the @file blocks that were added when we already had them and rebased off HEAD.

smustgrave’s picture

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

Test failure appeared random

Latest updates still appear fine.

quietone’s picture

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

Unfortunately, this needs to be rebased.

I also noticed is that this is using the string declare(strict_types=1); but, according to coding standards,there should be a space on each side of the "=" side. But later in the standard, the example for declaring string types uses the string as shown before. That is a problem for coding standards to sort out.

I went through all the changes and did not find any problems with the changes.

mstrelan’s picture

mstrelan commented

Where does it say to use a space? There was an issue and standards committee process for this and we settled on no space.

mstrelan’s picture

mstrelan commented

#3303206: Define a standard for adding declare(strict_types=1)

mstrelan’s picture

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

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

Rebase seems good.

I actually remember in a few modules having the space and then randomly it went no space so had to fix a bunch myself.

quietone’s picture

quietone commented

Sorry, my fault the standard for this does say that the space is omitted.

The declare statement is written without spaces around the equals sign.

catch’s picture

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

This is still adding empty @file blocks which looks like a mistake? If it's not, the issue summary could use an update.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Sorry about missing that. This should have corrected all the @file problems.

  • catch committed 489c3286 on 11.x 
    Issue #3400334 by mstrelan, quietone, smustgrave: Add declare(...
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Fixed

Thanks looks good now. Getting this in while it still applies cleanly.

Committed/pushed to 11.x, thanks!

catch closed merge request !5309

Status: Fixed » Closed (fixed)

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