Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
This is a child issue of #3376057: [META] Add declare(strict_types=1) to all tests. After adding enabling strict types to all tests there were around 3000 errors. Fixing them all in one issue will lead to an enormous merge request that's difficult to review, as per the issue scope guidelines.
Steps to reproduce
See #3399373: Add declare(strict_types=1) to all Unit tests
Run the test suite:
./vendor/bin/phpunit -c core/phpunit.xml.dist --bootstrap=core/tests/bootstrap.php --testsuite=unit
Proposed resolution
- Fix all strict type issues in unit tests in this issue
- Follow up #3399373: Add declare(strict_types=1) to all Unit tests
Remaining tasks
Review updated MR
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #34 | 3397890-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Issue fork drupal-3397890
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:
changes
Comments
Comment #3
mstrelan commentedComment #4
mstrelan commentedComment #5
mstrelan commentedComment #6
mstrelan commentedComment #8
smustgrave commentedSo MR 5216 appears to have test failures.
Comment #9
mstrelan commentedJS 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.
Comment #10
smustgrave commentedYup not seeing any failures now.
Comment #11
dww!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
Comment #12
mstrelan commentedI 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 after 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.
Comment #13
mstrelan commentedI've reduced the scope of this issue to simply provide the fixes. I've opened #3399370: Remove @file annotation from test classes which would allow the rector to run cleanly and #3399373: Add declare(strict_types=1) to all Unit tests to run the rector.
Comment #15
mstrelan commentedComment #16
dwwGreat, 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.
Comment #17
mstrelan commentedUpdated IS with simpler instructions to run rector and avoid manually reverting certain files.
Comment #18
mstrelan commentedComment #19
mstrelan commentedComment #20
xjmThis 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!
Comment #21
xjmOh 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.
Comment #22
mstrelan commentedMerge conflict resolved
Comment #23
xjmI confirmed the diffstat is the same; restoring RTBC. In the future though it'd be good to document the specific merge request on-issue. 🙂
Comment #26
xjmCommitted 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!
Comment #27
acbramley commentedGuess I should've commented on the issue, I did the initial review when this first issue was posted but wasn't given credit.
Comment #28
xjmAh yes, thanks @acbramley. Fixing.
Comment #29
acbramley commentedThank you @xjm! Need to remember in the future to comment on issues if I do an MR only review :)
Comment #31
mstrelan commentedAdded an MR for 10.1.x
The conflict was caused by #2921133: Remove "Please" from the codebase.
Comment #33
smustgrave commentedMR for 10.1 seems good.
Comment #34
needs-review-queue-bot commentedThe 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.
Comment #35
mstrelan commentedComment #37
xjmCommitted the backport to 10.1.x. Thanks!
Comment #40
mstrelan commented