Part of #3404246: [META] Fix strict type errors detected by phpstan
Add checkFunctionArgumentTypes: true to parameters section of phpstan.neon.dist
$ ./vendor/bin/phpstan analyse -c core/phpstan.neon.dist --error-format=raw | grep -F "Interface, PHPUnit\Framework\MockObject\MockObject given"
There are 17 violations (attached)
The return type of TestCase::createMock is typehinted as MockObject&RealInstanceType, where RealInstanceType is the class passed to the method.
We should use the same format for properties that are set via this method, for example:
@var \PHPUnit\Framework\MockObject\MockObject&\Drupal\Core\Lock\LockBackendInterface
Note, there are some adjacent properties that are union types instead of intersection types. We need the intersection so we can call methods from MockObject (like expects) while also calling methods from the Interface.
There is also one case where we are passing the MockBuilder instead of the MockObject that should be fixed too.
| Comment | File | Size | Author |
|---|---|---|---|
| mock-object.txt | 4.16 KB | mstrelan |
Show commandsStart 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, plain diff MR !13853
Comments
Comment #3
mstrelan commentedComment #4
mondrakeCommented inline.
Comment #5
smustgrave commentedSince you're fixing them shouldn't there be an update to the phpstan file so they don't come back?
Comment #6
mstrelan commented@smustgrave ideally yes, there is a parameter listed in the parent meta, but since it's split in to many child issues we need to fix them all before updating the config.
Comment #7
dcam commentedI ran PHPStan as shown in the issue summary on my local and got the same 17 errors. After applying the MR all 17 errors were gone. I read over all the changes and didn't see anything weird. For the record,
core/modules/image/tests/src/Unit/ImageStyleTest.php, which is at the top of the MR changes list, is the file where we're fixing theMockBuildermentioned in the IS.It looks like we're also standardizing the docblocks and type hints of all properties in the classes being edited. I think that's a good thing to help prevent something like this from happening again.
With all that said, this looks good to me.
Comment #8
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 necessarily 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 #9
mondrakeJust some thoughts here:
1) Given #3561671: [meta] Refactor tests to use stubs instead of mocks where mocks do not configure expectations, I think it should be checked here whether we are really needing a mock or we can use a stub instead - that will change the typehint from
FooInterface&MockObjecttoFooInterface&Stub. We should directionally use stubs when we are not setting expectations on the number of times a method is called.2) Just my personal preference here, so IIWII: I think the typehint is more readable when the original class of the mock comes first. In other words, prefer
FooInterface&MockObjectoverMockObject&FooInterface.Comment #10
mstrelan commentedRebased and address #9.2. Not sure about #9.1, it's not really what the scope of this issue is about, and there are plenty of other things that could be fixed or improved at the same time, but I'm trying to keep the scope down. There should not be BC concerns about changing these from MockObject to Stub later on.
Comment #11
mondrakeLGTM
Comment #13
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily 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 #14
dcam commentedPost-bot-rebellion rebase
Comment #15
longwaveBackported to 11.x to keep tests in sync, not worth going further back.
Committed and pushed c325b55a02c to main and 5fc1b187915 to 11.x. Thanks!