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

Use zval storage for Stream instances created from strings #230

Merged
merged 1 commit into from
Apr 20, 2023
Merged

Use zval storage for Stream instances created from strings #230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Use zval storage for Stream instances created from strings
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Apr 20, 2023
commit 2f55e3d28df6abf3f489158a54f2df2e2a77fc83
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
},
"extra": {
"branch-alias": {
"dev-master": "1.6-dev"
"dev-master": "1.7-dev"
}
}
}
105 changes: 101 additions & 4 deletions src/Stream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,7 @@ public static function create($body = ''): StreamInterface
}

if (\is_string($body)) {
$resource = \fopen('php://temp', 'rw+');
\fwrite($resource, $body);
\fseek($resource, 0);
$body = $resource;
$body = self::openZvalStream($body);
}

if (!\is_resource($body)) {
Expand Down Expand Up @@ -284,4 +281,104 @@ public function getMetadata($key = null)

return $meta[$key] ?? null;
}

private static function openZvalStream(string $body)
{
static $wrapper;

$wrapper ?? \stream_wrapper_register('Nyholm-Psr7-Zval', $wrapper = \get_class(new class() {
Copy link

Choose a reason for hiding this comment

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

Why the wrapper is registered twice - stream_wrapper_register?

Also any reason to not use ??= here / or regular if to improve the code readibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not registered twice, where do you read that? About ??=, PHP 7.1 rulez. For the "if", I find the current style more readable, because less indentation.

Copy link

Choose a reason for hiding this comment

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

on l318 and then l407, has the 2nd register on l407 any coverage and how it is guaranteed the wrapped is not registered when the 1st fopen fails? I would say the if on l406 is not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That "if" guards against unwanted calls to stream_wrapper_unregister('Nyholm-Psr7-Zval').

Copy link

Choose a reason for hiding this comment

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

I would personally prefer to not do it as any unrelated code MUST never touch wrappers that are not owned by the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer keeping it, adding some guards against global scope manipulations.

public $context;

private $data;
private $position = 0;

public function stream_open(): bool
{
$this->data = \stream_context_get_options($this->context)['Nyholm-Psr7-Zval']['data'];
\stream_context_set_option($this->context, 'Nyholm-Psr7-Zval', 'data', null);

return true;
}

public function stream_read(int $count): string
{
$result = \substr($this->data, $this->position, $count);
$this->position += \strlen($result);

return $result;
}

public function stream_write(string $data): int
{
$this->data = \substr_replace($this->data, $data, $this->position, \strlen($data));
$this->position += \strlen($data);

return \strlen($data);
}

public function stream_tell(): int
{
return $this->position;
}

public function stream_eof(): bool
{
return \strlen($this->data) <= $this->position;
}

public function stream_stat(): array
{
return [
'mode' => 33206, // POSIX_S_IFREG | 0666
'nlink' => 1,
'rdev' => -1,
'size' => \strlen($this->data),
'blksize' => -1,
'blocks' => -1,
];
}

public function stream_seek(int $offset, int $whence): bool
{
if (\SEEK_SET === $whence && (0 <= $offset && \strlen($this->data) >= $offset)) {
$this->position = $offset;
} elseif (\SEEK_CUR === $whence && 0 <= $offset) {
$this->position += $offset;
} elseif (\SEEK_END === $whence && (0 > $offset && 0 <= $offset = \strlen($this->data) + $offset)) {
$this->position = $offset;
} else {
return false;
}

return true;
}

public function stream_set_option(): bool
{
return true;
}

public function stream_truncate(int $new_size): bool
{
if ($new_size) {
Copy link

Choose a reason for hiding this comment

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

the if condition and else should not be needed

Copy link
Collaborator Author

@nicolas-grekas nicolas-grekas Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it's a small optimization

Copy link

Choose a reason for hiding this comment

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

I belive substr(expr, 0, 0) is optimized internally to return interned ''.

Copy link
Collaborator Author

@nicolas-grekas nicolas-grekas Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the optimization is about removing the need to call any functions :)

$this->data = \substr($this->data, 0, $new_size);
$this->position = \min($this->position, $new_size);
} else {
$this->data = '';
$this->position = 0;
}

return true;
}
}));

$context = \stream_context_create(['Nyholm-Psr7-Zval' => ['data' => $body]]);

if (!$stream = @\fopen('Nyholm-Psr7-Zval://', 'r+', false, $context)) {
\stream_wrapper_register('Nyholm-Psr7-Zval', $wrapper);
$stream = \fopen('Nyholm-Psr7-Zval://', 'r+', false, $context);
}

return $stream;
}
}
2 changes: 1 addition & 1 deletion tests/StreamTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public function testConstructorSeekWithStringContent()
$this->assertTrue($stream->isReadable());
$this->assertTrue($stream->isWritable());
$this->assertTrue($stream->isSeekable());
$this->assertEquals('php://temp', $stream->getMetadata('uri'));
$this->assertEquals('Nyholm-Psr7-Zval://', $stream->getMetadata('uri'));
$this->assertTrue(\is_array($stream->getMetadata()));
$this->assertSame(5, $stream->getSize());
$this->assertFalse($stream->eof());
Expand Down