Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Remove from testing framework (base test suite classes, traits, listeners, etc) the code that was deprecated in the D9 cycle for removal in D10.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3261266
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:
- 3261266-remove-deprecated-code changes, plain diff MR !1740
Comments
Comment #2
mondrakeOn this
Comment #4
longwaveWould appreciate a review of #3254726: Remove SimpleTest support from run-tests.sh, TestDiscovery and TestRunnerKernel if you have time.
Comment #5
mondrakeComment #6
mondrakeComment #7
longwaveOne question about strictness following this patch.
Also should we remove AssertButtonsTrait here? Not sure where else we would do it:
Otherwise I think this covers all test-related code.
Comment #8
mondrakeThank you! Removed AssertButtonsTrait.
Comment #9
longwaveOK, I can see the argument for keeping them to let contrib know, it only affects tests and will make them cleaner in the long run. Maybe we should flag these asserts for removal in 11.0.x and/or have a followup issue somewhere to do it?
Marking RTBC for core committer opinion.
Comment #10
catchWe already trigger deprecation errors where the extra argument is passed to those methods, so I think we could probably go ahead and remove the overrides altogether.
Comment #11
catchComment #12
mondrakererolled
Comment #13
daffie CreditAttribution: daffie commentedJust 1 remark.
Comment #14
mondrakeSo, #10 suggests to remove the overrides, and #13 suggests to keep them but change from assert() to \InvalidArgumentException.
Now, in PHPStan level-1 we will have a static analysis check to ensure that a function is not called with more arguments than it can handle, which is IMHO the best solution.
This is what e.g. you get running PHPStan with level 1:
because in that test we have this code in the UserCreationTest
That is we have the $group argument which is cruft from Simpletest.
But, we are still quite far from level-1.
IMHO we better leave some checks to ensure we do not add extra arguments, so that when we move to level-1 we do not find more assertions to change.
So, I still stand behind the patch as is now.
Comment #15
daffie CreditAttribution: daffie commented@mondrake: Ok, lets go with the assertions.
All changes look good to me.
Comment #16
catchHmm. So personally I would not have added the assertions, and would have removed the test coverage for the extra arguments, given that phpstan with (eventually) cover this. However it is not doing any harm, and is a step forward overall, so committed/pushed to 10.0.x, thanks!
Comment #18
mondrakeSomehow 'Fixed' did not stick in #16.