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.

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

[+] Guzzle Transpost #68

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

demyashev
Copy link
Contributor

Edits:

  • Added Hawk\Transport\GuzzleTransport client
  • Hawk\Transport\TransportInterface transform to Hawk\Transport class
    • repeating parameters url and timeout from child classes
    • required parent class with factory init function
  • Added Hawk\Transport::init factory
  • New property Hawk\Options::$transport (curl or guzzle, default curl)
  • New Hawk\Exception\TransportException
  • PHPUnit tests for default and custom Hawk\Options options timeout, transport

How to pass new options:

Hawk\Catcher::init([
    'integrationToken' => '<secret>',
    'timeout' => 33,
    'transport' => 'guzzle', # curl or guzzle
]);

PHPUnit results:

Time: 30 ms, Memory: 6.00 MB
OK (21 tests, 34 assertions)

@demyashev demyashev requested a review from neSpecc as a code owner December 5, 2024 23:11
@@ -91,6 +97,7 @@ private function setOption(string $key, $value): void
case 'integrationToken':
case 'release':
case 'url':
case 'transport':
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to add a separate case for transport validation to restrict it only for allowed values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@neSpecc Hawk\Transport::init have validation

public static function init(Options $options)
{
    $transports = self::getTransports();

    if (!array_key_exists($options->getTransport(), $transports)) {
        throw new TransportException('Invalid transport specified');
    }

    return new $transports[$options->getTransport()]($options);
}

Comment on lines 28 to 39
public static function init(Options $options)
{
$transports = self::getTransports();

if (!array_key_exists($options->getTransport(), $transports)) {
throw new TransportException('Invalid transport specified');
}

return new $transports[$options->getTransport()]($options);
}

public static function getTransports(): array
Copy link
Member

Choose a reason for hiding this comment

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

docs missed

Comment on lines +18 to +24
/**
* @var string
*/
protected $url;

/**
* @var int
Copy link
Member

Choose a reason for hiding this comment

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

docs missed

Copy link
Member

@neSpecc neSpecc Dec 6, 2024

Choose a reason for hiding this comment

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

docs are needed not only for typings. Add descriptions: why this property or function is used

src/Transport.php Outdated Show resolved Hide resolved
@demyashev
Copy link
Contributor Author

  • PHPDoc comments added
  • Hawk\Transport class transfer to abstract

}

/**
* @inheritDoc
*/
public function send(Event $event): void
protected function _send(Event $event): string
Copy link
Contributor

Choose a reason for hiding this comment

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

_send method violates the PSR-12 recommendations.
It is not recommended to add underscores to method names.

*/
public static function getTransports(): array
{
return [
Copy link
Contributor

Choose a reason for hiding this comment

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

Using a hardcoded static registry in getTransports violates the Open-Closed Principle (OCP). Adding a new transport requires modifying the base class.

$response = $this->_send($event);

try {
$data = json_decode($response, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

$data = json_decode($response, true);
if (json_last_error() !== JSON_ERROR_NONE) {
...
}

*
* @return mixed
*/
public function send(Event $event)
Copy link
Contributor

Choose a reason for hiding this comment

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

public function send(Event $event): ?array

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