-
Notifications
You must be signed in to change notification settings - Fork 0
Improve HTTP Client structure #1
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,29 @@ | ||||||
<?php | ||||||
|
||||||
|
||||||
namespace App\Notification\Client\Guzzle; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
|
||||||
use App\Notification\Client\HttpClientAdapterInterface; | ||||||
use GuzzleHttp\Client; | ||||||
use Psr\Http\Message\ResponseInterface; | ||||||
|
||||||
final class GuzzleHttpClient implements HttpClientAdapterInterface | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems at the moment there is no test for this class |
||||||
{ | ||||||
private Client $client; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
public function __construct() | ||||||
{ | ||||||
$this->client = new Client(); | ||||||
} | ||||||
|
||||||
public function post(string $url, array $params): ResponseInterface | ||||||
{ | ||||||
return $this->client->post( | ||||||
$url, | ||||||
[ | ||||||
'json' => $params | ||||||
] | ||||||
); | ||||||
} | ||||||
} |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
<?php | ||
|
||
|
||
namespace App\Notification\Client; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be more appropriated to have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Otherwise will leave the impression those are NotificationClients instead |
||
|
||
|
||
use Psr\Http\Message\ResponseInterface; | ||
|
||
interface HttpClientAdapterInterface | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if |
||
{ | ||
public function post(string $url, array $params): ResponseInterface; | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5,21 +5,21 @@ | |||||
|
||||||
|
||||||
use App\Notification\AppNotificationInterface; | ||||||
use App\Notification\Client\HTTPClientAdapterInterface; | ||||||
use App\Notification\Client\ResponseAdapterInterface; | ||||||
use App\Notification\Client\HttpClientAdapterInterface; | ||||||
use Psr\Http\Message\ResponseInterface; | ||||||
|
||||||
final class SlackNotification implements AppNotificationInterface | ||||||
{ | ||||||
private SlackStylizedMessageCreator $message; | ||||||
private HTTPClientAdapterInterface $client; | ||||||
private HttpClientAdapterInterface $client; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
public function __construct(HTTPClientAdapterInterface $client, string $message, string $messageType) | ||||||
public function __construct(HttpClientAdapterInterface $client, string $message, string $messageType) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
{ | ||||||
$this->message = new SlackStylizedMessageCreator($message, $messageType); | ||||||
$this->client = $client; | ||||||
} | ||||||
|
||||||
public function notify(): ResponseAdapterInterface | ||||||
public function notify(): ResponseInterface | ||||||
{ | ||||||
return $this->client->post( | ||||||
getenv('SLACK_API_WEBHOOK'), | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,6 @@ | |
"phpunit/phpunit": "8.5.*" | ||
}, | ||
"scripts": { | ||
"tests": "./vendor/bin/phpunit tests --color=always --stop-on-failure", | ||
"tests": "./vendor/bin/phpunit tests --color=always --stop-on-failure" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change doesn't belong to the main intent of this PR. A more appropriated approach would be to split things up.:
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
<?php | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to establish a pattern/consistency regarding the application files structure - tabs vs spaces, how many, blank lines between There a lot of variations it seems. I'd suggest you take a look on https://editorconfig.org |
||
|
||
namespace App\Test\Notification\Fake; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems An approach to get it clear up, is to have a test-subdomain called
and then pointing it out on "autoload-dev": {
"psr-4": {
"App\\Test\\": "tests/app",
"App\\Test\\Support\\": "tests/support"
}
} (perhaps "Test" should be the first term, actually - but this is for another discussion in another time) |
||
|
||
|
||
use App\Notification\Client\HttpClientAdapterInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
|
||
class FakeHttpClient implements HttpClientAdapterInterface | ||
{ | ||
private ResponseInterface $response; | ||
|
||
public function mockResponse(ResponseInterface $response): void | ||
{ | ||
$this->response = $response; | ||
} | ||
|
||
public function post(string $url, array $params): ResponseInterface | ||
{ | ||
return $this->response; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
<?php | ||
|
||
namespace App\Test\Notification\Slack; | ||
|
||
use App\Test\Notification\Fake\FakeHttpClient; | ||
use GuzzleHttp\Psr7\Response; | ||
use PHPUnit\Framework\Assert; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
class FakeNotificationResponseTest extends TestCase | ||
{ | ||
public static function test_FakeHttpClient_ShouldAssertHttpResponse(): void | ||
{ | ||
$notificationResponse = new Response( | ||
$statusCode = 200, | ||
$responseHeader = [], | ||
$responseBody = null, | ||
$protocolVersion = '1.1', | ||
$responseReason = 'OK' | ||
); | ||
|
||
$fakeClient = new FakeHttpClient(); | ||
$fakeClient->mockResponse($notificationResponse); | ||
|
||
$response = $fakeClient->post($fakeUrl = 'https://fake.com', $emptyParams = []); | ||
|
||
Assert::assertEquals($expectedStatusCodeResponse = 200, $response->getStatusCode()); | ||
Assert::assertEquals($expectedPhraseResponse = 'OK', $response->getReasonPhrase()); | ||
} | ||
} |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This namespace doesn't match the file structure.