Migrate's DownloadFunctionalTest:: testExceptionThrow() is failing on guzzlehttp/psr7 2.3.0

This has to do with this change, Safely get stream contents. There is now a check that a stream is readable before attempting to get the contents. Well, that makes sense. What doesn't make sense is that it is failing when trying to read the destination, which should only be written to.

If I change the mode of the destination to be w+' instead of 'w', (and make adjustments to the exception message) this will pass.

Why does guzzle think it needs to read the destination?

I haven't dug too far into this but my feeling, based on looking at the failed assertion @xjm posted in #14, is that the test is overzealous. It looks like it should be asserting a simple 404, but is asserting the entire exception message, which inherently ties it to how Guzzle internally wants to work. I'd suggest adjusting the test expectation so it's looking for a 404 status code, regardless of whatever the actual message is.

I'd probably just assert the presence of 404 Not Found in the exception message with $this->assertStringContainsStringIgnoringCase().

But that's not present in the message at all now.

Ah yes, that's true. I must have been malcaffeinated when I wrote #19.

Another option, then, is change handleException() to include the exception code in the logged message, along with some hard-coded prefix. For example, assuming a 404 error has a code of 404:

 protected function handleException(\Exception $exception, $save = TRUE) {
    $result = Error::decodeException($exception);
    $message = '[Guzzle] Error ' . $exception->getCode() . ': ' . $result['@message'] . ' (' . $result['%file'] . ':' . $result['%\
line'] . ')';
    if ($save) {
      $this->saveMessage($message);
    }
    $this->message->display($message, 'error');
  }

Then in the test you could have something like $this->assertStringStartsWith('[Guzzle] Error 404:', $message);.

Self-assigning to smash this bug.

The attached patch is obviously not the right fix, but it points out where the problem really is.

Short version: the test is broken.

We had a rollicking discussion about this in Slack today, mostly during the weekly migration meeting. I will summarize the discussion later. (Time for supper.)

Also, does someone want to open a separate issue for this? There are two very different problems here: the changed exception message because of the Guzzle update and then the broken attempt to cast a stream to a string so that we can log it.

I was in a hurry to get supper ... pretend you never saw the files attached to #27.

+++ b/core/tests/Drupal/Tests/BrowserHtmlDebugTrait.php
@@ -183,7 +184,12 @@ protected function getResponseLogHandler() {
               // tests should not fail if there is a problem. On PHP 7.4
               // \Drupal\Tests\migrate\Functional\process\DownloadFunctionalTest
               // fails without the usage of a silence operator.
-              $body = @(string) $response->getBody();

Hate to send a critical back for a nit, but this comment is now nonsensical
- edit Maybe not nonsensical, since the silence is still there a few lines down, but I also question if the silence is still necessary if we are no longer casting a stream.

Leaving at NR, the comment could be updated, but It's not as off as I first thought, and I really don't know whether the silence is still needed or not.

Yeah, I'll agree with out of scope for this critical. RTBC pending tests.

Looks like 10.0 needs a re-roll

Thanks to @quietone for #15 (and to @xjm for bringing that comment to my attention):

Why does guzzle think it needs to read the destination?

Good question!

I hacked the exception message in Guzzle, and that confirmed what @quietone said: Guzzle was calling getContents() on the destination stream. Then I hacked the download process plugin to add a stack trace to its exception message:

#0 /var/www/html/vendor/guzzlehttp/psr7/src/Stream.php(83): GuzzleHttp\Psr7\Stream->getContents()
#1 /var/www/html/core/tests/Drupal/Tests/BrowserHtmlDebugTrait.php(186): GuzzleHttp\Psr7\Stream->__toString()
#2 /var/www/html/vendor/guzzlehttp/promises/src/FulfilledPromise.php(41): Drupal\Tests\BrowserTestBase->Drupal\Tests\{closure}(Object(GuzzleHttp\Psr7\Response))

So it seems that BrowserHtmlDebugTrait::getResponseLogHandler() is trying to cast a Stream object to a string:

              // Get the response body as a string. Any errors are silenced as
              // tests should not fail if there is a problem. On PHP 7.4
              // \Drupal\Tests\migrate\Functional\process\DownloadFunctionalTest
              // fails without the usage of a silence operator.
              $body = @(string) $response->getBody();

The comment, and the cast, was added in #3086374-79: Make Drupal 8 & 9 compatible with PHP 7.4. I am adding that as a related issue.

While you were busy cross-posting, I was checking the return type of getBody(). Like @mikelutz, I think the comment should be removed.

We do not need that use statement any more.

Maybe we should raise an upstream issue with Guzzle. According to the API docs for Psr\Http\Message\StreamInterface::__toString,

This method MUST NOT raise an exception in order to conform with PHP's string casting operations.

Back to RTBC pending tests.

Now that we understand the problem better, let's update the IS.

I added #3292980: Testing system should explain why Guzzle responses can be unreadable as a follow-up issue, so I am updating the "Remaining tasks" in the issue summary. I also copied the other task to the new issue.

Oops:

              /* $var \Psr\Http\Message\StreamInterface $stream */

should have been

              /** @var \Psr\Http\Message\StreamInterface $stream */

Should we add a commit to this issue or make a follow-up issue?

Make a novice issue, let somebody get credit for it. If it survives the night I'll have let someone on my team get their first core credit tomorrow.

I opened #3293114: Fix var tag on recent commit, and am going to try to walk someone on my team through their first contribution this morning.

Closing a few loops on this issue.

Regarding

Why does guzzle think it needs to read the destination?

I posted a fairly detailed explaination here, but the short of it is that guzzle by default takes the download stream, dumps it into a sink stream and returns that stream for $response->getBody(). There is a guzzle option ($options['stream'] = TRUE) that would return the download stream instead, but it's rarely used, and certainly not by us here. Guzzle uses a php temp stream opened 'r+' (read/write) by default, and we overrode it to a write- only file stream, so it's not doing anything unusual, we just overrode the typically readable stream with a write-only stream.

Additionally:
We discussed a follow up to report the psr 7 violation of Stream::__toString throwing an exception in violation of psr7 StreamInterface. I dug in through the history, and the short version is they had a discussion and intentionally chose to throw the exception anyway.

The slightly longer version is that psr-7 was accepted in 2015, back in the php 5.6 days. Per the php docs for __toString()

Warning
It was not possible to throw an exception from within a __toString() method prior to PHP 7.4.0. Doing so will result in a fatal error.

Guzzle chose to interpret

This method MUST NOT raise an exception in order to conform with PHP's string casting operations.

.
to mean that they weren't permitted to throw an exception prior to php 7.4 due to php throwing a fatal error, but after 7.4 they were permitted to throw an exception. There was discussion of attempting to file an errata with PHP-FIG to change psr7, not sure if they did, but they didn't have much hope of getting it updated. Hence why the code in __toString catches exceptions and only rethrows them in php 7.4.

On top of that discussion which was a couple years ago, and only threw the exception on a detached stream, https://github.com/guzzle/psr7/pull/515/files mentioned above expanded the situations where an exception is thrown, including unreadable streams.

Further reading:
https://github.com/guzzle/psr7/issues/271
https://github.com/Nyholm/psr7/issues/142

We've also opened #3293114: Fix var tag on recent commit (committed) to fix the @var typo, and #3292980: Testing system should explain why Guzzle responses can be unreadable(RTBC) for further discussion and to update some documentation

Since it seems pointless to follow up upstream on the PSR violations. and the other internal follow-ups have been filed, I'm removing the Needs Followup tag.