Update ::url_stat() for directories

[BrianHenryIE]

I don't seem to be able to specify 0.4.0 as the target for the PR.

::directoryExists() is a Flysystem 3.0 function which I call when it's available, otherwise I have a re-implementation.

This is working well in my integration tests for BrianHenryIE/strauss#139

Closes #15 !

[elazar]

@BrianHenryIE I've created a 0.x branch based off the 0.4.0 tag. Can you pull that branch, rebase yours onto that, and then change the target of your PR to that branch?

[BrianHenryIE]

Ok, I was able to do that here in the GitHub UI – I had branched from the 0.4.0. tag earlier, so no need for a rebase per se.

If you're curious what I'm doing – I'm writing a class ReadOnlyFileSystem implements FilesystemOperator which performs first reads from a delegated FilesystemOperator, then uses InMemoryFilesystemAdapter for any writes, with a second InMemoryFilesystemAdapter to account for deletes. The goal is to add --dry-run to my project. Once I start using third party libraries, I need your project to pass filepaths that will route back to my ReadOnlyFileSystem.

[elazar]

@BrianHenryIE I'm running your branch against PHP 7.4.33, Xdebug 3.1.6, and Flysystem 2.5.0. With no changes to the existing test suite, this line isn't covered.

I tried making the following modifications to an existing test in attempt to get that coverage.

diff --git a/tests/StreamWrapperTest.php b/tests/StreamWrapperTest.php
index 2d24b3a..584572d 100644
--- a/tests/StreamWrapperTest.php
+++ b/tests/StreamWrapperTest.php
@@ -40,11 +40,14 @@ afterEach(function () {
     $this->registry->unregister('fly');
 });

-it('can create and delete directories', function () {
+it('can detect, create, and delete directories', function () {
+    $this->assertFalse(is_dir('fly://foo'));
     $result = mkdir('fly://foo');
     expect($result)->toBeTrue();
+    $this->assertTrue(is_dir('fly://foo'));
     rmdir('fly://foo');
-});
+    $this->assertFalse(is_dir('fly://foo'));
+})->only();

 it('handles opening a nonexistent directory', function () {
     $dir = opendir('fly://foo');

Unfortunately, the test fails, and I'm having trouble sorting out why. Any ideas?

FAIL  Tests\StreamWrapperTest
  ⨯ it can detect, create, and delete directories

  ---

  • Tests\StreamWrapperTest > it can detect, create, and delete directories
  Failed asserting that false is true.

  at tests/StreamWrapperTest.php:47
     43▕ it('can detect, create, and delete directories', function () {
     44▕     $this->assertFalse(is_dir('fly://foo'));
     45▕     $result = mkdir('fly://foo');
     46▕     expect($result)->toBeTrue();
  ➜  47▕     $this->assertTrue(is_dir('fly://foo'));
     48▕     rmdir('fly://foo');
     49▕     $this->assertFalse(is_dir('fly://foo'));
     50▕ })->only();
     51▕


  Tests:  1 failed
  Time:   0.18s

[BrianHenryIE]

I noticed the directory created was called fly:. After a wrong turn applying the PathNormalizer inside StreamWrapper, I realised it should be passed to the FlySystem in the test.

c984bc5...2780b76

But... as in #9, InMemoryFilesystemAdapter does not support querying the visibility of directories. So the test can be shown to work by replacing it with:

  $this->filesystem = new Filesystem(
    new \League\Flysystem\Local\LocalFilesystemAdapter(
      sys_get_temp_dir() . '/' . uniqid('flystream')
    ),
    [],
    $pathNormalizer
  );

I'll take a look at backporting your TestInMemoryFilesystemAdapter soon.

[BrianHenryIE]

I think this is ready to go now.

[elazar]

@BrianHenryIE I'm wondering if this clearstatcache() call shouldn't be in StreamWrapper->rmdir() instead?

Below are some related bits of a conversation I had with Edorian in the #help channel on the phpcommunity.org Discord earlier about this. Please give them a look and let me know what you think.

statcache only every caches the last accessed file/dir. Some references:

rmdir: https://github.com/php/php-src/blob/064ea9c505889fb84b2b3fc41230be26cc58a345/ext/standard/file.c#L1152
unlink: https://github.com/php/php-src/blob/064ea9c505889fb84b2b3fc41230be26cc58a345/ext/standard/file.c#L1300

Which should go through https://github.com/php/php-src/blob/38501ed48a6a8d9ad57605d6a49e796230ac4b42/main/php_streams.h#L158 and end up :

plain_files_unlink https://github.com/php/php-src/blob/0fe3a91494e9aece3ae948cf9bf9d3476686e0a5/main/streams/plain_wrapper.c#L1269
php_plain_files_rmdir https://github.com/php/php-src/blob/0fe3a91494e9aece3ae948cf9bf9d3476686e0a5/main/streams/plain_wrapper.c#L1500

On empty directories everything looks fine to me:

<?php

$dir = __DIR__ . '/tmp';

@rmdir($dir);

mkdir($dir);
var_dump("Exists:");
var_dump(stat($dir)['mtime']);
rmdir($dir);
var_dump("Cached?");
var_dump(stat($dir)['mtime']);

=> php foo.php
string(7) "Exists:"
int(1738881324)
string(7) "Cached?"
PHP Warning:  stat(): stat failed for /Users/edo/private/experiments/tmp in /Users/edo/private/experiments/foo.php on line 14

So the second stat call is not stale/cached information?

https://www.php.net/manual/en/streamwrapper.rmdir.php doesn't mention this, but I'm guessing that an implementation of streamWrapper->rmdir() has to handle calling clearstatcache() internally rather than PHP handling it automatically.

Custom stream wrapper don't have to work on a local (or any) file system, so I don't think they can do anything automatically.

But if you call underlying system functions for file manipulation these should do a clearstatcache then. (Like LocalFilesystemAdapter should do?)

Yeah looks like you're right on url_stat being cached by stat cache

<?php

$x = new class {

   public $context;

   public function url_stat(string $path, int $flags): array|false {
       var_dump("url_stat: $path");
       return [];
   }
};

stream_wrapper_register('test', get_class($x));

$dir = 'test://foo/bar';

var_dump(is_dir($dir));
var_dump(is_dir($dir));
var_dump(is_dir($dir));

=> php foo.php
string(24) "url_stat: test://foo/bar"
bool(false)
bool(false)
bool(false)

But here neither unlink nor rmdir clears the stat cache with a trivial implementation by its self:

https://3v4l.org/s6tnC

So yeah, with the added context of this being stream wrappers this makes more sense

From what I can see for stream wrappers this isn't done by php, the reasoning for that is beyond me though (or my tests are wrong somehow).

[BrianHenryIE]

I agree, whether or not we expect it to be done by PHP, evidently it's leaving stat cache in an invalid state, so it seems correct to clear it inside StreamWrapper->rmdir().

[BrianHenryIE]

clearstatcache() is still in 'can rename an existing file' but tests pass for me even with that commented out.

[elazar]

@BrianHenryIE I've released this change in versions 0.5.0 and 1.3.0. Thanks again for your contribution!