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

Support react/stream from 0.6 to ^1 #8

Closed
wants to merge 3 commits into from
Closed

Conversation

dontub
Copy link

@dontub dontub commented Mar 21, 2019

This PR adds support for current versions of react/stream.

src/Decoder.php Outdated
@@ -199,7 +200,7 @@ private function readHeader($header)
$record = unpack($this->format, $header);

// we only support "ustar" format (for now?)
if ($record['magic'] !== 'ustar') {
if (rtrim($record['magic']) !== 'ustar') {
Copy link
Author

Choose a reason for hiding this comment

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

The rtrim is required for a tar created with GNU tar 1.32.

Copy link
Owner

Choose a reason for hiding this comment

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

This sounds unrelated and should probably be discussed in a separate PR, see also #7 and #3.

@nazar-pc
Copy link

Wow, I guess this is the response to issues I've posted earlier today. I have done exactly the same tweaks myself (except rtrim, I incorrectly used just trim) and I can confirm it worked for me perfectly.
Thanks, @dontub! I hope to see this PR merged and released soon.

@dontub
Copy link
Author

dontub commented May 24, 2019

@clue Do you think this can be merged?

Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

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

@dontub Thank you very much for looking into this and filing this PR!

I'm currently busy with a few other projects and will get back to this as soon as time permits. I think your changes are an excellent starting point and I will look into this again in more detail then. In the meantime, here are a couple of quick remarks.

src/Decoder.php Outdated
@@ -199,7 +200,7 @@ private function readHeader($header)
$record = unpack($this->format, $header);

// we only support "ustar" format (for now?)
if ($record['magic'] !== 'ustar') {
if (rtrim($record['magic']) !== 'ustar') {
Copy link
Owner

Choose a reason for hiding this comment

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

This sounds unrelated and should probably be discussed in a separate PR, see also #7 and #3.

@@ -27,7 +25,7 @@
echo 'Received entry headers:' . PHP_EOL;
var_dump($header);

BufferedSink::createPromise($file)->then(function ($contents) {
$file->on('data', function ($contents) {
Copy link
Owner

Choose a reason for hiding this comment

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

This will now print this message for each chunk of data whereas the old version buffered the whole file. Possible alternative: https://github.com/reactphp/promise-stream#buffer

@dontub
Copy link
Author

dontub commented Jun 28, 2019

@clue Thanks for your initial feedback. I hope you can get some time to continue...

@bartvanhoutte
Copy link

Didn't see this PR and worked on this today as well, my code is very similar to this PR. Good work 👌

@clue
Copy link
Owner

clue commented Sep 7, 2019

@dontub Thank you for keeping up with this and for you patience! I've just squashed your changes with #11 and added some additional adjustments to strictly follow stream semantics, now let's get this shipped! :shipit:

@clue clue closed this Sep 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants