Encoding #5
Encoding #5
Conversation
| @@ -22,6 +22,8 @@ branches: | |||
| - /^analysis-.*$/ | |||
|
|
|||
| matrix: | |||
| allow_failures: | |||
| - php: hhvm | |||
There was a problem hiding this comment.
we could try to detect hhvm and trigger a skip just for the tests that don't work with hhvm rather than ignore hhvm issues over the whole package.
sagikazarmark
force-pushed
the
encoding
branch
from
0df243e
to
c7fcd53
Compare
| @@ -18,6 +18,8 @@ | |||
| "require-dev": { | |||
| "zendframework/zend-diactoros": "^1.0", | |||
| "guzzlehttp/psr7": "^1.0", | |||
| "ext-zlib": "*", | |||
| "clue/stream-filter": "^1.3", | |||
There was a problem hiding this comment.
It should be in the require part, otherwise encoding does not work
There was a problem hiding this comment.
lets add these to suggest, and explain for what case one would want to use them in a project.
sagikazarmark
force-pushed
the
encoding
branch
4 times, most recently
from
cfffd0d
to
82b4bde
Compare
sagikazarmark
force-pushed
the
encoding
branch
from
82b4bde
to
7f7dd42
Compare
|
|
||
| public function getReadFilter() | ||
| { | ||
| return 'zlib.deflate'; |
There was a problem hiding this comment.
Should we throw an exception when zlib is not found, but this stream is being used?
There was a problem hiding this comment.
good idea. with the optional dependency, this will at least explain what is going on. the exception message should say that you need the ext.zlib in your project if you want to use this.
sagikazarmark
force-pushed
the
encoding
branch
2 times, most recently
from
8275f80
to
a2a1310
Compare
|
I had to add ignore platform deps hack to the build. As soon as we drop 5.4, it can be removed. |
|
I think this is ready to be merged. |
| @@ -25,7 +24,9 @@ matrix: | |||
| fast_finish: true | |||
| include: | |||
| - php: 5.4 | |||
| env: COMPOSER_FLAGS="--prefer-stable --prefer-lowest" COVERAGE=true TEST_COMMAND="composer test-ci" | |||
| env: COMPOSER_FLAGS="--ignore-platform-reqs" | |||
There was a problem hiding this comment.
is this because of the stream-filter library? if so i would move it to require-dev and suggest. if we support php 5.4, we should not fail with weird errors or tell people to ignore the platform and then they are surprised when trying to use streams.
for travis, we will need to keep the flag, as we do need the dev dependencies.
There was a problem hiding this comment.
No, it is required for php-mock, which I used to mock extension_loaded function to test exceptions thrown.
sagikazarmark
force-pushed
the
encoding
branch
4 times, most recently
from
12e183f
to
6518ee5
Compare
|
Replaced the php-mock dependency with a custom solution. |
| { | ||
| public function __construct(StreamInterface $stream, $level = -1) | ||
| { | ||
| parent::__construct($stream, ['window' => -15], ['window' => -15, 'level' => $level]); |
There was a problem hiding this comment.
does this not need a check for zlib as well?
|
apart from the zlib check on InflateStream, this looks ready to merge. not sure if you want to squash the commits or not. |
sagikazarmark
force-pushed
the
encoding
branch
from
dbff437
to
4b07a1f
Compare
Add stream behavior Improve zlib tests Remove php-mock dependency Add zlib extension check to InflateStream
sagikazarmark
force-pushed
the
encoding
branch
from
4b07a1f
to
827ed4e
Compare
|
No squashing is needed IMO as these commits rather make sense to be preserved. I squashed some which were related to the zlib check. |
see php-http/httplug#98