Problem/Motivation

Guzzle 7 is compatible with PSR-18 out of the box, but PSR-17 compatibility is only available with the 2.x branch of the guzzlehttp/psr7 component. (Yes, they acknowledge that the naming is confusing.)

Drupal currently uses Diactoros for its PSR-17 implementation. However, Laminas projects have recently adopted a policy of placing an upper bound on PHP version compatibility, meaning we need to wait for them to do a release after every PHP minor in order to support that minor, which means we could go 6+ months without supporting the latest PHP version. Therefore, it is desirable to look for alternatives to the Laminas packages.

Proposed resolution

Test guzzlehttp/psr7 2.1.0 by explicitly requiring it.

After we land this issue, we move to #3255243: Replace Diactoros' PSR-17 implementation with Guzzle's.

Remaining tasks

TBD

User interface changes

N/A

API changes

TBD (but the PSR interfaces are supposed to be the public API, so...)

Data model changes

TBD

Release notes snippet

guzzlehttp/psr7 has been updated to 2.1.0

CommentFileSizeAuthor
#16 3220220-16.patch4.26 KBlongwave

Issue fork drupal-3220220

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

xjm created an issue. See original summary.

xjm’s picture

Status: Active » Needs review
xjm’s picture

The two fails are the two cases of ProviderRepositoryTest::testEmptyProviderList() (checking for the case where either an empty string or an empty array is returned as the JSON content of the provider database:

Drupal\Tests\media\Functional\ProviderRepositoryTest::testEmptyProviderList with data set "empty array" ('[]')
Failed asserting that exception of type "TypeError" matches expected exception "Drupal\media\OEmbed\ProviderException". Message was: "Double\GuzzleHttp\Psr7\Response\P1::getBody(): Return value must be of type Psr\Http\Message\StreamInterface, string returned" at
/var/www/html/core/modules/media/src/OEmbed/ProviderRepository.php:88
/var/www/html/core/modules/media/tests/src/Functional/ProviderRepositoryTest.php:39
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:722

The test method code:

    $response = $this->prophesize('\GuzzleHttp\Psr7\Response');
    $response->getBody()->willReturn($content);

    $client = $this->createMock('\GuzzleHttp\Client');
    $client->method('request')->withAnyParameters()->willReturn($response->reveal());
    $this->container->set('http_client', $client);

    $this->expectException(ProviderException::class);
    $this->expectExceptionMessage('Remote oEmbed providers database returned invalid or empty list.');
    $this->container->get('media.oembed.provider_repository')->getAll();

GuzzleHttp\Psr7\MessageTrait::getBody() is changed in the 2.x branch to have a return typehint of StreamInterface. (There was no return typehint in the 1.x branch.) This means that the expected exception can't ever be thrown because there's a TypeError before we get to that point.

It makes me suspect the test wasn't quite testing the correct thing to begin with -- even in 1.x, the docs say getBody() should only return StreamInterface or NULL. I suspect what we want to test is that the contents of the response evaluate to the empty values. I don't think we want to change the test to expect TypeError because the point is to test our custom exception handling.

xjm’s picture

Status: Needs review » Needs work
phenaproxima’s picture

It makes me suspect the test wasn't quite testing the correct thing to begin with -- even in 1.x, the docs say getBody() should only return StreamInterface or NULL.

Yeah, I suspect this was just an oversight. I would suggest we change the test to return a stream! I would think this will work:

use GuzzleHttp\Psr7\Utils;

$response = $this->prophesize('\GuzzleHttp\Psr7\Response');
$response->getBody()->willReturn(Utils::streamFor($content));

// ...everything else the same
xjm’s picture

Status: Needs work » Needs review

Locally, #6 does the trick (when the correct namespace for Utils is added which I missed at first). Thanks @phenaproxima!

xjm’s picture

(Assuming that passes, I'll split it into its own issue since it's wrong-ish in HEAD as well.)

andypost’s picture

Title: Test guzzlehttp/psr7 2.0.0-rc1 for PSR-17 compatibility » Test guzzlehttp/psr7 2.0.0 for PSR-17 compatibility

2.0 released

andypost’s picture

Rebased and upgraded guzzlehttp/psr7 to 2.0.0

gábor hojtsy’s picture

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Version: 9.4.x-dev » 10.0.x-dev
Related issues: +#2497699: PSR-7 middleware support
xjm’s picture

Title: Test guzzlehttp/psr7 2.0.0 for PSR-17 compatibility » Update guzzlehttp/psr7 to 2.0.0
Issue tags: +Needs followup

Forgot to mention here what I mentioned on the D10 roadmap. Let's split this into two issues:

  1. Update the MR here for the latest HEAD, review, commit.
  2. Open a new issue where we try to replace Diactoros PSR-17 services with guzzlehttp/psr7 ones.
gábor hojtsy’s picture

Title: Update guzzlehttp/psr7 to 2.0.0 » Update guzzlehttp/psr7 to 2.1.0
Issue summary: View changes
Issue tags: -Needs followup

2.1.0 is available as of two months ago, should we update to that directly? I assume so.

Opened #3255243: Replace Diactoros' PSR-17 implementation with Guzzle's and updated this issue summary.

longwave’s picture

StatusFileSize
new4.26 KB
$ composer update guzzlehttp/psr7
...
$ composer-lock-diff --no-links
+--------------------+-------+-------+
| Production Changes | From  | To    |
+--------------------+-------+-------+
| guzzlehttp/psr7    | 1.8.3 | 2.1.0 |
+--------------------+-------+-------+
andypost’s picture

Status: Needs review » Reviewed & tested by the community

There's follow-up and tests pass

  • catch committed aa1ef1a on 10.0.x
    Issue #3220220 by xjm, andypost, longwave, Gábor Hojtsy, phenaproxima:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed aa1ef1a and pushed to 10.0.x. Thanks!

longwave’s picture

Issue tags: +10.0.0 release notes
gábor hojtsy’s picture

Issue tags: +Needs release note

Also needs release note snippet to be copied :D

catch’s picture

Issue summary: View changes
Issue tags: -Needs release note

Added a release note snippet.

Status: Fixed » Closed (fixed)

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