EntityResourceTestBase::assert406Response uses a wrong assertion and results in false positives (and fails for PHPUnit 7)

It's meant to assert that the substring in the first parameter is present in the second parameter. The second parameter used to return a string, not an array

So apparently what I said in #2 is still true, sometimes 🙃

The need for this change is caused by https://github.com/sebastianbergmann/phpunit/commit/5113b86ce113e531b65b....

All this change does is guarantee that string is passed to $this->stringContains(), instead of potentially string[].

RTBC. Thanks for doing this!

That's not what the patch is doing :)

NW for #14/#15.

Looks like there is sometimes no header at all and then it fails on that?

Thanks, @mondrake, #36 looks great :)

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1538,8 +1538,13 @@ protected function assert406Response(ResponseInterface $response) {
+      if ($actual_link_header) {

any way to find out when this if() triggers and add an else? Because currently we have no idea if this part of the test is EVER run, the if could be if ($i_am_always_false) and the test would still be green.

Let's answer #38

+1 RTBC

1. $response->getHeader() always returns an array so $this->assertTrue(is_array($actual_link_header)); is unnecessary.

2. We can use $response->hasHeader('Link').

vendor\psr\http-message\src\MessageInterface.php

/**
 * Checks if a header exists by the given case-insensitive name.
 *
 * @param string $name Case-insensitive header field name.
 * @return bool Returns true if any header names match the given header
 *     name using a case-insensitive string comparison. Returns false if
 *     no matching header name is found in the message.
 */
public function hasHeader($name);

/**
 * Retrieves a message header value by the given case-insensitive name.
 *
 * This method returns an array of all the header values of the given
 * case-insensitive header name.
 *
 * If the header does not appear in the message, this method MUST return an
 * empty array.
 *
 * @param string $name Case-insensitive header field name.
 * @return string[] An array of string values as provided for the given
 *    header. If the header does not appear in the message, this method MUST
 *    return an empty array.
 */
public function getHeader($name);

My suggestions

if ($response->hasHeader('Link')) {
  $actual_link_header = $response->getHeader('Link');
  $expected_type = explode(';', static::$mimeType)[0];
  $this->assertContains('?_format=' . static::$format . '>; rel="alternate"; type="' . $expected_type . '"', $actual_link_header[0]);
  $this->assertContains('?_format=foobar>; rel="alternate"', $actual_link_header[0]);
}

Interdiff

diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
index d4b111c..a5b6df6 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1538,9 +1538,8 @@ protected function assert406Response(ResponseInterface $response) {
     else {
       // This is the desired response.
       $this->assertSame(406, $response->getStatusCode());
-      $actual_link_header = $response->getHeader('Link');
-      if ($actual_link_header) {
-        $this->assertTrue(is_array($actual_link_header));
+      if ($response->hasHeader('Link')) {
+        $actual_link_header = $response->getHeader('Link');
         $expected_type = explode(';', static::$mimeType)[0];
         $this->assertContains('?_format=' . static::$format . '>; rel="alternate"; type="' . $expected_type . '"', $actual_link_header[0]);
         $this->assertContains('?_format=foobar>; rel="alternate"', $actual_link_header[0]);

Committed b57f550 and pushed to 8.8.x. Thanks!