[D8 only] Add forwards-compatibility shim for assertInternalType() replacements in phpunit 6&7

see related issues for similar implementation

$this->assertInternalType('iterable', $variable); is not supported in PHPUnit 6, calling is_iterable() instead, see the test-only patch attached, \Drupal\Tests\node\Unit\PageCache\DenyNodePreviewTest::testIsIterableWithBuiltInAssertion should fail

assertIsArray()
assertIsBool()
assertIsFloat()
assertIsInt()
assertIsNumeric()
assertIsObject()
assertIsResource()
assertIsString()
assertIsScalar()
assertIsCallable()
assertIsIterable()
assertIsNotArray()
assertIsNotBool()
assertIsNotFloat()
assertIsNotInt()
assertIsNotNumeric()
assertIsNotObject()
assertIsNotResource()
assertIsNotString()
assertIsNotScalar()
assertIsNotCallable()
assertIsNotIterable()

Full list of alternatives to assertInternalType() and assertNotInternalType(), see https://github.com/sebastianbergmann/phpunit/issues/3368#issue-373444914

It's a wrong test only patch in #8

Failed as expected

Brilliant. Just the PHPUnit 6 version of the trait should not have the void return typehint, so tests can run on PHP 7.0 too. I think string $message typehint can work, though.

@mondrake, thanks for your review!

Removed the PHPUnit 6 version of void return type hint, and string $message type-hint too, to keep consistency with other ones in PHPUnit 6 version.

Looks good to go, thanks

A patch for 8.8.x in case it's needed

FWIW I'm potentially +1 on backporting this to 8.8.x, not for contrib's convenience, but so that we don't constantly break HEAD by backporting the new assertion types. I'll double-check with @catch before calling that RM signoff, though.

Meanwhile, a couple nitpicks:

+++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit6/TestCompatibilityTrait.php
@@ -81,4 +81,158 @@ public static function assertNotEqualsCanonicalizing($expected, $actual, $messag
+   * Forward compatibility for assertIsArray.

• "Forward-compatibility" should be hyphenated.
• Method names should always be followed by () in documentation.
• Also, we have a silly rule that requires function and class one-line summaries to start with a verb.

So, between all that, I think this should read:

Provides forward-compatibility for assertIsArray().

(And similar for the rest of the methods.)

Thanks @xjm!

1. Addressing #18
2. is_iterable() is available in PHP 7.1, (PHP 7 >= 7.1.0), the min version of PHP in 8.8.x/8.9.x is 7.0.8, so dropped related assertions, see https://www.php.net/manual/en/function.is-iterable.php
3. Comments of assertStringNotContainsStringIgnoringCase()-ish in TestCompatibilityTraits introduced by #3126797, do not follow "Provides ...", but it's out of scope here, so keep them untouched. If it's necessary, to correct them in a separate issue. Furthermore, Provides forward-compatibility for assertStringNotContainsStringIgnoringCase() exceeds 80 chars, for cases like this, suggest replacing forward-compatibility with FC to bypass exceeding

@jungle is working on a CR for this.

CR added.

The CR looks pretty good. I made a few improvements. Reviewing now...

Under a very strict reading of coding standards, these should have @param documentation, but it'd be pretty redundant (and it's a temporary shim anyway and the other methods in the shim are also lacking it), so I'm fine leaving that out here. Maybe some day we'll enable the phpcs rule for @param, but that's a long way off and it'd only happen in D9 if so.

1. +++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit6/TestCompatibilityTrait.php
   @@ -81,4 +81,144 @@ public static function assertNotEqualsCanonicalizing($expected, $actual, $messag
   +  /**
   +   * Provides forward-compatibility for assertIsObject().
   +   */
   +  public static function assertIsInt($actual, $message = '') {
   

   Oops, there's a mismatch here between the docblock and the method.

2. +++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit6/TestCompatibilityTrait.php
   @@ -81,4 +81,144 @@ public static function assertNotEqualsCanonicalizing($expected, $actual, $messag
   +  /**
   +   * Provides forward-compatibility for assertIsNotObject().
   +   */
   +  public static function assertIsNotInt($actual, $message = '') {
   

   Again here.

3. +++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit7/TestCompatibilityTrait.php
   @@ -81,4 +81,144 @@ public static function assertNotEqualsCanonicalizing($expected, $actual, string
   +   * Provides forward-compatibility for assertIsObject().
   +   */
   +  public static function assertIsInt($actual, string $message = ''): void {
   

   And here.

4. +++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit7/TestCompatibilityTrait.php
   @@ -81,4 +81,144 @@ public static function assertNotEqualsCanonicalizing($expected, $actual, string
   +  /**
   +   * Provides forward-compatibility for assertIsNotObject().
   +   */
   +  public static function assertIsNotInt($actual, string $message = ''): void {
   

   And (as expected) here too.

Everything else looks good.

Nice catch. Thanks, @xjm, my bad.

Addressing #24

#24 is fixed.

Committed and pushed to 8.9.x, thanks!

Leaving RTBC against 8.8.x for the moment because we're still weighing the pros and cons of backporting these to 8.8.

Okay, I changed my mind about 8.8.x. There is some risk of disruption, and the final bugfix window for 8.8.x is next week. We shouldn't risk any disruption in such a late patch release.

The main downside is that we might accidentally complicate testing for security issues by introducing calls to these methods in the test for a security issue sometime in the next six months. However, we often withhold the tests for security issues for a couple months anyway (to avoid disclosing more information that the attacker could use for the exploit), which reduces the impact of that. And, if this does come up in a security issue between now and December, well, we can just deal with it.

It's also slightly less convenient for contrib projects that are supporting 8.8 through 9.0 together, but that's not different from any other API addition.

Thanks!

@MegaChriz asked about the deprecation errors for these, and in the course of discussion it came up that #3126797: [D8 only] Add forwards-compatibility shim for assertString(Not)ContainsString()replacements in phpunit 6&7 has been backported to 8.8.x already, so having the one issue backported but not the other led to them having to create their own methods on 8.8, and confusion about which methods were available. Based on that I'm going to backport this to 8.8.x before the final patch release.

Committed the 8.8.x patch from #26. Thanks for everyone who helped discuss this again this morning!

I also published the CR which I apparently neglected to do before. That didn't help the situation.

I'm wondering if we should put a brief note in the 8.9.0 release notes that a forward-compatibility layer for PHPUnit 9 is provided. Also probably 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.

Setting to Needs Work as this is tagged as needing a release note snippet but there currently isn't one.

I think we will have just one release note lumping all the issues together.

Release notes snippet added, by replacing the CR URL in the template which @xjm sent on slack.