Skip to content
This repository has been archived by the owner on May 19, 2021. It is now read-only.

Fix reading with any buffer size #9

Closed
wants to merge 4 commits into from

Conversation

clue
Copy link
Contributor

@clue clue commented Aug 7, 2013

Fixes #7 and #8.

@ghost
Copy link

ghost commented Aug 7, 2013

Some quick testing revealed that this loop could read forever if the file is empty or the end is reached.

On PHP 5.4.9, fread() returns an empty string on EOF.

I suggest adding an feof() check to the while loop, and keeping the strlen() check after the loop.

@clue
Copy link
Contributor Author

clue commented Aug 7, 2013

[..] this loop could read forever if the file is empty or the end is reached

Indeed, obviously we need some additional tests? :) Adjusted the loop to properly handle EOF

I suggest adding [...]

Fixed it slightly different and imho more concise. I'm planning to embed the extractor into every phar I'm producing, so its file size will be critical and I'd love to see it as concise as possible (see clue/phar-composer#8).

@ghost
Copy link

ghost commented Aug 7, 2013

I'll be closing this pull request in favor of commit 26fedf8.

The error handling is there to prevent the class from misinterpreting the data it has read (specifically, the manifest in particular), in addition to providing better debugging information for when things do go wrong. The original extraction class provided by the phar extension didn't take any of this into consideration. It merely assumed that things would always work as intended. While it may be true in the case of the phar extension generating the default stub, it unfortunately is not for our case.

I am all for saving space where necessary, but I don't believe it is warranted in this particular case.

@ghost ghost closed this Aug 7, 2013
@clue
Copy link
Contributor Author

clue commented Aug 7, 2013

I'll be closing this pull request in favor of commit 26fedf8.

Yeah, valid reasoning. The commit looks good to me, I'll confirm tomorrow.

@clue
Copy link
Contributor Author

clue commented Aug 12, 2013

Confirmed to work without ext-phar, good job 👍

@ghost
Copy link

ghost commented Aug 12, 2013

Awesome!

@clue clue deleted the fix-extract-read branch March 14, 2014 09:08
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running (extracting) phar archives without ext-phar fails
1 participant