Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple from "guzzlehttp/psr7" and "php-http/discovery", replace by "friendsofphp/well-known-implementations" #1418

Closed
wants to merge 1 commit into from
+40 −41
Closed

Decouple from "guzzlehttp/psr7" and "php-http/discovery", replace by "friendsofphp/well-known-implementations" #1418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,11 @@
"php": "^7.2|^8.0",
"ext-json": "*",
"ext-mbstring": "*",
"friendsofphp/well-known-implementations": "^1",
"guzzlehttp/promises": "^1.4",
"guzzlehttp/psr7": "^1.8.4|^2.1.1",
"jean85/pretty-package-versions": "^1.5|^2.0.4",
"php-http/async-client-implementation": "^1.0",
"php-http/client-common": "^1.5|^2.0",
"php-http/discovery": "^1.11",
"php-http/httplug": "^1.1|^2.0",
"php-http/message": "^1.5",
"psr/http-factory": "^1.0",
Expand All @@ -39,6 +38,7 @@
},
"require-dev": {
"friendsofphp/php-cs-fixer": "^2.19|3.4.*",
"guzzlehttp/psr7": "^1.8.4|^2.1.1",
"http-interop/http-factory-guzzle": "^1.0",
"monolog/monolog": "^1.6|^2.0|^3.0",
"nikic/php-parser": "^4.10.3",
Expand Down Expand Up @@ -89,6 +89,7 @@
"sort-packages": true,
"allow-plugins": {
"composer/package-versions-deprecated": true,
"friendsofphp/well-known-implementations": false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running the plugin is not needed for tests. Only the "lib" part of the package is used when running the CI.

"phpstan/extension-installer": true
}
},
Expand Down
14 changes: 7 additions & 7 deletions src/ClientBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

namespace Sentry;

use Http\Discovery\Psr17FactoryDiscovery;
use FriendsOfPHP\WellKnownImplementations\WellKnownPsr17Factory;
use Psr\Log\LoggerInterface;
use Sentry\HttpClient\HttpClientFactory;
use Sentry\Serializer\RepresentationSerializerInterface;
Expand Down Expand Up @@ -175,19 +175,19 @@ private function createTransportInstance(): TransportInterface
*/
private function createDefaultTransportFactory(): DefaultTransportFactory
{
$streamFactory = Psr17FactoryDiscovery::findStreamFactory();
$psr17Factory = new WellKnownPsr17Factory();
$httpClientFactory = new HttpClientFactory(
Psr17FactoryDiscovery::findUriFactory(),
Psr17FactoryDiscovery::findResponseFactory(),
$streamFactory,
null,
null,
Copy link
Contributor Author

@nicolas-grekas nicolas-grekas Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first two arguments of HttpClientFactory are never used. We can thus make them nullable and skip injecting factories there.

$psr17Factory,
null,
$this->sdkIdentifier,
$this->sdkVersion
);

return new DefaultTransportFactory(
$streamFactory,
Psr17FactoryDiscovery::findRequestFactory(),
$psr17Factory,
$psr17Factory,
$httpClientFactory,
$this->logger
);
Expand Down
19 changes: 10 additions & 9 deletions src/HttpClient/HttpClientFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

namespace Sentry\HttpClient;

use FriendsOfPHP\WellKnownImplementations\ConcreteImplementation;
use FriendsOfPHP\WellKnownImplementations\WellKnownHttplugClient;
use GuzzleHttp\RequestOptions as GuzzleHttpClientOptions;
use Http\Adapter\Guzzle6\Client as GuzzleHttpClient;
use Http\Client\Common\Plugin\AuthenticationPlugin;
Expand All @@ -14,7 +16,6 @@
use Http\Client\Common\PluginClient;
use Http\Client\Curl\Client as CurlHttpClient;
use Http\Client\HttpAsyncClient as HttpAsyncClientInterface;
use Http\Discovery\HttpAsyncClientDiscovery;
use Psr\Http\Client\ClientInterface;
use Psr\Http\Message\ResponseFactoryInterface;
use Psr\Http\Message\StreamFactoryInterface;
Expand Down Expand Up @@ -54,16 +55,16 @@ final class HttpClientFactory implements HttpClientFactoryInterface
/**
* Constructor.
*
* @param UriFactoryInterface $uriFactory The PSR-7 URI factory
* @param ResponseFactoryInterface $responseFactory The PSR-7 response factory
* @param UriFactoryInterface|null $uriFactory The PSR-7 URI factory
* @param ResponseFactoryInterface|null $responseFactory The PSR-7 response factory
* @param StreamFactoryInterface $streamFactory The PSR-17 stream factory
* @param HttpAsyncClientInterface|null $httpClient The HTTP client
* @param string $sdkIdentifier The SDK identifier
* @param string $sdkVersion The SDK version
*/
public function __construct(
UriFactoryInterface $uriFactory,
ResponseFactoryInterface $responseFactory,
?UriFactoryInterface $uriFactory,
?ResponseFactoryInterface $responseFactory,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those arguments are never used, no need to force passing anything.

StreamFactoryInterface $streamFactory,
?HttpAsyncClientInterface $httpClient,
string $sdkIdentifier,
Expand Down Expand Up @@ -110,7 +111,7 @@ public function create(Options $options): HttpAsyncClientInterface
*/
private function resolveClient(Options $options)
{
if (class_exists(SymfonyHttplugClient::class)) {
if (ConcreteImplementation::VENDOR_SYMFONY === ConcreteImplementation::HTTPLUG_VENDOR) {
Copy link
Contributor Author

@nicolas-grekas nicolas-grekas Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using these consts gives more control over which implementation should be used (the consts are generated by the plugin xor computed at autoload-time when the plugin is disabled)

$symfonyConfig = [
'timeout' => $options->getHttpConnectTimeout(),
'max_duration' => $options->getHttpTimeout(),
Expand All @@ -123,7 +124,7 @@ private function resolveClient(Options $options)
return new SymfonyHttplugClient(SymfonyHttpClient::create($symfonyConfig));
}

if (class_exists(GuzzleHttpClient::class)) {
if (ConcreteImplementation::VENDOR_HTTPLUG_GUZZLE6 === ConcreteImplementation::HTTPLUG_VENDOR) {
$guzzleConfig = [
GuzzleHttpClientOptions::TIMEOUT => $options->getHttpTimeout(),
GuzzleHttpClientOptions::CONNECT_TIMEOUT => $options->getHttpConnectTimeout(),
Expand All @@ -136,7 +137,7 @@ private function resolveClient(Options $options)
return GuzzleHttpClient::createWithConfig($guzzleConfig);
}

if (class_exists(CurlHttpClient::class)) {
if (ConcreteImplementation::VENDOR_HTTPLUG_CURL === ConcreteImplementation::HTTPLUG_VENDOR) {
$curlConfig = [
\CURLOPT_TIMEOUT => $options->getHttpTimeout(),
\CURLOPT_CONNECTTIMEOUT => $options->getHttpConnectTimeout(),
Expand All @@ -153,6 +154,6 @@ private function resolveClient(Options $options)
throw new \RuntimeException('The "http_proxy" option requires either the "php-http/curl-client", the "symfony/http-client" or the "php-http/guzzle6-adapter" package to be installed.');
}

return HttpAsyncClientDiscovery::find();
return new WellKnownHttplugClient();
}
}
4 changes: 2 additions & 2 deletions src/Integration/RequestFetcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

namespace Sentry\Integration;

use GuzzleHttp\Psr7\ServerRequest;
use FriendsOfPHP\WellKnownImplementations\WellKnownPsr17Factory;
use Psr\Http\Message\ServerRequestInterface;

/**
Expand All @@ -22,6 +22,6 @@ public function fetchRequest(): ?ServerRequestInterface
return null;
}

return ServerRequest::fromGlobals();
return (new WellKnownPsr17Factory())->createServerRequestFromGlobals();
Copy link
Contributor Author

@nicolas-grekas nicolas-grekas Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes the coupling to guzzlehttp/psr7: "well-known-implem" provides the same feature, but compatible with any PSR-7 implementations.

}
}
26 changes: 13 additions & 13 deletions tests/HttpClient/HttpClientFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

namespace Sentry\Tests\HttpClient;

use FriendsOfPHP\WellKnownImplementations\WellKnownPsr17Factory;
use Http\Client\HttpAsyncClient as HttpAsyncClientInterface;
use Http\Discovery\Psr17FactoryDiscovery;
use Http\Mock\Client as HttpMockClient;
use PHPUnit\Framework\TestCase;
use Sentry\HttpClient\HttpClientFactory;
Expand All @@ -19,13 +19,13 @@ final class HttpClientFactoryTest extends TestCase
*/
public function testCreate(bool $isCompressionEnabled, string $expectedRequestBody): void
{
$streamFactory = Psr17FactoryDiscovery::findStreamFactory();
$psr17Factory = new WellKnownPsr17Factory();

$mockHttpClient = new HttpMockClient();
$httpClientFactory = new HttpClientFactory(
Psr17FactoryDiscovery::findUrlFactory(),
Psr17FactoryDiscovery::findResponseFactory(),
$streamFactory,
null,
null,
$psr17Factory,
$mockHttpClient,
'sentry.php.test',
'1.2.3'
Expand All @@ -37,9 +37,9 @@ public function testCreate(bool $isCompressionEnabled, string $expectedRequestBo
'enable_compression' => $isCompressionEnabled,
]));

$request = Psr17FactoryDiscovery::findRequestFactory()
$request = $psr17Factory
->createRequest('POST', 'http://example.com/sentry/foo')
->withBody($streamFactory->createStream('foo bar'));
->withBody($psr17Factory->createStream('foo bar'));

$httpClient->sendAsyncRequest($request);

Expand Down Expand Up @@ -67,9 +67,9 @@ public function createDataProvider(): \Generator
public function testCreateThrowsIfDsnOptionIsNotConfigured(): void
{
$httpClientFactory = new HttpClientFactory(
Psr17FactoryDiscovery::findUrlFactory(),
Psr17FactoryDiscovery::findResponseFactory(),
Psr17FactoryDiscovery::findStreamFactory(),
null,
null,
new WellKnownPsr17Factory(),
null,
'sentry.php.test',
'1.2.3'
Expand All @@ -84,9 +84,9 @@ public function testCreateThrowsIfDsnOptionIsNotConfigured(): void
public function testCreateThrowsIfHttpProxyOptionIsUsedWithCustomHttpClient(): void
{
$httpClientFactory = new HttpClientFactory(
Psr17FactoryDiscovery::findUrlFactory(),
Psr17FactoryDiscovery::findResponseFactory(),
Psr17FactoryDiscovery::findStreamFactory(),
null,
null,
new WellKnownPsr17Factory(),
$this->createMock(HttpAsyncClientInterface::class),
'sentry.php.test',
'1.2.3'
Expand Down
9 changes: 5 additions & 4 deletions tests/HttpClient/Plugin/GzipEncoderPluginTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

namespace Sentry\Tests\HttpClient\Plugin;

use Http\Discovery\Psr17FactoryDiscovery;
use FriendsOfPHP\WellKnownImplementations\WellKnownPsr17Factory;
use Http\Promise\Promise as PromiseInterface;
use PHPUnit\Framework\TestCase;
use Psr\Http\Message\RequestInterface;
Expand All @@ -17,11 +17,12 @@ final class GzipEncoderPluginTest extends TestCase
{
public function testHandleRequest(): void
{
$plugin = new GzipEncoderPlugin(Psr17FactoryDiscovery::findStreamFactory());
$psr17Factory = new WellKnownPsr17Factory();
$plugin = new GzipEncoderPlugin($psr17Factory);
$expectedPromise = $this->createMock(PromiseInterface::class);
$request = Psr17FactoryDiscovery::findRequestFactory()
$request = $psr17Factory
->createRequest('POST', 'http://www.example.com')
->withBody(Psr17FactoryDiscovery::findStreamFactory()->createStream('foo'));
->withBody($psr17Factory->createStream('foo'));

$this->assertSame('foo', (string) $request->getBody());
$this->assertSame($expectedPromise, $plugin->handleRequest(
Expand Down
4 changes: 0 additions & 4 deletions tests/bootstrap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

declare(strict_types=1);

use Http\Discovery\ClassDiscovery;
use Http\Discovery\Strategy\MockClientStrategy;
use Sentry\Breadcrumb;
use Sentry\Event;
use Sentry\Tracing\Span;
Expand All @@ -12,8 +10,6 @@

require_once __DIR__ . '/../vendor/autoload.php';

ClassDiscovery::appendStrategy(MockClientStrategy::class);
Copy link
Contributor Author

@nicolas-grekas nicolas-grekas Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"well-known-implem" will discover and auto-load php-http/mock-client when no other implem is found.


// According to the Symfony documentation the proper way to register the mocked
// functions for a certain class would be to configure the listener in the
// phpunit.xml file, however in our case it doesn't work because PHPUnit loads
Expand Down