Add declare(strict_types=1) to all test traits

Hrm, one of the JS tests failed in the MR pipeline. Could be random fail, but no time to dig deeply now. NW to either re-run and verify it was random, or to fix the problem if there is one. 😅

Sweet, yup. Pipeline is now green. Changes are clean. Title, summary and scope are proper. RTBC.

Thanks!
-Derek

Do we have a mechanism (e.g. a phpcs check or the like) to ensure newly added test traits also get strict typing, so that this doesn't slip? I think we need that too. Thanks!

There is

rule ref="vendor/slevomat/coding-standard/SlevomatCodingStandard/Sniffs/TypeHints/DeclareStrictTypesSniff.php"

@mstrelan Ah excellent. I think it'd be good to include them on a per-test-type basis if it's easy, so that we don't have to do a second round of cleanup after the big MR (which might take awhile). If it's not easy then we can wait until they're all complete and deal with the fact that we might have to add it to (and fix) additional tests added to core between now and then.

Rule seems to have been added and didn't cause any issues.

Thanks @mstrelan; that looks great.

I did notice one thing regarding the coding standard of the declaration itself. I also belatedly notice that this is a point in the remaining tasks section as well. Per our coding standards for comparison and assignment operators, the space is required on either side of the strict type declaration equals sign. If and when that's properly enforced (or properly enforced within a function call), we might end up with a situation where the two CS rules conflict with each other. So, I think we should use the format with the spaces.

For updating to with spaces per #13,

This seems to be the main one, or first one that will need to land.

Re: #13 + #15: While technically the declare is not an assignment, it sure looks like one to the vast majority of people reading the code. If only to be more self-consistent, I think @xjm is right and we should change it to the one with spaces.

However, not sure how to handle the scope of that with the existing inconsistencies. Can that be a final step all the way "up the chain" at #3050720: [Meta] Implement strict typing in existing code (which probably needs to be turned into a formal plan)? I'm attaching the lists of the existing files in 11.x core with and without the space. It's a spread of lib, modules and tests for both. Can we just fix all ~100 without a space in a single issue, and then as we turn on our PHPCS rule about this, we require the version with the space from now on?

Meanwhile, yay! 🎉 https://git.drupalcode.org/issue/drupal-3401994/-/pipelines/51223

composer phpcs -- --report-full --report-summary --report-\\Micheh\\PhpCodeSniffer\\Report\\Gitlab=phpcs-quality-report.json
> phpcs --standard=core/phpcs.xml.dist --parallel=$(nproc) -- '--report-full' '--report-summary' '--report-\Micheh\PhpCodeSniffer\Report\Gitlab=phpcs-quality-report.json'
FILE: ...ests/Drupal/Tests/Core/Database/SchemaIntrospectionTestTrait.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] Missing declare(strict_types=1).
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
Time: 9.74 secs; Memory: 6MB
PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
...ts/Core/Database/SchemaIntrospectionTestTrait.php  1       0
----------------------------------------------------------------------
A TOTAL OF 1 ERROR AND 0 WARNINGS WERE FOUND IN 1 FILE
----------------------------------------------------------------------
PHPCBF CAN FIX 1 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

I'll rebase to 11.x and remove the revert commit to get us back to normal.

Thanks,
-Derek

I updated the phpcs.xml.dist file to enforce spaces, and re-ran phpcbf on all the affected files. Had to change one of the existing call sites in core since it's in a test trait and is now covered by the rule active in this MR. We can handle the rest in a follow-up. ;)

Per @larowlan at #3376057-29: [META] Add declare(strict_types=1) to all tests, this should probably be blocked on #3303206: Define a standard for adding declare(strict_types=1) before we try to decide this on our own in here.

Sorry for the almost duplicate comment, but want to make sure people working on both issues see this.

I'm just calling out that this issue and the seemingly related https://www.drupal.org/project/drupal/issues/3400018 are making opposite decisions on spaces and enforcement.

This trait issue has this rule:
And put all declarations like this: declare(strict_types = 1);

But the JS issue has this rule:
And put all declarations like this: declare(strict_types=1);

There was some discussion on the trait issue around the spaces so I think that is the correct format https://www.drupal.org/project/drupal/issues/3401994#comment-15322179

Nice work!

@mestrelan fair enough! I did not have a strong opinion either way, I just noticed two nearly identical tickets approaching it in opposite directions and wanted to make sure it was flagged. I'll add the details you mentioned to the other ticket too since that one is also adding spaces.

Postponing on the official acceptance of the coding standards issue so I don't get in trouble. 😇

It really can be unpostponed now because the committers have adopted the rule, and the remaining steps are to implement a rule and then enable for the codebase. Well congrats, we're already doing that for these issues. So back to RTBC.

Merge conflict just due to me having committed the other:

++<<<<<<< HEAD
 +    <!-- @todo Broaden this in https://www.drupal.org/project/drupal/issues/3400434 -->
 +    <!-- <include-pattern>*/tests/*</include-pattern> -->
 +    <include-pattern>*/tests/src/Traits/*</include-pattern>
 +    <include-pattern>./tests/Drupal/Tests/*Trait.php</include-pattern>
 +    <include-pattern>./tests/Drupal/Tests/Traits/*</include-pattern>
 +    <exclude-pattern>./tests/Drupal/Tests/Composer/*</exclude-pattern>
 +    <exclude-pattern>./tests/Drupal/Tests/Listeners/*</exclude-pattern>
++=======
+     <include-pattern>./tests/Drupal/BuildTests/*</include-pattern>
++>>>>>>> origin/11.x

I fixed that by putting the Build Test line first (for alphabetical order). Pushed the fix.

Saving credits.

Looks like we need a new trait marked:

FILE: ...core/modules/field/tests/src/Traits/EntityReferenceTestTrait.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] Missing declare(strict_types=1).
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
Time: 10.7 secs; Memory: 6MB
PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
...eld/tests/src/Traits/EntityReferenceTestTrait.php  1       0
----------------------------------------------------------------------
A TOTAL OF 1 ERROR AND 0 WARNINGS WERE FOUND IN 1 FILE
----------------------------------------------------------------------
PHPCBF CAN FIX 1 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Looks great, all traits are marked with the declare statement, back to rtbc.

Committed to 11.x and cherry-picked to 10.2.x as an RC-safe test improvement. Thanks!