Problem/Motivation

After we drop support for PHPUnit 4.8, we can remove any BC layer for PHPUnit 4 from PhpunitCompatibilityTrait as well.

Proposed resolution

Do it.

Remaining tasks

Nope.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

Release notes snippet

Nope.

CommentFileSizeAuthor
#11 3053417-11.patch9.34 KBLendude
#9 3053417-9.patch8.22 KBLendude
#9 interdiff-3053417-7-9.txt2.95 KBLendude
#7 3053417-7.patch5.1 KBamateescu
#2 3053417-combined.patch52.11 KBamateescu
#2 3053417.patch10.53 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
3053417.patch10.53 KB
3053417-combined.patch52.11 KB

Here's a patch for this. The combined one includes #3008870: Drop support for PHPUnit 4.8 once PHP 5 is no longer supported (8.8.x).

Status: Needs review » Needs work

The last submitted patch, 2: 3053417-combined.patch, failed testing. View results

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Are we sure we want to do this as a full removal? We're likely going to need the ability to bridge two PHPUnit versions again in the future because of their very aggressive release cycle vs. our LTSes.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Related issues: +#2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4
Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented

Removing this BC layer will break existing tests, see #2. We can however remove \Drupal\Tests\PhpunitCompatibilityTrait::createMock since the forward compatibility part is no longer needed.

Removing the BC part sounds like D9 territory to me.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Title: Remove PhpunitCompatibilityTrait » Clean up PhpunitCompatibilityTrait
Status: Needs work » Needs review
FileSize
3053417-7.patch5.1 KB

Agreed with #6. We can also remove some BC code that's not needed anymore from the other methods of the trait.

Status: Needs review » Needs work

The last submitted patch, 7: 3053417-7.patch, failed testing. View results

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented
Status: Needs work » Needs review
FileSize
interdiff-3053417-7-9.txt2.95 KB
3053417-9.patch8.22 KB

We can simplify the test (a lot). Something like this maybe?

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Needs work

Will certainly need a reroll.

+1 for #6, createMock is a fully PHPUnit only method now, no purpose to keep it nor test it.

+++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTraitTest.php
@@ -11,108 +11,21 @@
+   * Tests that getMock is available..

Duplicated dot at the end.

Other than that, it'd be RTBC from my POV.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented
Status: Needs work » Needs review
FileSize
3053417-11.patch9.34 KB

@mondrake thanks for looking at this!

Rerolled, removed the dot, simplified a little more. No interdiff because of reroll.

mondrake’s picture

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

Thanks! RTBC.

BTW do we want to tackle setExpectedException at any point? It's a BC shim too. Can be a separate issue anyway, and one may argue it's a pretty useful method to keep regardless of PHPUnit dropping it.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Issue summary: View changes

Updated IS, added this issue to the PHPUnit4 drop support CR.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed d1ffb5b and pushed to 8.8.x. Thanks!

Definitely seems worth it to file a follow-up to deprecate \Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException() and replace that with PHUnit 6 code.

  • alexpott committed d1ffb5b on 8.8.x 
    Issue #3053417 by amateescu, Lendude, mondrake, xjm: Clean up...
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Filed #3059090: Deprecate \Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException().

Status: Fixed » Closed (fixed)

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