Skip to content

Conversation

@GaryPEGEOT
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets fixes partially #212
Documentation php-http/documentation#220
License MIT

What's in this PR?

Add a mock client factory to ease functional testing.

Example Usage

Configuration:

    # config_test.yml
    httplug:
        clients:
            my_awesome_client:
                factory: 'httplug.factory.mock' # replace factory

Usage:

$client = static::createClient();
// $client->disableReboot(); You might uncomment this if your client (BrowserKit) make multiple requests as kernel is rebooted on each request.

$response = $this->createMock('Psr\Http\Message\ResponseInterface');
$response->method('getBody')->willReturn(/* Psr\Http\Message\Interface instance containing expected response content. */);
$client->getContainer()->get('httplug.client.mock')->addResponse($response);

Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

great idea, thanks!

you could add the mock client to the composer.json suggest section and propose to add it in require-dev of the users project.

*/
public function createClient(array $config = [])
{
if (!class_exists('Http\Mock\Client')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use the ::class pseudoconstant. we are on php 5.5 or newer.

}
}

$mockClass = 'Http\Mock\Client';
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets use the ::class pseudoconstant here. and make the use statement say use Http\Mock\Client as MockClient; to make things more readable.

throw new \LogicException('To use the mock adapter you need to install the "php-http/mock-client" package.');
}

return $this->client ?: new Client();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will create a new client on each call, if none has been registered. should we instead register the newly created client if none was set?

<service id="httplug.factory.socket" class="Http\HttplugBundle\ClientFactory\SocketFactory" public="false">
<argument type="service" id="httplug.message_factory"/>
</service>
<service id="httplug.factory.mock" class="Http\HttplugBundle\ClientFactory\MockFactory" public="false" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this service should be removed from the container when the mock client is not available. that way, users notice problems at container build time instead of at runtime.

Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks, looks good to me!

Copy link
Contributor

@fbourigault fbourigault left a comment

Choose a reason for hiding this comment

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

Great! I wrote a comment to improve the readability.

$mockServiceId = 'httplug.client.mock';

if (!\class_exists(MockClient::class)) {
$container->removeDefinition('httplug.factory.mock');
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of removing a definition when the class does not exist, could you move the relevant service definition to a dedicated file and load this file if the class is available?

Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

looks good to me, thanks!

@dbu dbu merged commit 7564416 into php-http:master Jan 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants