Testing system should explain why Guzzle responses can be unreadable

I repeated the test in the issue summary with Drupal 9.4.1:

$ vendor/bin/phpunit -c core core/modules/migrate/tests/src/Functional/process/DownloadFunctionalTest.php
PHPUnit 8.5.26 #StandWithUkraine

Testing Drupal\Tests\migrate\Functional\process\DownloadFunctionalTest
.                                                                   1 / 1 (100%)

Time: 2.04 seconds, Memory: 4.00 MB

OK (1 test, 10 assertions)

HTML output was generated
https://drupal.ddev.site/sites/simpletest/browser_output/Drupal_Tests_migrate_Functional_process_DownloadFunctionalTest-19-15906678.html
https://drupal.ddev.site/sites/simpletest/browser_output/Drupal_Tests_migrate_Functional_process_DownloadFunctionalTest-20-15906678.html

This time, when I open the second link in a browser, I see this:

ID #20 (Previous | Next)
---
Called from GuzzleHttp\Promise\FulfilledPromise::GuzzleHttp\Promise\{closure}() line 41
---
GET request to: http://web/core/misc/favicon.ico
---
---

I think that just means that the stream is still unreadable, but the older version of guzzlehttp/psr7 casts it to an empty string. Before #3285230: Migrate's DownloadFunctionalTest:: testExceptionThrow() is failing on guzzlehttp/psr7 2.3.0, we used @(string) $response->getBody() to cast to a string and suppress errors. (Does that suppress errors but not exceptions?)

According to @mikelutz on Slack:

... we pass a full stream to the sink option for guzzle and not a stream string to make guzzle create it’s own stream for output. ... the way we wire guzzle up in the download plugin, I don’t think there really is such a thing as a ‘source stream’. I think guzzle pumps and drains that source stream into the destination stream right as the response comes in, before it even starts parsing headers or anything. All that’s left IS the destination stream, and that’s probably exactly how it’s supposed to work when you use the sink option with a stream.

Here is the code from the download plugin:

    $destination_stream = @fopen($final_destination, 'w');
    // ...

    // Stream the request body directly to the final destination stream.
    $this->configuration['guzzle_options']['sink'] = $destination_stream;

    try {
      // Make the request. Guzzle throws an exception for anything but 200.
      $this->httpClient->get($source, $this->configuration['guzzle_options']);
    }
    catch (\Exception $e) {
      throw new MigrateException("{$e->getMessage()} ($source)");
    }

The next few lines may also be relevant here:

    if (is_resource($destination_stream)) {
      fclose($destination_stream);
    }

    return $final_destination;

Why do we close the stream and then return it? Is that related to the "HTML output was generated"?

Edit: We return the string $final_destination, not the closed stream, so there is nothing wrong with these lines.

If the quoted explanation in #4 is correct, then the resolution to this issue might be simply to add a code comment. Something like this:

              $stream = $response->getBody();

              // Get the response body as a string. If the request is sent with
              // $options['sink'] = $sink, then $stream is set to $sink, which
              // may not be readable.
              $body = $stream->isReadable()
                ? (string) $stream
                : 'Response is not readable.';

Is that accurate? Can we get a link to the Guzzle docs supporting this?

I cannot find any documentation for how the 'sink' option affects the response, so I am adding a test to confirm the comment.

MR 2469 adds the test and a comment similar to the one in #5.

I am giving @mikelutz credit for the helpful discussion on Slack. See #4.

Is Drupal\Tests\Core\Http a good namespace for the new test?

@mikelutz:

Thanks for the review.

If anyone else is interested, @mikelutz and I discussed this issue at length during the #3294422: [meeting] Migrate Meeting 2022-07-07 2100Z.

I updated the MR, removing the test and changing the comment to this:

              // Get the response body as a string. The response stream is set
              // to the sink, which defaults to a readable temp stream but can
              // be overridden by setting $options['sink'].

Part of our discussion is that Guzzle is not doing anything wrong, and our comment should not suggest that it is. In that spirit, I am updating the issue title.