Raise exception on write() if stream is not writeable #15

mtymek · 2015-05-24T22:31:07Z

Fixes #13.

I had to change how isWritable does the check, as is_writable($meta['uri']) is unreliable for TEMP and MEMORY streams.

Ocramius · 2015-05-25T07:30:57Z

src/Stream.php

-        return is_writable($meta['uri']);
+        $mode = $meta['mode'];
+
+        return (strstr($mode, 'w') || strstr($mode, '+'));


What are all the possible valid $mode strings? Can w be before +?

Answer is complex, depending on your definition of "valid". If by "valid" you mean everything that PHP accepts, then:

for TEMP and MEMORY streams, mode is normalized, you always get rb or w+b

for files PHP will accept even things like this: $f2 = fopen('/tmp/test', 'ah()+');, and report them under mode

So, apparently in order to satisfy fopen docs, we have to check if any of characters +, a, c, w, x is present in mode. If you agree, I can post fix in a moment.

Yes, w can be before +. Not sure why you ask though.

weierophinney · 2015-05-25T18:56:06Z

src/Stream.php

@@ -60,7 +60,7 @@ public function __construct($stream, $mode = 'r')
    public function __toString()
    {
        if (! $this->isReadable()) {
-            return '';
+            throw new RuntimeException('Stream is not readable');


Actually, we cannot throw exceptions here; see the PHP manual. The correct behavior here is to return an empty string.

Uhm, that's what I asked about in #16 :)

I know; it was seeing it in the context of the method that made me remember why I wasn't throwing an exception previously here. Since the exception is non-recoverable, we must cast to an empty string instead.

Ah, now I remember the horror of debugging ZF1's view helper throwing exception...

mtymek · 2015-05-25T20:54:26Z

Updated!

Ocramius · 2015-05-26T10:09:11Z

src/Stream.php

+            strstr($mode, 'x')
+            || strstr($mode, 'w')
+            || strstr($mode, 'c')
+            || strstr($mode, 'a')


All these modes need to be checked in the tests - consider using a data-provider

mtymek · 2015-05-26T11:39:27Z

Added tests for all modes that make sense; did the same for isReadable.

Ocramius · 2015-05-26T11:42:03Z

Thanks @mtymek

Ocramius · 2015-05-26T11:48:50Z

test/StreamTest.php

+    private function findNonExistentTempName()
+    {
+        while (true) {
+            $tmpnam = sys_get_temp_dir() . DIRECTORY_SEPARATOR . 'diac' . uniqid();


There is tempnam for this

See docs - tempnam creates new file. If you open it with x mode, you get a warning:

PHP Warning: fopen(/tmp/zrazAZD39N): failed to open stream: File exists in phar:///home/mat/bin/boris.phar/lib/Boris/EvalWorker.php(133) : eval()'d code on line 1

good point, didn't know :)

Me neither, I realized this when my tests started to fail :-)

…into develop Close #15 Forward port #15

Ocramius · 2015-05-26T12:04:20Z

@mtymek merged, thanks!

master: d7d7751
develop: 4718cf4

Raise exception on write() if stream is not writeable

51b2061

Ocramius reviewed May 25, 2015
View reviewed changes

Ocramius added the bug label May 25, 2015

Ocramius added this to the 1.0.1 milestone May 25, 2015

Ocramius self-assigned this May 25, 2015

mtymek force-pushed the throw_exception_write branch from d9ea433 to 8a6427a Compare May 25, 2015 11:22

weierophinney reviewed May 25, 2015
View reviewed changes

correct valid modes for writable stream

9088e59

mtymek force-pushed the throw_exception_write branch from 71f512f to 9088e59 Compare May 25, 2015 20:48

Rise exception in getContents

1021fbc

Ocramius reviewed May 26, 2015
View reviewed changes

Ocramius mentioned this pull request May 26, 2015

Raise exception on write() if stream is not writeable #13

Closed

Check all valid modes

206778b

mtymek force-pushed the throw_exception_write branch from 1f79766 to 206778b Compare May 26, 2015 11:42

Ocramius reviewed May 26, 2015
View reviewed changes

Ocramius closed this in d7d7751 May 26, 2015

Ocramius added a commit that referenced this pull request May 26, 2015

Merge branch 'hotfix/#15-raise-exceptions-if-stream-is-not-writable' …

4718cf4

…into develop Close #15 Forward port #15

mtymek deleted the throw_exception_write branch January 27, 2016 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise exception on write() if stream is not writeable #15

Raise exception on write() if stream is not writeable #15

mtymek commented May 24, 2015

Ocramius May 25, 2015

mtymek May 25, 2015

weierophinney May 25, 2015

mtymek May 25, 2015

weierophinney May 25, 2015

mtymek May 25, 2015

mtymek commented May 25, 2015

Ocramius May 26, 2015

mtymek commented May 26, 2015

Ocramius commented May 26, 2015

Ocramius May 26, 2015

mtymek May 26, 2015

Ocramius May 26, 2015

mtymek May 26, 2015

Ocramius commented May 26, 2015

Raise exception on write() if stream is not writeable #15

Raise exception on write() if stream is not writeable #15

Conversation

mtymek commented May 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtymek commented May 25, 2015

Choose a reason for hiding this comment

mtymek commented May 26, 2015

Ocramius commented May 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented May 26, 2015