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. 😅

Looks like it's 🐛 [random test fail] TimestampFormatterWithTimeDiffViewsTest::testTimestampFormatterWithTimeDiff Active

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

@xjm it's implemented in 📌 Enforce strict types in tests Fixed but with a much broader glob pattern. So do we want to adjust that pattern for each of these issues as they are committed, or do it once at the end? Happy either way.

@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.

I've added the rule for this issue and fixed two more files. I won't add to other issues until this one is committed, for a simpler merge.

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. 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.

FWIW core already has 101 instances without the space and 95 with the space. It's worth noting that declare is a construct, not a function call, and strict_types=1 is a directive, not an assignment, so the same rules don't necessarily apply. That said, I'm not fussed either way, just went with the one that rector picked, but probably should have gone with the Slevomat default. Do we need to clean up the existing instances in core too? I guess that can be a follow up.

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 🌱 [Meta] Implement strict typing in existing code Active (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.

Simplified the steps to not require rector

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 📌 Add declare(strict_types=1) to all Functional JavaScript tests Fixed 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);

@nicxvan the decision is to be made in #3303206: Define a standard for adding declare(strict_types=1) → . This was discussed in #3402696: Coding Standards Meeting Tuesday 2023-11-21 2100 UTC → and it looks no spaces is the way, so I will revert to that. I don't think we need to postpone this any longer, but correct me if I'm wrong.

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.

xjm → changed the visibility of the branch 11.x to hidden.

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

I don't remember why ./tests/Drupal/Tests/Composer/* was excluded and this is passing so I've removed that exclusion. This should be good to go.

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!

mstrelan → closed merge request !5422

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