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

#1836 count amount of bytes read instead of chunks #1846

Merged
merged 6 commits into from
Apr 13, 2016
Merged

Conversation

llvdl
Copy link
Contributor

@llvdl llvdl commented Apr 12, 2016

No description provided.

@llvdl
Copy link
Contributor Author

llvdl commented Apr 12, 2016

This is a fix for #1836 where not all data is read if StreamInterface::read returns less than the request amount of bytes.

@akrabat akrabat added this to the 3.4.0 milestone Apr 13, 2016
@akrabat
Copy link
Member

akrabat commented Apr 13, 2016

Unit tests fail in testRespondWithPaddedStreamFilterOutput.

@akrabat
Copy link
Member

akrabat commented Apr 13, 2016

Suggested fix:

diff --git a/Slim/App.php b/Slim/App.php
index cd478f1..e65c6e4 100644
--- a/Slim/App.php
+++ b/Slim/App.php
@@ -386,6 +386,9 @@ class App
             if (isset($contentLength)) {
                 $amountToRead = $contentLength;
                 while ($amountToRead > 0 && !$body->eof()) {
+                    if ($amountToRead < $chunkSize) {
+                        $chunkSize = $amountToRead;
+                    }
                     $data = $body->read($chunkSize);
                     echo $data;

@llvdl
Copy link
Contributor Author

llvdl commented Apr 13, 2016

The unit tests run without problems on my local environment (Xubuntu 15.10, PHP 5.6.11), but fail on Travis.

If you could help me reproduce the failing test locally, then I would appreciat it. I can probably get on IRC tonigh (CET).

@akrabat
Copy link
Member

akrabat commented Apr 13, 2016

You need to install the mcrypt PHP extension locally as the test that's failing only runs if that's installed.

@llvdl
Copy link
Contributor Author

llvdl commented Apr 13, 2016

Thanks!

@llvdl
Copy link
Contributor Author

llvdl commented Apr 13, 2016

Hmm.. installed php5-mcrypt, but the test still succeeds.

@akrabat
Copy link
Member

akrabat commented Apr 13, 2016

Can you see mcrypt in the list when you run php -m ?

@llvdl
Copy link
Contributor Author

llvdl commented Apr 13, 2016

mcrypt does not show up, I will investigate why.

The fix you suggested seems to do the trick! Thanks for looking into this.

@akrabat akrabat merged commit 0ef4677 into slimphp:3.x Apr 13, 2016
akrabat added a commit that referenced this pull request Apr 13, 2016
@akrabat
Copy link
Member

akrabat commented Apr 13, 2016

Thanks!

@llvdl llvdl deleted the 1836 branch May 12, 2016 17:56
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.

2 participants