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.

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

Jump to bottom

Corrections for PR 95 #96

Merged
merged 10 commits into from
Sep 20, 2023
Merged

Corrections for PR 95 #96

Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d84fbbb
Corrections for PR number 95
arogachev Sep 18, 2023
03b391f
Fix Psalm
arogachev Sep 18, 2023
89fa63b
Refactor
arogachev Sep 18, 2023
9311963
More refactor
arogachev Sep 18, 2023
c39206e
Partial revert
arogachev Sep 18, 2023
f153c62
Move write to stream logic to a separate method
arogachev Sep 18, 2023
1560037
Use a dedicated function for getting socket protocols
arogachev Sep 19, 2023
ce96593
Review fix related with requiring sockets php extension
arogachev Sep 19, 2023
2b398b2
Use less variables (code review fix)
arogachev Sep 19, 2023
6019929
Revert socket protocols
arogachev Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
82 changes: 47 additions & 35 deletions src/Handler/StreamHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,28 @@
use function is_resource;
use function is_string;

/**
* Uses stream ({@link https://www.php.net/manual/en/intro.stream.php} for writing variable's data. Requires "sockets"
xepozz marked this conversation as resolved.
Show resolved Hide resolved
* PHP extension.
*/
final class StreamHandler implements HandlerInterface
{
/**
* @var callable|null
*/
private $encoder = null;
private mixed $encoder = null;
/**
* @var resource|Socket|null
* @psalm-suppress PropertyNotSetInConstructor
*/
private $stream = null;
private mixed $stream = null;

/**
* @var resource|Socket|string|null
* @var resource|Socket|string
*/
private $uri = null;
private mixed $uri;

private const SOCKET_PROTOCOLS = ['udp', 'udg', 'tcp', 'unix'];

/**
* @param mixed|resource|string $uri
Expand All @@ -48,6 +55,14 @@
$this->uri = $uri;
}

public function __destruct()
{
if (!is_string($this->uri) || !is_resource($this->stream)) {
return;
}
fclose($this->stream);
}

/**
* Encodes {@param $variable} with {@see self::$encoder} and sends the result to the stream.
*/
Expand All @@ -67,22 +82,10 @@
$this->initializeStream();
}

if ($this->stream instanceof Socket) {
socket_write($this->stream, $data, strlen($data));
return;
}

/** @psalm-suppress PossiblyNullArgument */
if (@fwrite($this->stream, $data) === false) {
if (!$this->writeToStream($data)) {
$this->initializeStream();

if ($this->stream instanceof Socket) {
socket_write($this->stream, $data, strlen($data));
return;
}

/** @psalm-suppress PossiblyNullArgument */
if (@fwrite($this->stream, $data) === false) {
if (!$this->writeToStream($data)) {
throw new RuntimeException('Cannot write a stream.');
}
}
Expand All @@ -97,31 +100,40 @@

private function initializeStream(): void
{
if (is_string($this->uri)) {
if (
str_starts_with($this->uri, 'udp://') ||
str_starts_with($this->uri, 'udg://') ||
str_starts_with($this->uri, 'tcp://') ||
str_starts_with($this->uri, 'unix://')
) {
$this->stream = fsockopen($this->uri);
} else {
$this->stream = fopen($this->uri, 'wb+');
}
if (!is_string($this->uri)) {
xepozz marked this conversation as resolved.
Show resolved Hide resolved
$stream = $this->uri;
} else {
$this->stream = $this->uri;
$hasSocketProtocol = false;
foreach (self::SOCKET_PROTOCOLS as $protocol) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So then it's better to call https://www.php.net/manual/en/function.stream-get-transports.php

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. But the list returned by this function is bigger:

[
    "tcp",
    "udp",
    "unix",
    "udg",
    "ssl",
    "tls",
    "tlsv1.0",
    "tlsv1.1",
    "tlsv1.2",
    "tlsv1.3",
]

if (str_starts_with($this->uri, "$protocol://")) {
$hasSocketProtocol = true;

break;
}
}

$stream = $hasSocketProtocol ? fsockopen($this->uri) : fopen($this->uri, 'wb+');
}

if (!is_resource($this->stream) && !$this->stream instanceof Socket) {
if (!is_resource($stream) && !$stream instanceof Socket) {
throw new RuntimeException('Cannot initialize a stream.');
}

$this->stream = $stream;
}

public function __destruct()
private function writeToStream(string $data): bool
{
if (!is_string($this->uri) || !is_resource($this->stream)) {
return;
if ($this->stream === null) {
return false;

Check warning on line 128 in src/Handler/StreamHandler.php

View check run for this annotation

Codecov / codecov/patch

src/Handler/StreamHandler.php#L128 

Added line #L128 was not covered by tests
}
fclose($this->stream);

if ($this->stream instanceof Socket) {
socket_write($this->stream, $data, strlen($data));

return true;
}

return @fwrite($this->stream, $data) !== false;
}
}