Problem/Motivation

Symfony's PHPUnit-bridge provides a couple of traits to polyfill lower-version-upwards methods that PHPUnit introduces in its latest versions, https://symfony.com/doc/current/components/phpunit_bridge.html#polyfills....

Proposed resolution

  • Adjust Drupal\TestTools\PhpUnitCompatibility\PhpUnit8\ClassWriter::mutateTestBase so that it rewrites the PHPUnit\Framework\Assert class as well, as we do for PHPUnit\Framework\TestCase, and for each of them inject the polyfill trait as part of the rewrite.
  • Replace calls of assertDirectoryNotExists with assertDirectoryDoesNotExist, and remove the deprecation silencer, to prove the model works properly.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3174200

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake’s picture

Huh, I'm afraid we'd have to rewrite the PHPUnit\Framework\Assert class as well, as we do for PHPUnit\Framework\TestCase in Drupal\TestTools\PhpUnitCompatibility\PhpUnit8\ClassWriter::mutateTestBase, in order to get this to work in PHPUnit8...

mondrake’s picture

Issue tags: +PHPUnit 9, +PHPUnit 10
longwave’s picture

How does Symfony deal with the Assert class BC issue?

mondrake’s picture

mondrake’s picture

mondrake’s picture

mondrake’s picture

mondrake’s picture

mondrake’s picture

Reuploading patch in #9 that is still good after commit of #3127141: Use PHPUnit 9 for PHP 7.4+, while keeping support for PHPUnit 8.4 in PHP 7.3.

This approach would allow changing how we manage tests' codebase in relation to PHPUnit versions: instead of trying to fit latest versions of PHPUnit by providing BC backfills, we would now be able to convert methods deprecated in latest PHPUnit even before we adopt them, and the polyfills help us supporting also PHPUnit versions where the new methods are not yet available.

For this reason, if we move forward here I think we should backport this to 8.9 so that contrib too can leverage the polyfills for their branches that are currently supporting both D8 and D9.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

mondrake’s picture

Reroll and changed to MR workflow.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

rerolled; no longer sure this needs a backport with 6 months to go before D8 end of support.

longwave’s picture

Status: Needs review » Needs work

Couple of comments on the MR, but otherwise I think this is a good idea - letting us get rid of PHPUnit deprecations early.

I did think we could merge the two methods into one common mutator and pass it configuration in the form of filenames and sets of regexps, but it seems overkill for two files - if we start mutating more than two maybe we should try that.

I notice the Symfony trait is marked @internal, but the TestCase one is too, and as this is in test code we can react fairly easily if Symfony changes something under us, so I think that's OK.

mondrake’s picture

Status: Needs work » Needs review

OK, still maybe we can do a little in that sense.

longwave’s picture

Status: Needs review » Needs work

One little comment nit but otherwise this looks ready to go.

mondrake’s picture

Status: Needs work » Needs review

Thanks!

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

This looks really smart. It's nice to be able to fix PHPUnit 10 deprecations in our code and will help people move to PHPUnit 9 too.

I've pondered whether we need a change record for this and I think it might be helpful for module developers to be informed about this and reminded that keeping tests up-to-date with the latest version of PHPUnit supported by core means they'll have less surprises.

mondrake’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record
alexpott’s picture

Title: Use PHPUnit-bridge polyfills for FC layers » Use PHPUnit-bridge polyfills for forward compatibility layer
Status: Reviewed & tested by the community » Fixed

Committed 5be5048 and pushed to 9.3.x. Thanks!

  • alexpott committed 5be5048 on 9.3.x
    Issue #3174200 by mondrake, longwave: Use PHPUnit-bridge polyfills for...
alexpott’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Fixed » Patch (to be ported)

Discussed with @catch and @xjm, who agreed that we could consider this for 9.2 because:

  • The change is minimal
  • Affects only tests
  • It is likely that Drupal 10 will only support PHPUnit ^10 - this would make it easier for contrib to prep for D10 once D9.3 is out - because only D9.3 and D9.2 will be supported at that point and both releases would have this in.

Can someone create an MR or patch against 9.2.x so we can get a test run? Thanks.

mondrake’s picture

Status: Patch (to be ported) » Needs review
FileSize
13.59 KB

Here's a patch, the MR diff applied smoothly.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Patch is identical, so RTBC if tests pass.

mondrake’s picture

  • catch committed 36a4303 on 9.2.x
    Issue #3174200 by mondrake, longwave, alexpott: Use PHPUnit-bridge...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +9.2.0 release notes

Committed/pushed to 9.2.x, thanks!

Status: Fixed » Closed (fixed)

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