Catch exceptions; return false

[BrianHenryIE]

PHP's file_get_contents() just returns false when a file cannot be found.

$result = @file_get_contents('doesnotexist.txt'); 
echo $result === false ? 'false' : $result;

false

I've been using this to wrap League\Flysystem\Local\LocalFilesystemAdapter which throws exceptions when trying to read a file that does not exits.

League\Flysystem\UnableToReadFile : Unable to read file from location: /doesnotexist.txt. file does not exist

The test is currently failing with:

Failed asserting that '' is identical to false.

Any ideas why that might be?

I caught Throwable rather than UnableToReadFile because I don't know all exceptions that might be thrown, and it seems consistent with the existing code.

[elazar]

@BrianHenryIE My apologies for the delayed response; I've had a lot of personal stuff going on lately.

Your issue

stream_open() isn't behaving correctly: it always returns true rather than verifying that the file exists and returningfalse if it doesn't (i.e. in the same way an fopen() call would respond if the file didn't exist).

My patch

If you apply the patch below to your branch, I think it should get your added test passing.

diff --git a/src/StreamWrapper.php b/src/StreamWrapper.php
index 1429287..9d54992 100644
--- a/src/StreamWrapper.php
+++ b/src/StreamWrapper.php
@@ -258,6 +258,17 @@ class StreamWrapper
         $this->log('info', __METHOD__, func_get_args());
         $this->path = $path;
         $this->mode = $mode;
+
+        // Attempt to open for reading if mode is read or read/write
+        if (strpbrk($mode, 'r+') !== false) {
+            try {
+                $this->openRead();
+            } catch (\Throwable $e) {
+                $this->log('error', __METHOD__, ['exception' => $e]);
+                return false;
+            }
+        }
+
         return true;
     }
 
@@ -273,6 +284,9 @@ class StreamWrapper
             $this->openRead();
             return stream_get_contents($this->read, $count);
         } catch (Throwable $e) {
+            $this->log('error', __METHOD__, func_get_args() + [
+                'exception' => $e,
+            ]);
             return false;
         }
     }
@@ -310,6 +324,9 @@ class StreamWrapper
             $this->openRead();
             return fstat($this->read);
         } catch (Throwable $e) {
+            $this->log('error', __METHOD__, func_get_args() + [
+                'exception' => $e,
+            ]);
             return false;
         }
     }
diff --git a/tests/StreamWrapperTest.php b/tests/StreamWrapperTest.php
index 2a56fe7..203565f 100644
--- a/tests/StreamWrapperTest.php
+++ b/tests/StreamWrapperTest.php
@@ -119,6 +119,8 @@ it('can handle writes that force a buffer flush', function () {
 });
 
 it('can acquire multiple shared locks', function () {
+    touch('fly://foo');
+
     $stream1 = fopen('fly://foo', 'r');
     $result = flock($stream1, LOCK_SH);
     expect($result)->toBeTrue();
@@ -148,6 +150,8 @@ it('cannot acquire multiple exclusive locks', function () {
 });
 
 it('cannot acquire an exclusive lock with existing locks', function () {
+    touch('fly://foo');
+
     $stream1 = fopen('fly://foo', 'r');
     $result = flock($stream1, LOCK_SH);
     expect($result)->toBeTrue();
@@ -278,9 +282,8 @@ it('can read and write to a Flysystem filesystem', function () {
     expect($actual)->toBe($expected);
 });
 
-it('it returns false for file_get_contents missing file', function () {
-
-    $actual = file_get_contents("fly://doesnotexist.txt");
+it('fails attempting to read a missing file', function () {
+    $actual = @file_get_contents("fly://doesnotexist.txt");
 
     expect($actual)->toBe(false);
 });

Changes to locking tests

Fixing the behavior of stream_open() caused some tests related to locking to start failing.

These tests expected a file to exist, but weren't creating it. As written, they were reliant on the previous behavior of stream_open() to effectively fail silently.

To address this, I added touch() calls to those tests to create the relevant file.

Changes to your test logic

Calling file_get_contents() with a path that doesn't exist results in both false being returned and a PHP warning being emitted.

PHPUnit and Pest normally handle the emission of a warning by failing the test, which isn't the desired behavior in this case.

To allow execution of the test logic to continue, but suppress the warning, I applied the error control operator (i.e. @) to the file_get_contents() call in the test.

Change to your test description

I modified your original test description to be more consistent with other tests in the same suite. Specifically, they conventionally use a high-level description of the filesystem functionality being tested and avoid referencing specific functions.

Additional error logging

I took the liberty of including some additional error logging when a failure occurs in stream_read() or stream_stat() to help better diagnose these sorts of issues moving forward.

Next steps

Let me know what you think of this patch; if you see any issues with it, please let me know and we can discuss the matter further.

If you are amendable to these changes as they are and apply them to your branch, I can merge this PR and cut a 0.x release.

I can also take care of porting this fix to the master branch once we have it finalized.

[BrianHenryIE]

Nicely explained, thank you.

The way you shared the patch was great. I copied it into a file in PhpStorm and it gave a great UI to view it – but no merge button (so I had to manually type git apply diff.diff)!

No need to apologise, I take years to close some issues.

[elazar]

@BrianHenryIE Just a quick follow-up: 0.6.0 and 1.40 releases are tagged and pushed.

Any chance you'll be moving off of PHP 7.4 anytime soon?

• Per this page, 8.0 and 7.4 have been unsupported for over a year and two years now respectively.
• I'm planning to drop this project's support for 8.1 when official support for it ends at the end of this year.
• I'm happy to continue accepting patches for the 0.x branch in the short term, but in the longer term, I'd rather focus my efforts on one branch.