Skip to content

Commit b4870b1

Browse files
uuf6429uuf6429
authored
feat: Improve handling errors & edge cases in file operations (#367)
Consolidates more file handling code to reduce duplication and improve error handling. PHP errors that occur during some filesystem operations will now be detected and included in the exception message, regardless of the user's error_handling configuration. This in turn solves some PHPStan warnings when we were not previously handling a potential `false` return from filesystem operations. Extracted from #364.
1 parent ca5d00b commit b4870b1

File tree

12 files changed

+136
-24
lines changed

12 files changed

+136
-24
lines changed

‎src/Cache/FileCache.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ public function isFresh(string $path, int $timestamp)
7373
return false;
7474
}
7575

76-
return filemtime($cachePath) > $timestamp;
76+
return Filesystem::getLastModified($cachePath) > $timestamp;
7777
}
7878

7979
/**

‎src/Filesystem.php‎

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,14 @@
1111
namespace Behat\Gherkin;
1212

1313
use Behat\Gherkin\Exception\FilesystemException;
14+
use ErrorException;
1415
use JsonException;
1516
use RecursiveDirectoryIterator;
1617
use RecursiveIteratorIterator;
1718
use SplFileInfo;
1819

20+
use function assert;
21+
1922
/**
2023
* @internal
2124
*/
@@ -26,12 +29,18 @@ final class Filesystem
2629
*/
2730
public static function readFile(string $fileName): string
2831
{
29-
$data = @file_get_contents($fileName);
30-
if ($data === false) {
31-
throw new FilesystemException("Failed to read file: $fileName");
32+
try {
33+
$result = self::callSafely(static fn () => file_get_contents($fileName));
34+
} catch (ErrorException $e) {
35+
throw new FilesystemException(
36+
sprintf('File "%s" cannot be read: %s', $fileName, $e->getMessage()),
37+
previous: $e,
38+
);
3239
}
3340

34-
return $data;
41+
assert($result !== false, 'file_get_contents() should not return false without emitting a PHP warning');
42+
43+
return $result;
3544
}
3645

3746
/**
@@ -43,7 +52,7 @@ public static function readJsonFileArray(string $fileName): array
4352
{
4453
$result = json_decode(self::readFile($fileName), true, flags: JSON_THROW_ON_ERROR);
4554

46-
\assert(is_array($result), 'File must contain JSON with an array at its root');
55+
assert(is_array($result), 'File must contain JSON with an array at its root');
4756

4857
return $result;
4958
}
@@ -67,4 +76,53 @@ public static function findFilesRecursively(string $path, string $pattern): arra
6776

6877
return $found;
6978
}
79+
80+
public static function getLastModified(string $fileName): int
81+
{
82+
try {
83+
$result = self::callSafely(static fn () => filemtime($fileName));
84+
} catch (ErrorException $e) {
85+
throw new FilesystemException(
86+
sprintf('Last modification time of file "%s" cannot be found: %s', $fileName, $e->getMessage()),
87+
previous: $e,
88+
);
89+
}
90+
91+
assert($result !== false, 'filemtime() should not return false without emitting a PHP warning');
92+
93+
return $result;
94+
}
95+
96+
public static function getRealPath(string $path): string
97+
{
98+
$result = realpath($path);
99+
100+
if ($result === false) {
101+
throw new FilesystemException("Cannot retrieve the real path of $path");
102+
}
103+
104+
return $result;
105+
}
106+
107+
/**
108+
* @template TResult
109+
*
110+
* @param (callable(): TResult) $callback
111+
*
112+
* @return TResult
113+
*
114+
* @throws ErrorException
115+
*/
116+
private static function callSafely(callable $callback): mixed
117+
{
118+
set_error_handler(
119+
static fn (int $severity, string $message, string $file, int $line) => throw new ErrorException($message, 0, $severity, $file, $line)
120+
);
121+
122+
try {
123+
return $callback();
124+
} finally {
125+
restore_error_handler();
126+
}
127+
}
70128
}

‎src/Filter/PathsFilter.php‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010

1111
namespace Behat\Gherkin\Filter;
1212

13+
use Behat\Gherkin\Exception\FilesystemException;
14+
use Behat\Gherkin\Filesystem;
1315
use Behat\Gherkin\Node\FeatureNode;
1416
use Behat\Gherkin\Node\ScenarioInterface;
1517

@@ -33,18 +35,27 @@ class PathsFilter extends SimpleFilter
3335
public function __construct(array $paths)
3436
{
3537
foreach ($paths as $path) {
36-
if (($realpath = realpath($path)) === false) {
38+
try {
39+
$realpath = Filesystem::getRealPath($path);
40+
} catch (FilesystemException) {
3741
continue;
3842
}
43+
3944
$this->filterPaths[] = rtrim($realpath, DIRECTORY_SEPARATOR)
4045
. (is_dir($realpath) ? DIRECTORY_SEPARATOR : '');
4146
}
4247
}
4348

4449
public function isFeatureMatch(FeatureNode $feature)
4550
{
46-
foreach ($this->filterPaths as $path) {
47-
if (str_starts_with(realpath($feature->getFile()), $path)) {
51+
if (($filePath = $feature->getFile()) === null) {
52+
return false;
53+
}
54+
55+
$realFeatureFilePath = Filesystem::getRealPath($filePath);
56+
57+
foreach ($this->filterPaths as $filterPath) {
58+
if (str_starts_with($realFeatureFilePath, $filterPath)) {
4859
return true;
4960
}
5061
}

‎src/Loader/AbstractFileLoader.php‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010

1111
namespace Behat\Gherkin\Loader;
1212

13+
use Behat\Gherkin\Filesystem;
14+
1315
/**
1416
* Abstract filesystem loader.
1517
*
@@ -29,7 +31,7 @@ abstract class AbstractFileLoader implements FileLoaderInterface
2931
*/
3032
public function setBasePath(string $path)
3133
{
32-
$this->basePath = realpath($path);
34+
$this->basePath = Filesystem::getRealPath($path);
3335
}
3436

3537
/**
@@ -57,16 +59,15 @@ protected function findRelativePath(string $path)
5759
*/
5860
protected function findAbsolutePath(string $path)
5961
{
60-
if (is_file($path) || is_dir($path)) {
62+
if (file_exists($path)) {
6163
return realpath($path);
6264
}
6365

6466
if ($this->basePath === null) {
6567
return false;
6668
}
6769

68-
if (is_file($this->basePath . DIRECTORY_SEPARATOR . $path)
69-
|| is_dir($this->basePath . DIRECTORY_SEPARATOR . $path)) {
70+
if (file_exists($this->basePath . DIRECTORY_SEPARATOR . $path)) {
7071
return realpath($this->basePath . DIRECTORY_SEPARATOR . $path);
7172
}
7273

‎src/Loader/GherkinFileLoader.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
namespace Behat\Gherkin\Loader;
1212

1313
use Behat\Gherkin\Cache\CacheInterface;
14+
use Behat\Gherkin\Filesystem;
1415
use Behat\Gherkin\Node\FeatureNode;
1516
use Behat\Gherkin\ParserInterface;
1617

@@ -72,7 +73,7 @@ public function load(mixed $resource)
7273
{
7374
$path = $this->getAbsolutePath($resource);
7475
if ($this->cache) {
75-
if ($this->cache->isFresh($path, filemtime($path))) {
76+
if ($this->cache->isFresh($path, Filesystem::getLastModified($path))) {
7677
$feature = $this->cache->read($path);
7778
} elseif (null !== $feature = $this->parseFeature($path)) {
7879
$this->cache->write($path, $feature);

‎tests/Cache/FileCacheTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ public function testBrokenCacheRead(): void
7777
file_put_contents($files[0], '');
7878

7979
$this->expectException(CacheException::class);
80-
$this->expectExceptionMessageMatches('/^Can not load cache for a feature "broken_feature" from (.+)$/');
80+
$this->expectExceptionMessageMatches('/^Can not load cache for a feature "broken_feature" from .+$/');
8181

8282
$cache->read('broken_feature');
8383
}
@@ -87,7 +87,7 @@ public function testMissingCacheFileRead(): void
8787
$cache = $this->createCache();
8888

8989
$this->expectException(CacheException::class);
90-
$this->expectExceptionMessageMatches('/^Can not load cache: Failed to read file: (.+)$/');
90+
$this->expectExceptionMessageMatches('/^Can not load cache: File "[^"]+" cannot be read: .+$/');
9191

9292
$cache->read('missing_file');
9393
}

‎tests/FilesystemTest.php‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Behat Gherkin Parser.
5+
* (c) Konstantin Kudryashov <ever.zet@gmail.com>
6+
*
7+
* For the full copyright and license information, please view the LICENSE
8+
* file that was distributed with this source code.
9+
*/
10+
11+
namespace Tests\Behat\Gherkin;
12+
13+
use Behat\Gherkin\Exception\FilesystemException;
14+
use Behat\Gherkin\Filesystem;
15+
use PHPUnit\Framework\TestCase;
16+
17+
final class FilesystemTest extends TestCase
18+
{
19+
public function testInexistentFileCannotHaveModificationTime(): void
20+
{
21+
$this->expectExceptionObject(new FilesystemException(
22+
'Last modification time of file "inexistent-file.txt" cannot be found: filemtime(): stat failed for inexistent-file.txt',
23+
));
24+
25+
Filesystem::getLastModified('inexistent-file.txt');
26+
}
27+
}

‎tests/Filter/PathsFilterTest.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,4 +76,14 @@ public function testIsScenarioMatchFilter(): void
7676

7777
$this->assertFalse($filter->isScenarioMatch($scenario));
7878
}
79+
80+
public function testMissingFileShouldBeSkipped(): void
81+
{
82+
$fixtures = __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures' . DIRECTORY_SEPARATOR;
83+
$feature = new FeatureNode(null, null, [], null, [], '', '', null, 1);
84+
85+
$filter = new PathsFilter([$fixtures . 'full']);
86+
87+
$this->assertFalse($filter->isFeatureMatch($feature));
88+
}
7989
}

‎tests/Loader/DirectoryLoaderTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ protected function setUp(): void
2727
$this->gherkin = $this->createGherkinMock();
2828
$this->loader = new DirectoryLoader($this->gherkin);
2929

30-
$this->featuresPath = (string) realpath(__DIR__ . '/../Fixtures/directories');
30+
$this->featuresPath = dirname(__DIR__) . DIRECTORY_SEPARATOR . 'Fixtures' . DIRECTORY_SEPARATOR . 'directories';
3131
}
3232

3333
protected function createGherkinMock(): MockObject&Gherkin

‎tests/Loader/GherkinFileLoaderTest.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ protected function setUp(): void
2727
$parser = new Parser(new Lexer(new CucumberDialectProvider()));
2828
$this->loader = new GherkinFileLoader($parser);
2929

30-
$this->featuresPath = (string) realpath(__DIR__ . '/../Fixtures/features');
30+
$this->featuresPath = dirname(__DIR__) . DIRECTORY_SEPARATOR . 'Fixtures' . DIRECTORY_SEPARATOR . 'features';
3131
}
3232

3333
public function testSupports(): void
@@ -106,12 +106,12 @@ public function testBasePath(): void
106106
$features = $this->loader->load('features/pystring.feature');
107107
$this->assertCount(1, $features);
108108
$this->assertEquals('A py string feature', $features[0]->getTitle());
109-
$this->assertEquals(realpath($this->featuresPath . DIRECTORY_SEPARATOR . 'pystring.feature'), $features[0]->getFile());
109+
$this->assertEquals($this->featuresPath . DIRECTORY_SEPARATOR . 'pystring.feature', $features[0]->getFile());
110110

111111
$this->loader->setBasePath($this->featuresPath);
112112
$features = $this->loader->load('multiline_name.feature');
113113
$this->assertCount(1, $features);
114114
$this->assertEquals('multiline', $features[0]->getTitle());
115-
$this->assertEquals(realpath($this->featuresPath . DIRECTORY_SEPARATOR . 'multiline_name.feature'), $features[0]->getFile());
115+
$this->assertEquals($this->featuresPath . DIRECTORY_SEPARATOR . 'multiline_name.feature', $features[0]->getFile());
116116
}
117117
}

0 commit comments

Comments
 (0)