Problem/Motivation

expects($this->any()) is deprecated in PHPUnit 12. any() is equivalent to having no expectation at all. Other issues are dealing with removing existing uses, see #3561671: [meta] Refactor tests to use stubs instead of mocks where mocks do not configure expectations. I want to prevent the new uses that are creeping into the code base by making PHPStan throw errors for them. This will have a few benefits:

  • Reviewers and committers won't have to do manual checks for these uses.
  • The removal issues won't have to chase new commits anymore.
  • It will force people to write better tests in advance of upgrading to PHPUnit 12. Use of any() has been shown to be associated with bad/invalid tests where someone thought that a thing was being tested, but it wasn't in reality.

Proposed resolution

  • Add a Composer dev dependency on spaze/phpstan-disallowed-calls and add a custom rule to forbid TestCase::any().
  • Add existing uses of any() to the baseline. We could do find & replace removals of any() instead, but some of these can and should be converted to more specific expectations. I believe adding to the baseline is preferable.

Dependency Evaluation

The following is the dependency evaluation for spaze/phpstan-disallowed-calls.

Maintainership of the package

The repository appears to be maintained solely by the owner, spaze (Michal Špaček). All commits made over the last year were made by spaze.

Security policies of the package

There is no security policy.

Expected release and support cycles

The maintainer appears to make semi-frequent releases on an as-needed basis. There have been seven releases over the past year with an additional two releases made within a year + one week. As of the time of this writing, the most recent release was made six days ago in response to the release of PHPUnit 13.

Code quality

  • Includes PSR-4 autoloading
  • Declares strict types throughout its classes
  • Includes automated PHPUnit tests via GitHub Actions
  • Includes PHPStan linting on the maximum level
  • Includes numerous documentation files

Other dependencies it would add

None - the package requires phpstan/phpstan, which is already a dev dependency. Aside from that, it only has dev dependencies.

Remaining tasks

New uses of any() are creeping into the code with every batch of commits. Rebuild the baseline before committing, lest you break tests on HEAD.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3573259

Command icon 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:

About issue forks

Comments

dcam created an issue. See original summary.

dcam opened merge request !14749

dcam’s picture

dcam commented
Status: Active » Needs review

It should be noted that the custom extension won't be necessary when we upgrade PHPUnit to version >12.5.0. At that point the deprecated any() function will be picked up by PHPStan like any other deprecated function. But that doesn't help us now.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

@dcam pinged me about this in slack. Definitely a +1 to adding it to help with the cleanup to prevent more from being added to the pile. Also thanks for adding that comment so we know when this can be removed!

Think it would be good to get this in soon so all the open conversion tickets can be updated too.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

+1 for making it more readable by inverting the condition.

There is also https://github.com/spaze/phpstan-disallowed-calls which is somewhat overkill for one method, but might be worth swapping to if we want to disallow more types of things in the future.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3390368: Adopt ekino/phpstan-banned-code and use it to prevent further usage of uniqid(), md5(), sha1()

#6 +1 on adopting a generic tool - there are other cases we could think of starting banning in core, eg #3390368: Adopt ekino/phpstan-banned-code and use it to prevent further usage of uniqid(), md5(), sha1()

dcam opened merge request !14759

dcam changed the visibility of the branch 3573259-prevent-new-expects-any to hidden.

dcam’s picture

dcam commented
Issue summary: View changes
Status: Needs work » Needs review

I made a new MR for requiring spaze/phpstan-disallowed-calls.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Thanks. Any new dependency needs a dependency evaluation. As this is only a dev dependency, posting some brief answers in a comment here should be enough presuming the package meets our criteria.

dcam’s picture

dcam commented
Issue summary: View changes
Status: Needs work » Needs review

I added the evaluation to the issue summary.

dcam’s picture

dcam commented
Issue tags: +Needs release manager review, +Needs framework manager review

Tagging for release and framework manager review, per the evaluation guidelines.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

dcam’s picture

dcam commented

I tried adding an additional custom rule to forbid the use of PHPUnit\Framework\MockObject\Builder\InvocationStubber::with() due to the deprecation in PHPUnit 12.5.11. For some reason it didn't work out. There were no additional errors found when I added the rule. If anyone has ideas let me know.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: -Needs release manager review

Happy with the additional dependency and the evaluation; personally I'm not bothered about a lack of security policy as this is a dev only extension to PHPStan - we can always drop it if there is a problem and I really don't see how there would be a security problem in this package anyway. Removing the RM tag.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs work

Needs a reroll

mondrake’s picture

mondrake
Location 🇮🇹
commented

Filed #3574204: Disallow usage of uniqid(), md5(), sha1(), crc32() and hash() with weak algorithms for follow up

dcam’s picture

dcam commented
Status: Needs work » Needs review

Rebased. There were conflicts in composer.lock and the baseline.

For the record, there was at least one new use of any() that had to be added to the baseline along with the rebase. This change might have prevented that from happening.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community
dcam’s picture

dcam commented
Issue summary: View changes
dcam’s picture

dcam commented

Rebased. There was another conflict in the baseline.

nicxvan’s picture

nicxvan commented

Don't we need a security policy for dependencies?

dcam’s picture

dcam commented

Rebased due to changes in the baseline.

There are four new uses of any() added to the MR's baseline today that would have been caught by this new rule.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs framework manager review

This makes sense to me, we can drop the dependency again when we're actually on PHPUnit 12. Needs a rebase though.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +Avoid commit conflicts

Rebased. Restoring RTBC.

Since this has consensus and many changes to the baseline can cause problems I'm going to add the "avoid commit conflicts" tag.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main, thanks!

edit: posted that too soon, the phpstan baseline was outdated again so commit checks failed. But rebuilt it locally prior to actual successful commit.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !14759

  • catch committed 20936de0 on main 
    task: #3573259 Prevent new expects($this->any()) in tests

By: dcam
By:...

Status: Fixed » Closed (fixed)

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