Skip to content

chore(): Remove cache dependency and move others to dev dependencies #11

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

Merged
merged 4 commits into from
Oct 22, 2021
Merged

chore(): Remove cache dependency and move others to dev dependencies #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9afb144
chore(): Remove cache dependency and move others to dev dependencies
TiMESPLiNTER Aug 12, 2021
2d56565
chore(): Remove unused use statement
TiMESPLiNTER Aug 12, 2021
d1a4bb1
chore(): Improve mutant kill rate
TiMESPLiNTER Aug 12, 2021
dc90a86
chore(): Fix infection testing problems
TiMESPLiNTER Aug 12, 2021
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
23 changes: 12 additions & 11 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,24 @@
"homepage": "https://careers.jobcloud.ch/en/our-team/?term=133"
}
],
"config": {
"sort-packages": true
},
"require": {
"php": ">=7.4",
"ext-json": "*",
"pimple/pimple": "^3.2",
"symfony/cache": "^5.0",
"nyholm/psr7": "^1.2",
"kriswallsmith/buzz": "^1.0",
"psr/http-message": "^1.0"
"psr/http-message": "^1.0",
"psr/http-client": "^1.0.1"
},
"require-dev": {
"squizlabs/php_codesniffer": "^3.4.2",
"phpunit/phpunit": "^9.5",
"infection/infection": "^0.21",
"kriswallsmith/buzz": "^1.0",
"nyholm/psr7": "^1.2",
"php-mock/php-mock-phpunit": "^2.6",
"phpstan/phpstan": "^0.12",
"phpunit/phpunit": "^9.5",
"pimple/pimple": "^3.2",
"rregeer/phpunit-coverage-check": "^0.3.1",
"infection/infection": "^0.21"
"squizlabs/php_codesniffer": "^3.4.2"
},
"config": {
"sort-packages": true
}
}
14 changes: 13 additions & 1 deletion phpunit.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,17 @@
<?xml version="1.0" encoding="UTF-8"?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" backupGlobals="false" backupStaticAttributes="false" colors="true" convertErrorsToExceptions="true" convertNoticesToExceptions="true" convertWarningsToExceptions="true" processIsolation="false" stopOnFailure="false" bootstrap="vendor/autoload.php" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd">
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd"
backupGlobals="false"
backupStaticAttributes="false"
colors="true"
convertErrorsToExceptions="true"
convertNoticesToExceptions="true"
convertWarningsToExceptions="true"
processIsolation="false"
stopOnFailure="false"
bootstrap="vendor/autoload.php"
executionOrder="random"
resolveDependencies="true">
<coverage>
<include>
<directory>src</directory>
Expand Down
20 changes: 12 additions & 8 deletions src/ServiceProvider/KafkaSchemaRegistryApiClientProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@

use Buzz\Client\Curl;
use Jobcloud\Kafka\SchemaRegistryClient\ErrorHandler;
use Jobcloud\Kafka\SchemaRegistryClient\ErrorHandlerInterface;
use Jobcloud\Kafka\SchemaRegistryClient\HttpClient;
use Jobcloud\Kafka\SchemaRegistryClient\HttpClientInterface;
use Jobcloud\Kafka\SchemaRegistryClient\KafkaSchemaRegistryApiClient;
use Jobcloud\Kafka\SchemaRegistryClient\KafkaSchemaRegistryApiClientInterface;
use LogicException;
use Nyholm\Psr7\Factory\Psr17Factory;
use Pimple\Container;
Expand Down Expand Up @@ -35,26 +38,26 @@ public function register(Container $container): void
{
$this->checkRequiredOffsets($container);

if (false === isset($container[self::REQUEST_FACTORY])) {
$container[self::REQUEST_FACTORY] = static function (Container $container) {
if (false === isset($container[self::REQUEST_FACTORY]) && class_exists('Nyholm\Psr7\Factory\Psr17Factory')) {
Copy link
Contributor

@healerz healerz Aug 12, 2021

Choose a reason for hiding this comment

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

I'm not a big fan of service providers in libraries anymore, how about removing it and "replace" it with a "how to use" part in the readme? 😉

Copy link
Contributor Author

@TiMESPLiNTER TiMESPLiNTER Aug 13, 2021

Choose a reason for hiding this comment

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

So what do you do then? Just duplicating stuff? I was also thinking about the whole reusable service providers. In this case here we could make it configurable with service ids you can inject over the constructor so it can locate a request factory and a client and works for the schema api client... or do you just started duplicating the whole service provider logic over and over again?

Copy link
Contributor

@healerz healerz Aug 13, 2021

Choose a reason for hiding this comment

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

Basically, yes ^^ To me, service providers are application-specific glue code and therefore shouldn't be provided by libraries.
In the core project, we just made the experience that sooner or later the service providers provided by the libraries don't suit the application needs at some point, either because of different dependencies for the same interface, because of the introduction of either an adapter or a decorator or because you want to re-use an already registered service, etc. Another benefit imo is that if you don't use the provided service provider it's easier to see how the application is "glued" together. And on top of that, we could get rid of the additional dependencies in the libraries.

But I guess this is highly opinionated 🙂 . So I'm not saying we must get rid of the service providers in the libraries since everyone is free to not use them and I agree your solution provided with this PR is an improvement. Especially due to moving the "external" dependencies to the dev-dependencies 👍

So either way is fine to me, just quickly wanted to raise the thought. One suggestion I have, though, is that we should use PSR-11 instead of pimple, that way, it's at least container implementation agnostic.

$container[self::REQUEST_FACTORY] = static function (): RequestFactoryInterface {
return new Psr17Factory();
};
}

if (false === isset($container[self::CLIENT])) {
$container[self::CLIENT] = static function (Container $container) {
if (false === isset($container[self::CLIENT]) && class_exists('Buzz\Client\Curl')) {
$container[self::CLIENT] = static function (Container $container): ClientInterface {
return new Curl($container[self::REQUEST_FACTORY]);
};
}

if (false === isset($container[self::ERROR_HANDLER])) {
$container[self::ERROR_HANDLER] = static function () {
$container[self::ERROR_HANDLER] = static function (): ErrorHandlerInterface {
return new ErrorHandler();
};
}

if (false === isset($container[self::HTTP_CLIENT])) {
$container[self::HTTP_CLIENT] = static function (Container $container) {
$container[self::HTTP_CLIENT] = static function (Container $container): HttpClientInterface {
/** @var ClientInterface $client */
$client = $container[self::CLIENT];

Expand All @@ -73,7 +76,9 @@ public function register(Container $container): void
}

if (false === isset($container[self::API_CLIENT])) {
$container[self::API_CLIENT] = static function (Container $container) {
$container[self::API_CLIENT] = static function (
Container $container
): KafkaSchemaRegistryApiClientInterface {
/** @var HttpClient $client */
$client = $container[self::HTTP_CLIENT];

Expand All @@ -84,7 +89,6 @@ public function register(Container $container): void

private function checkRequiredOffsets(Container $container): void
{

if (false === isset($container[self::CONTAINER_KEY][self::SETTING_KEY_BASE_URL])) {
throw new LogicException(
sprintf(
Expand Down
37 changes: 37 additions & 0 deletions tests/KafkaSchemaRegistryApiClientProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
use Jobcloud\Kafka\SchemaRegistryClient\KafkaSchemaRegistryApiClientInterface;
use Jobcloud\Kafka\SchemaRegistryClient\ServiceProvider\KafkaSchemaRegistryApiClientProvider;
use LogicException;
use phpmock\phpunit\MockObjectProxy;
use phpmock\phpunit\PHPMock;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use Pimple\Container;
use Psr\Http\Message\RequestFactoryInterface;
Expand All @@ -16,10 +19,32 @@
*/
class KafkaSchemaRegistryApiClientProviderTest extends TestCase
{
use PHPMock;
use ReflectionAccessTrait;

/**
* @var MockObject|MockObjectProxy
*/
private $classExistsMock;

protected function setUp(): void
{
parent::setUp(); // TODO: Change the autogenerated stub

$this->classExistsMock = $this->getFunctionMock(
'Jobcloud\Kafka\SchemaRegistryClient\ServiceProvider',
'class_exists'
);
}


public function testDefaultContainersAndServicesSetWithMinimalConfig(): void
{
$this->classExistsMock->expects(self::exactly(2))->withConsecutive(
['Nyholm\Psr7\Factory\Psr17Factory'],
['Buzz\Client\Curl']
)->willReturn(true);

$container = new Container();

self::assertArrayNotHasKey('kafka.schema.registry', $container);
Expand Down Expand Up @@ -62,6 +87,11 @@ public function testDefaultContainersAndServicesSetWithMinimalConfig(): void

public function testSuccessWithMissingAuth(): void
{
$this->classExistsMock->expects(self::exactly(2))->withConsecutive(
['Nyholm\Psr7\Factory\Psr17Factory'],
['Buzz\Client\Curl']
)->willReturn(true);

$container = new Container();

$container['kafka.schema.registry'] = [
Expand All @@ -83,6 +113,8 @@ public function testSuccessWithMissingAuth(): void

public function testFailOnMissingBaseUrlInContainer(): void
{
$this->classExistsMock->expects(self::never());

$container = new Container();

$container['kafka.schema.registry'] = [
Expand All @@ -98,6 +130,11 @@ public function testFailOnMissingBaseUrlInContainer(): void

public function testUserNameAndPasswordFromSettingsArePassedToHttpClient(): void
{
$this->classExistsMock->expects(self::exactly(2))->withConsecutive(
['Nyholm\Psr7\Factory\Psr17Factory'],
['Buzz\Client\Curl']
)->willReturn(true);

$container = new Container();

$container['kafka.schema.registry'] = [
Expand Down