Fix strict type errors in unit tests

So MR 5216 appears to have test failures.

JS test was a random fail, Unit test was due to a line number mismatch since this MR is not including the declaration. Should be good now.

Yup not seeing any failures now.

!5216 is only fixing the strict type bugs in the tests. That much looks good to my eyes. A few trivial nits, but only personal preference and not worth holding up anything over. However, that's point 1 of proposed resolution. Is the idea to commit that first, then re-visit !5196 as part of this same issue? Or should we split these into separate issues for better scope management?

Thanks!
-Derek

I don't mind either way. Separate commits would make for better git archaeology and therefore a separate issue probably makes sense. There are some erroneous line break removals are running rector which seems to occur when there is a file doc block in a test. Maybe we should remove those doc blocks in another issue too, since they are not needed, then run rector again.

I've reduced the scope of this issue to simply provide the fixes. I've opened 📌 Remove @file annotation from test classes Needs review which would allow the rector to run cleanly and 📌 Add declare(strict_types=1) to all unit tests Active to run the rector.

mstrelan → closed merge request !5196

Great, thanks!

In that case, !5216 is hereby RTBC. The changes are well-scoped and correct, this issue summary and title are accurate, the tests are still passing.

Thanks,
-Derek

p.s. Crediting mstrelan for working on this, and Sam and myself for reviews.

Updated IS with simpler instructions to run rector and avoid manually reverting certain files.

This all looks correct to me. Thanks for the scope fix.

Unfortunately, it no longer applies as a diff nor rebases cleanly. Presumably there's a merge conflict. Thanks!

Oh I actually would not credit @smustgrave here based on comments so far since the two comments are specifically raised as "will not be credited" types in the core issue credit policy. (Also his name is Stephen, not Sam.) 😀 I am trying to encourage some more detailed review comments generally so that the credits reflect all his work.

Merge conflict resolved

I confirmed the diffstat is the same; restoring RTBC. In the future though it'd be good to document the specific merge request on-issue. 🙂

Committed to 11.x and 10.2.x. I tried to cherry-pick it to 10.1.x, but it did not cherry-pick cleanly. Presumably the pre-merge version of the MR would be what we want; however, I was not able to figure out how to get GitLab to give me a plain diff version of the difference between version 3 and HEAD. (https://git.drupalcode.org/project/drupal/-/compare/11.x...d8d0a9eb27417... or https://git.drupalcode.org/project/drupal/-/merge_requests/5216/diffs?di... but as plain diff was the goal.)

So, we could make a patch or 10.1.x MR from the previous commit to backport this to 10.1.x so that the tests are similarly robust there.

Thanks!

Guess I should've commented on the issue, I did the initial review when this first issue was posted but wasn't given credit.

Ah yes, thanks @acbramley. Fixing.

Thank you @xjm! Need to remember in the future to comment on issues if I do an MR only review :)

Added an MR for 10.1.x

The conflict was caused by 📌 Remove "Please" from the codebase Fixed .

mstrelan → closed merge request !5216

MR for 10.1 seems good.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Committed the backport to 10.1.x. Thanks!

xjm → closed merge request !5472

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