gh-1838 added checks for pipe streams

[llvdl]

Fixes #1838

[coveralls]

Coverage increased (+0.07%) to 88.043% when pulling b570cec on llvdl:1838 into 954472c on slimphp:3.x.

[coveralls]

Coverage decreased (-0.06%) to 87.917% when pulling 07410c8 on llvdl:1838 into 954472c on slimphp:3.x.

[coveralls]

Coverage decreased (-0.06%) to 87.917% when pulling 2a61314 on llvdl:1838 into 954472c on slimphp:3.x.

[coveralls]

Coverage decreased (-0.06%) to 87.917% when pulling 4a4c305 on llvdl:1838 into 954472c on slimphp:3.x.

[coveralls]

Coverage increased (+0.2%) to 88.191% when pulling f8d6249 on llvdl:1838 into 954472c on slimphp:3.x.

[ghost]

Since Slim's implementation has methods beyond what's in StreamInterface anyway, I'd add isSeekableForward() and seekForward() for completeness. If one's dealing with pipes, he/she might need them anyway.

    /**
     * Returns whether or not the stream is seekable forward.
     *
     * Note: This method is not part of the PSR-7 standard
     * and is provided for Stream's compatibility with pipes.
     *
     * @return bool
     */
    public function isSeekableForward()
    {
        return $this->isSeekable() || $this->isPipe();
    }

    /**
     * Seek forward in the stream.
     *
     * Note: This method is not part of the PSR-7 standard
     * and is provided for Stream's compatibility with pipes.
     *
     * @param int $offset Stream offset
     * @throws RuntimeException on failure.
     */
    public function seekForward($offset)
    {
        // Note that fseek returns 0 on success!
        if (!$this->isSeekableForward() || fseek($this->stream, $offset, SEEK_CUR) === -1) {
            throw new RuntimeException('Could not seek forward in stream');
        }
    }

I'd also change isSeekable() implementation - there's no need for isPipe() call, I think, as 'seekable' metadata is guaranteed to be 1 in case of seekable handle and something else (typically 0 but undef for pipes) in other cases.

    public function isSeekable()
    {
        if ($this->seekable === null) {
            $this->seekable = false;
            if ($this->isAttached()) {
                $meta = $this->getMetadata();
                $this->seekable = ($meta['seekable'] === 1);
            }
        }

        return $this->seekable;
    }

HTH.

[llvdl]

Thanks for the feedback, @ddalek.

I replaced isSeekable() with your version, but then multiple unit tests fail, so I reverted to the version I submitted before.

[coveralls]

Coverage increased (+0.2%) to 88.158% when pulling 2e615a7 on llvdl:1838 into 954472c on slimphp:3.x.

[ghost]

Argh, I misinterpreted print_r() output. My comparison was unnecessary, the value returned is bool, not int. This will teach me not to rely on print_r(). :)

Still, 'seekable' meta should be false for pipes (despite SEEK_CUR working) and && !$this->isPipe() in isSeekable() adds the cost of fstat() to the first isSeekable() call on verifiably seekable streams. But that's just a tiny perf nitpick (if I'm not talking out of my back again).

[llvdl]

For some reason the tests fail on the hhvm machine: RuntimeException: Could not seek forward in stream is raised when trying to read from the pipe.

[llvdl]

I removed the check for && !$this->isPipe() and the test still pass, so it seems you are right, @ddalek .

[coveralls]

Coverage increased (+0.2%) to 88.158% when pulling 0f3a41f on llvdl:1838 into 954472c on slimphp:3.x.

[ghost]

Cursory search through their issues shows quite a few historical incompatibilities around files and streams, including a new one around fseek() so I'm not terribly surprised. I'll take the time over the weekend to see if there's anything that can and should be done in Slim or if it's HHVM problem.

[akrabat]

Tests fail on HHVM.

[coveralls]

Coverage increased (+0.2%) to 93.785% when pulling 5f7cb0c on llvdl:1838 into 794073a on slimphp:3.x.

[llvdl]

@i created a virtual machine with hhvm and could reproduce the failing tests. The cause was isReadable() returning false in hhvm. I added a unit test and a check for isPipe() and the tests

I removed the seekForward implementation for now, as it doesn't seem to work for pipes in PHP.

Update: the reason isReadable() returned false with hhvm is that the mode is empty when using stream_get_meta_data() on a resource opened with popen():

<?php
$fh = popen('echo 1', 'r');
$meta = stream_get_meta_data($fh);
var_dump($meta['mode']);

stream_get_contents($fh);
pclose($fh);

Result using PHP (version 7.0.4): string(1) "r"
Result using hhvm (version 3.13.1): string(0) ""

Apparently, the mode for the stream cannot be determined using stream_get_meta_data() in hhvm in the case of pipes. For what it's worth, the manual does not mention streams using popen(), so the behaviour of hhvm is technically not incorrect.

Also, although the metadata provided by hhvm using stream_get_meta_data() on a pipe states that the stream is seekable, a fseek($fh, 1, SEEK_CUR) seems to always return an error (-1) and does not change the stream pointer.

With the current behaviour of hhvm, I think it will be hard to get pipes supported by Stream correctly.

Update: I have reported the issue at hhvm: facebook/hhvm#7089

[coveralls]

Coverage increased (+0.2%) to 93.785% when pulling 0199b88 on llvdl:1838 into 794073a on slimphp:3.x.

[akrabat]

Please name this property isPipe.

[llvdl]

That would collide with the isPipe method

[akrabat]

No. PHP can tell the difference between a property and a method with the same name…

[llvdl]

I didn't know that :)

[coveralls]

Coverage increased (+3.002%) to 96.603% when pulling 2e5a54b on llvdl:1838 into 794073a on slimphp:3.x.

[coveralls]

Coverage increased (+3.002%) to 96.603% when pulling 3882722 on llvdl:1838 into 794073a on slimphp:3.x.