Problem/Motivation

#3126333: Replace usage of the optional $canonicalize parameter of assertEquals(), that is deprecated removes usages of this function from core in D9. It might be useful for contrib projects trying to maintain both 8.x and 9.x compatibility to have the method available in 8.9.x, or even 8.8.x.

This is a D8 only issue.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

mondrake’s picture

Status: Active » Needs review
FileSize
3.21 KB

Here's a concept. If this works, could also be extended to other FC needs (thinking of assertString(Not)ContainsString, assertIsString, etc)

mondrake’s picture

Title: Add forwards-compatibility shim for assertEqualsCanonicalizing() in phpunit » [D8 only] Add forwards-compatibility shim for assertEqualsCanonicalizing() in phpunit
Issue summary: View changes
mondrake’s picture

Title: [D8 only] Add forwards-compatibility shim for assertEqualsCanonicalizing() in phpunit » [D8 only] Add forwards-compatibility shim for assertEqualsCanonicalizing() in phpunit 6&7
longwave’s picture

+++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit7/TestCompatibilityTrait.php
@@ -39,4 +39,30 @@ public static function assertFalse($actual, string $message = ''): void {
+    $parent = new \ReflectionClass(parent::class);
+    if ($parent->hasMethod('assertEqualsCanonicalizing')) {
+      parent::assertEqualsCanonicalizing($expected, $actual, $message);
+    }
+    else {
+      static::assertEquals($expected, $actual, $message, 0.0, 10, TRUE);
+    }
+  }

Can we just call static::assertEquals() here? Both methods are functionally identical in the final release of PHPUnit 7.5, there will never be a deprecation warning, and so using reflection here seems like over engineering.

Reference: https://github.com/sebastianbergmann/phpunit/blob/7.5.20/src/Framework/A...

mondrake’s picture

#5 I agree, but then we should make clear that we only support PHPUnit 7.5.0 and above in D8, whereas in the composer.json we currently have "phpunit/phpunit": "^6.5 || ^7".

And if we do that, then we do not even need to have ANY method in the PHPUnit7 version of the trait.

longwave’s picture

Why does this change the version of PHPUnit? parent::assertEquals() will work the same in 7.0.0 up to 7.5.20 unless I am missing something?

edit: in both cases I wrote parent::assertEquals() but really I should say static::assertEquals()!

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Needs review » Needs work

@longwave you're right, thanks

from https://www.php.net/manual/en/language.oop5.traits.php :

Precedence

An inherited member from a base class is overridden by a member inserted by a Trait. The precedence order is that members from the current class override Trait methods, which in turn override inherited methods.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
2.79 KB

Here we go.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, as simple as it can be.

  • catch committed f24bce3 on 8.9.x
    Issue #3126695 by mondrake: [D8 only] Add forwards-compatibility shim...
catch’s picture

Version: 8.9.x-dev » 8.8.x-dev
Issue tags: +Needs release manager review

Committed f24bce3 and pushed to 8.9.x. Thanks!

Thinking about an 8.8 backport here too - it is a small API addition, but it's test-only and enables modules to stay compatible with all three branches easier. But since it's a patch release will get a +1 from xjm too.

catch’s picture

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

Doesn't apply to 8.8.x though, so could use a reroll.

jonathan1055’s picture

Assigned: Unassigned » jonathan1055

OK I'll do the re-roll

jonathan1055’s picture

Assigned: jonathan1055 » Unassigned
Status: Needs work » Needs review
FileSize
3.02 KB

Re-rolled patch #9 for core 8.8

jonathan1055’s picture

Issue tags: -Needs reroll

removing re-roll tag

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Reroll looks good.

  • catch committed ebf37dd on 8.8.x
    Issue #3126695 by mondrake, jonathan1055, catch, longwave: [D8 only] Add...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs release manager review

Originally tagged this for @xjm to look at too, but since this is useful for 8.8 -> 9.0 compatibility and it's test only, I think it's a straightforward decision to backport it after all.

Committed ebf37dd and pushed to 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: +8.9.0 release notes, +8.8.6 release notes

We should put a brief note in the 8.9.0 release notes that a forward-compatibility layer for PHPUnit 9 is provided, and mention it in the 8.8.6 release notes since technically backporting this API is outside our allowed changes policy and there's a small risk it might be disruptive with method name collisions.

xjm’s picture

Status: Closed (fixed) » Needs work
Issue tags: +Needs change record

Oops, this issue is missing a CR. We could add the issue to the CR for #3126787: [D8 only] Add forwards-compatibility shim for assertInternalType() replacements in phpunit 6&7 and cover all the BC layer methods in one place.

daffie’s picture

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

Added a CR.
The CR is not jet published, therefor putting the status to RTBC.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Thanks @daffie -- I guess a separate CR is OK too. Published the CR.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -8.8.6 release notes +8.8.7 release notes