Skip to content

[Filesystem] Add watch method to watch filesystem for changes #31462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Symfony/Component/Filesystem/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
CHANGELOG
=========

5.1.0
-----

* added `watch()` method to watch filesystem for changes

5.0.0
-----

Expand Down
22 changes: 22 additions & 0 deletions src/Symfony/Component/Filesystem/Filesystem.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
use Symfony\Component\Filesystem\Exception\FileNotFoundException;
use Symfony\Component\Filesystem\Exception\InvalidArgumentException;
use Symfony\Component\Filesystem\Exception\IOException;
use Symfony\Component\Filesystem\Watcher\FileChangeWatcher;
use Symfony\Component\Filesystem\Watcher\INotifyWatcher;

/**
* Provides basic utility to manipulate the file system.
Expand Down Expand Up @@ -695,6 +697,26 @@ public function appendToFile(string $filename, $content)
}
}

/**
* Watches a file or directory for any changes, and calls $callback when any changes are detected.
*
* @param mixed $path The path to watch for changes. Can be a path to a file or directory, iterator or array with paths
* @param callable $callback The callback to execute when a change is detected
* @param float $timeout The idle timeout in milliseconds after which the process will be aborted if there are no changes detected
*
* @throws \InvalidArgumentException|IOException
*/
public function watch($path, callable $callback, float $timeout = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to define events we want to listen (by default creating, changing, and deleting).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The events are passed to the callback function, so you can skip certain events already by adding an if statement in the callback

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some instructions take time to be executed and limit changes we want to watch can help to reduce useless time. Also, the change must in the watcher code to do it.

{
if (\extension_loaded('inotify')) {
$watcher = new INotifyWatcher();
} else {
$watcher = new FileChangeWatcher();
}

$watcher->watch($path, $callback, $timeout);
}

private function toIterable($files): iterable
{
return \is_array($files) || $files instanceof \Traversable ? $files : [$files];
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Filesystem\Tests\Fixtures;

use Symfony\Component\Filesystem\Watcher\FileChangeEvent;
use Symfony\Component\Filesystem\Watcher\Resource\ResourceInterface;

/**
* @author Pierre du Plessis <pdples@gmail.com>
*/
final class ChangeFileResource implements ResourceInterface
{
private $path;

public function __construct(string $path)
{
$this->path = $path;
}

public function detectChanges(): array
{
return [new FileChangeEvent($this->path, FileChangeEvent::FILE_CHANGED)];
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Filesystem\Tests\Watcher;

use Symfony\Component\Filesystem\Tests\FilesystemTestCase;
use Symfony\Component\Filesystem\Tests\Fixtures\ChangeFileResource;
use Symfony\Component\Filesystem\Watcher\FileChangeEvent;
use Symfony\Component\Filesystem\Watcher\FileChangeWatcher;
use Symfony\Component\Filesystem\Watcher\Resource\DirectoryResource;
use Symfony\Component\Filesystem\Watcher\Resource\ResourceInterface;

class FileSystemWatchTest extends FilesystemTestCase
{
public function testWatch()
{
$workspace = $this->workspace;

$locator = new class($workspace) {
private $workspace;

public function __construct($workspace)
{
$this->workspace = $workspace;
}

public function locate($path): ?ResourceInterface
{
return new ChangeFileResource($this->workspace.'/foobar.txt');
}
};

$watcher = new FileChangeWatcher();
$watcher->locator = $locator;

$count = 0;
$watcher->watch($this->workspace, function ($file, $code) use (&$count) {
$this->assertSame($this->workspace.'/foobar.txt', $file);
$this->assertSame(FileChangeEvent::FILE_CHANGED, $code);
++$count;

if (2 === $count) {
return false;
}
});

$this->assertSame(2, $count);
}

public function testWatchTimeout()
{
$locator = new class() {
public function locate($path): ?ResourceInterface
{
return new DirectoryResource($path);
}
};

$watcher = new FileChangeWatcher();
$ref = new \ReflectionProperty($watcher, 'locator');
$ref->setAccessible(true);
$ref->setValue($watcher, $locator);

$start = microtime(true);
$watcher->watch($this->workspace, static function ($file, $code) {
}, 500);

$this->assertGreaterThan(0.5, microtime(true) - $start);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Filesystem\Tests\Watcher\Resource;

use Symfony\Component\Filesystem\Tests\FilesystemTestCase;
use Symfony\Component\Filesystem\Watcher\FileChangeEvent;
use Symfony\Component\Filesystem\Watcher\Resource\ArrayResource;
use Symfony\Component\Filesystem\Watcher\Resource\FileResource;

class ArrayResourceTest extends FilesystemTestCase
{
public function testFileChange()
{
$file = $this->workspace.'/foo.txt';
touch($file);

$resource = new ArrayResource([new FileResource($file)]);

$this->assertSame([], $resource->detectChanges());

touch($file, time() + 1);

$this->assertEquals([new FileChangeEvent($file, FileChangeEvent::FILE_CHANGED)], $resource->detectChanges());
$this->assertSame([], $resource->detectChanges());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are testing here behaviour of FileResource, not ArrayResource, which is confusing. Changing return values on subsequent executions of detectChanges is FileResource specific defined behaviour. If these are unit tests, they should test only behaviour defined by ArrayResource itself, or be renamed to something else, since they aren't unit tests.

Same applies to all of the test classes in this namespace below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are testing here behaviour of FileResource, not ArrayResource

I don't understand this part. This is checking that ArrayResource correctly returns the detected changes for all the resources defined. The only other option is to create mocks for the changed events here, but I prefer relying on concrete implementations rather than mocking

Copy link
Contributor

@ostrolucky ostrolucky May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explained it at

Changing return values on subsequent executions of detectChanges is FileResource specific defined behaviour.

This is not something ArrayResource does, so this test case shouldn't explicitly test it. If you want this to be unit test, then indeed you need to provide mock instead of FileResource and assert that ArrayResource calls it when expected, because that's all it does. It won't return different results on subsequent calls for different implementations of decorated resource instance.

Or we can have generic test case like I explain in some other comment, then this is no longer important because there will be no ArrayResource specific test code of these, it will just test these things for consistency sake.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Filesystem\Tests\Watcher\Resource;

use Symfony\Component\Filesystem\Tests\FilesystemTestCase;
use Symfony\Component\Filesystem\Watcher\FileChangeEvent;
use Symfony\Component\Filesystem\Watcher\Resource\DirectoryResource;

class DirectoryResourceTest extends FilesystemTestCase
{
public function testCreateFile()
{
$dir = $this->workspace.\DIRECTORY_SEPARATOR.'foo';
mkdir($dir);

$resource = new DirectoryResource($dir);

$this->assertSame([], $resource->detectChanges());

touch($dir.'/foo.txt');

$this->assertEquals([new FileChangeEvent($dir.\DIRECTORY_SEPARATOR.'foo.txt', FileChangeEvent::FILE_CREATED)], $resource->detectChanges());
$this->assertSame([], $resource->detectChanges());
}

public function testDeleteFile()
{
$dir = $this->workspace.\DIRECTORY_SEPARATOR.'foo';
mkdir($dir);

touch($dir.'/foo.txt');
touch($dir.'/bar.txt');

$resource = new DirectoryResource($dir);

$this->assertSame([], $resource->detectChanges());

unlink($dir.'/foo.txt');

$this->assertEquals([new FileChangeEvent($dir.\DIRECTORY_SEPARATOR.'foo.txt', FileChangeEvent::FILE_DELETED)], $resource->detectChanges());
$this->assertSame([], $resource->detectChanges());
}

public function testFileChanges()
{
$dir = $this->workspace.\DIRECTORY_SEPARATOR.'foo';
mkdir($dir);

touch($dir.'/foo.txt');
touch($dir.'/bar.txt');

$resource = new DirectoryResource($dir);

$this->assertSame([], $resource->detectChanges());

touch($dir.'/foo.txt', time() + 1);

$this->assertEquals([new FileChangeEvent($dir.\DIRECTORY_SEPARATOR.'foo.txt', FileChangeEvent::FILE_CHANGED)], $resource->detectChanges());
$this->assertSame([], $resource->detectChanges());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Filesystem\Tests\Watcher\Resource;

use Symfony\Component\Filesystem\Tests\FilesystemTestCase;
use Symfony\Component\Filesystem\Watcher\FileChangeEvent;
use Symfony\Component\Filesystem\Watcher\Resource\FileResource;

class FileResourceTest extends FilesystemTestCase
{
public function testFileChanges()
{
$file = $this->workspace.'/foo.txt';
touch($file);

$resource = new FileResource($file);

$this->assertSame([], $resource->detectChanges());

touch($file, time() + 1);

$this->assertEquals([new FileChangeEvent($file, FileChangeEvent::FILE_CHANGED)], $resource->detectChanges());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying same code over and over is a sign it needs extraction. Reducing PR size is also needed to incentive people to review it, larger PRs put people off reviewing it. I would suggest to have abstract test class where most of this code is defined once only and subclasses should define only target class and file location - in other words, only code that differs. Another side benefit is guarantee that all subclasses are tested for all operations. Currently it seems only DirectoryResource is fully tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reducing PR size is also needed to incentive people to review it, larger PRs put people off reviewing it

For some people maybe, but how do other big features than make its way into the codebase?
Abstracting a single line to be re-used does not seem to have a big benefit.

Currently it seems only DirectoryResource is fully tested.

How so? If you think there are test cases missing, let me know and I will gladly add more tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Abstracting a single line to be re-used does not seem to have a big benefit.

It's not a single line. It's everything except two lines of code like I mentioned at

subclasses should define only target class and file location

Pretty much everything else is currently copy pasted

How so? If you think there are test cases missing, let me know and I will gladly add more tests

testCreateFile, testDeleteFile are missing from FileResourceTest, ArrayResourceTest. I don't want more code to be copy pasted though.

$this->assertSame([], $resource->detectChanges());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Filesystem\Tests\Watcher\Resource\Locator;

use Symfony\Component\Filesystem\Tests\FilesystemTestCase;
use Symfony\Component\Filesystem\Watcher\Resource\ArrayResource;
use Symfony\Component\Filesystem\Watcher\Resource\DirectoryResource;
use Symfony\Component\Filesystem\Watcher\Resource\FileResource;
use Symfony\Component\Filesystem\Watcher\Resource\Locator\FileResourceLocator;

class FileResourceLocatorTest extends FilesystemTestCase
{
public function testLocateIterator()
{
$locator = new FileResourceLocator();

$path = new \ArrayIterator([$this->createFile('foo.txt')]);

$this->assertEquals(new ArrayResource([new FileResource($this->workspace.\DIRECTORY_SEPARATOR.'foo.txt')]), $locator->locate($path));
}

public function testLocateSplFileInfo()
{
$locator = new FileResourceLocator();

$path = new \SplFileInfo($this->createFile('foo.txt'));

$this->assertEquals(new FileResource($this->workspace.\DIRECTORY_SEPARATOR.'foo.txt'), $locator->locate($path));
}

public function testFilePath()
{
$locator = new FileResourceLocator();

$path = $this->createFile('foo.txt');

$this->assertEquals(new FileResource($this->workspace.\DIRECTORY_SEPARATOR.'foo.txt'), $locator->locate($path));
}

public function testGlob()
{
$locator = new FileResourceLocator();

$this->createFile('bar.txt');
$this->createFile('foo.txt');

$this->assertEquals(
new ArrayResource([new FileResource($this->workspace.\DIRECTORY_SEPARATOR.'bar.txt'), new FileResource($this->workspace.\DIRECTORY_SEPARATOR.'foo.txt')]),
$locator->locate($this->workspace.\DIRECTORY_SEPARATOR.'*.txt')
);
}

public function testArray()
{
$locator = new FileResourceLocator();

$path = [$this->createFile('foo.txt')];

$this->assertEquals(new ArrayResource([new FileResource($this->workspace.\DIRECTORY_SEPARATOR.'foo.txt')]), $locator->locate($path));
}

public function testDirectory()
{
$locator = new FileResourceLocator();

$dir = $this->createDirecty('foobar');

$this->assertEquals(new DirectoryResource($this->workspace.\DIRECTORY_SEPARATOR.'foobar'), $locator->locate($dir));
}

private function createFile(string $file)
{
$fullPath = $this->workspace.\DIRECTORY_SEPARATOR.$file;
touch($fullPath);

return $fullPath;
}

private function createDirecty(string $dir)
{
$fullPath = $this->workspace.\DIRECTORY_SEPARATOR.$dir;

mkdir($fullPath, 0777, true);

return $fullPath;
}
}
Loading