Skip to content

feat: add ClientService configuration option #59768

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

Open
SMillerDev wants to merge 2 commits into
base: master
Choose a base branch
from
+58 −3
Open

feat: add ClientService configuration option #59768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d7a02b
feat: add ClientService configuration option
SMillerDev Apr 26, 2026
082c137
feat: implement config for newClient
SMillerDev Apr 26, 2026
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 lib/private/Http/Client/ClientService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public function __construct(
) {
}

public function newClient(): IClient {
public function newClient(array $baseConfig = []): IClient {
$handler = new CurlHandler();
$stack = HandlerStack::create($handler);
if ($this->config->getSystemValueBool('dns_pinning', true)) {
Expand All @@ -51,7 +51,8 @@ public function newClient(): IClient {
$this->eventLogger->end('http:request');
}), 'event logger');

$client = new GuzzleClient(['handler' => $stack]);
$config = array_merge($baseConfig, ['handler' => $stack]);
$client = new GuzzleClient($config);

return new Client(
$this->config,
Expand Down
4 changes: 3 additions & 1 deletion lib/public/Http/Client/IClientService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
* @since 8.1.0
*/
interface IClientService {

/**
* @param array $baseConfig default configuration for the client
Copy link
Copy Markdown
Contributor

@come-nc come-nc May 4, 2026

Choose a reason for hiding this comment

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

Which options are supported? If we merge something like that it will need proper documentation.

Copy link
Copy Markdown
Contributor Author

@SMillerDev SMillerDev May 4, 2026

Choose a reason for hiding this comment

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

In practice it would support whatever the GuzzleClient supports. Do you suggest documenting all the options for it?

Copy link
Copy Markdown
Contributor

@come-nc come-nc May 4, 2026

Choose a reason for hiding this comment

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

The point of this wrapper is to hide away Guzzle from apps so that they have a stable API they can use.
I’m not knowledgeable enough to understand whether these options are needed and which ones would be useful.

But either we list here options which are supported and we’ll have to work on keep supporting them (meaning even if Guzzle drops them), or we decide that it’s okay to rely on guzzle for this and we add a link to guzzle documentation to see what’s supported.

Copy link
Copy Markdown
Contributor Author

@SMillerDev SMillerDev May 4, 2026

Choose a reason for hiding this comment

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

For news it's these headers and config values: https://github.com/nextcloud/news/blob/master/lib/Config/FetcherConfig.php#L131-L146

* @return IClient
* @since 8.1.0
*/
public function newClient(): IClient;
public function newClient(array $baseConfig = []): IClient;
}
52 changes: 52 additions & 0 deletions tests/lib/Http/Client/ClientServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,58 @@ public function testNewClient(): void {
);
}

public function testNewClientWithConfig(): void {
/** @var IConfig $config */
$config = $this->createMock(IConfig::class);
$config->method('getSystemValueBool')
->with('dns_pinning', true)
->willReturn(true);
/** @var ICertificateManager $certificateManager */
$certificateManager = $this->createMock(ICertificateManager::class);
$dnsPinMiddleware = $this->createMock(DnsPinMiddleware::class);
$dnsPinMiddleware
->expects($this->atLeastOnce())
->method('addDnsPinning')
->willReturn(function (): void {
});
$remoteHostValidator = $this->createMock(IRemoteHostValidator::class);
$eventLogger = $this->createMock(IEventLogger::class);
$logger = $this->createMock(LoggerInterface::class);
$serverVersion = $this->createMock(ServerVersion::class);

$clientService = new ClientService(
$config,
$certificateManager,
$dnsPinMiddleware,
$remoteHostValidator,
$eventLogger,
$logger,
$serverVersion,
);

$handler = new CurlHandler();
$stack = HandlerStack::create($handler);
$stack->push($dnsPinMiddleware->addDnsPinning());
$stack->push(Middleware::tap(function (RequestInterface $request) use ($eventLogger): void {
$eventLogger->start('http:request', $request->getMethod() . ' request to ' . $request->getRequestTarget());
}, function () use ($eventLogger): void {
$eventLogger->end('http:request');
}), 'event logger');
$guzzleClient = new GuzzleClient(['handler' => $stack, 'timeout' => 2.0]);

$this->assertEquals(
new Client(
$config,
$certificateManager,
$guzzleClient,
$remoteHostValidator,
$logger,
$serverVersion,
),
$clientService->newClient(['timeout' => 2.0])
);
}

public function testDisableDnsPinning(): void {
/** @var IConfig $config */
$config = $this->createMock(IConfig::class);
Expand Down
Loading